Database: Add autoincrement, uniqueness, and sync-writes polyfills#28
Database: Add autoincrement, uniqueness, and sync-writes polyfills#28
Conversation
amotl
force-pushed
the
amo/polyfill
branch
4 times, most recently
from
9cc237c to
8158fa6
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
amotl
force-pushed
the
amo/polyfill
branch
4 times, most recently
from
6b65a19 to
600f8ac
Compare
matriv
left a comment
matriv
left a comment
There was a problem hiding this comment.
thx! LGTM, but please wait for one more review.
amotl
force-pushed
the
amo/polyfill
branch
2 times, most recently
from
11076e8 to
5e39bbf
Compare
docs/index.rst
Outdated
|
|
||
| The package bundles a few support and utility functions that try to fill a few | ||
| gaps you will observe when working with CrateDB, when compared with other | ||
| databases. As a distributed OLAP database, it naturally does not include certain |
There was a problem hiding this comment.
I think that we shouldn't mention as a distributed OLAP, specially combining it with naturally after that, I would simplify and say that CrateDB's behavior and features deviate from those found usually in an RDBMS.
There was a problem hiding this comment.
Maybe something like "Due to its distributed nature, CrateDB's behavior and features differ from those found in other RDBMS systems".
docs/overview.rst
Outdated
| which enable automatically assigning incremental values when inserting records. | ||
| However, it offers server-side support by providing an SQL function to generate | ||
| random identifiers of ``STRING`` type, and client-side support for generating | ||
| ``INTEGER``-based identifiers on behalf of the SQLAlchemy dialect. |
There was a problem hiding this comment.
Not sure what "on behalf of" means here - does this mean "when using the SQLAlchemy dialect" or something else? "on behalf of" implies there's some ID generating function that can be plugged into SQLAlchemy. I don't know SQLAlchemy so am not sure what happens here.
There was a problem hiding this comment.
"when using the SQLAlchemy dialect" is better. Thanks.
simonprickett
left a comment
simonprickett
left a comment
There was a problem hiding this comment.
Left a couple of small suggestions.
Sources: MLflow, LangChain, Singer/Meltano, rdflib-sqlalchemy
Co-authored-by: Marios Trivyzas <5058131+matriv@users.noreply.github.com> Co-authored-by: Simon Prickett <simon@crudworks.org>
|
@matriv and @simonprickett: Thanks a stack for your excellent reviews. 💯 |
| full_table_name = f'"{clauseelement.table.name}"' | ||
| if clauseelement.table.schema is not None: | ||
| full_table_name = f'"{clauseelement.table.schema}".' + full_table_name |
There was a problem hiding this comment.
I think I'd prefer moving this quoting logic into the refresh_tablefunction, by change it's signature to consume an optional schema. This would avoid that it will be called with unquoted or invalid relation names.
4 participants
About
Add a few polyfills conceived on behalf of the LangChain adapter, and patches to
rdflib-sqlalchemyas well asmeltano-target-cratedb. By adding them here, they can be used by downstream applications without needing to installcratedb-toolkit.Documentation
Preview: https://sqlalchemy-cratedb--28.org.readthedocs.build/support.html
@simonprickett: I would also be happy about your review on documentation syntax/grammar/wording, as I am not an English native speaker. Thanks!
References
UNIQUEconstraints #76AUTOINCREMENTcolumns #77REFRESH TABLE#83Backlog