-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Database: Add autoincrement, uniqueness, and sync-writes polyfills #28
Conversation
9cc237c
to
8158fa6
Compare
6b65a19
to
600f8ac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx @amotl !
Nice stuff, left some suggestions on the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx! LGTM, but please wait for one more review.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"when using the SQLAlchemy dialect" is better. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple of small suggestions.
Sources: MLflow, LangChain, Singer/Meltano, rdflib-sqlalchemy
Co-authored-by: Marios Trivyzas <[email protected]> Co-authored-by: Simon Prickett <[email protected]>
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd prefer moving this quoting logic into the refresh_table
function, by change it's signature to consume an optional schema. This would avoid that it will be called with unquoted or invalid relation names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should work. Thanks!
About
Add a few polyfills conceived on behalf of the LangChain adapter, and patches to
rdflib-sqlalchemy
as 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
UNIQUE
constraints #76AUTOINCREMENT
columns #77REFRESH TABLE
#83Backlog