-
Notifications
You must be signed in to change notification settings - Fork 0
Switch to new SQLAlchemy dialect for CrateDB #15
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #15 +/- ##
==========================================
+ Coverage 86.11% 88.87% +2.75%
==========================================
Files 7 6 -1
Lines 713 665 -48
==========================================
- Hits 614 591 -23
+ Misses 99 74 -25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
pyproject.toml
Outdated
| "cratedb-toolkit", | ||
| 'importlib-resources; python_version < "3.9"', # "meltanolabs-target-postgres==0.0.9", | ||
| "meltanolabs-target-postgres@ git+https://github.com/singer-contrib/meltanolabs-target-postgres.git@pgvector", | ||
| "sqlalchemy-cratedb[vector]@ git+https://github.com/crate-workbench/sqlalchemy-cratedb@amo/type-float-vector", |
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.
Of course, both of them need to be released before a serious GA release of meltano-target-cratedb.
0c5d1ba to
90a88a6
Compare
This includes the `FloatVector` SQLAlchemy type.
90a88a6 to
148a2d5
Compare
| TYPES_MAP["real_array"] = ARRAY(sqltypes.DOUBLE) | ||
| TYPES_MAP["timestamp without time zone"] = sqltypes.TIMESTAMP | ||
| TYPES_MAP["timestamp with time zone"] = sqltypes.TIMESTAMP | ||
| # abc() |
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.
remove? Otherwise, what does this mean?
| json_encoder_default = CrateJsonEncoder.default | ||
| json_encoder_default = crate.client.http.json_encoder | ||
|
|
||
| def default(self, o): | ||
| if isinstance(o, Decimal): | ||
| return float(o) | ||
| return json_encoder_default(o) | ||
| def json_encoder_new(obj: Any) -> Union[int, str, float]: | ||
| if isinstance(obj, Decimal): | ||
| return float(obj) | ||
| return json_encoder_default(obj) | ||
|
|
||
| CrateJsonEncoder.default = default | ||
| crate.client.http.json_encoder = json_encoder_new |
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.
That patch should go into crate/crate-python instead?
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.
Hm. We are still in a limbo whether to forward Decimal type values as float or string?
About
The new SQLAlchemy dialect for CrateDB includes the
FloatVectortype. This patch intends to validate that everything works well, and removes the previous implementation, which originally was conceived here.References
FLOAT_VECTORdata type andKNN_MATCHfunction crate/sqlalchemy-cratedb#9/cc @hammerhead, @surister, @ckurze