Fix: CLOB, NCHAR, NVARCHAR, DATETIME and DATE compile to DDL CrateDB cannot parse - #305
Conversation
…eDB compatible types
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| def visit_DATETIME(self, type_, **kw): | ||
| return self.visit_TIMESTAMP(type_, **kw) | ||
|
|
||
| def visit_DATE(self, type_, **kw): |
There was a problem hiding this comment.
There is an open question how to handle DATE type in CrateDB >=6.5, We can store them as TIMESTAMP but they will cost "not implementing" new type storage. Maybe implementing crate version check?
There was a problem hiding this comment.
there's also the case of casting as DATE.
sqlalchemy can tell the two apart: when it renders the type of a column, it passes the column as type_expression. So visit_DATE can return TIMESTAMP for columns only, if our get_column_specification passes type_expression=column to it. visit_ARRAY must also pass **kw through, or ARRAY(sa.DATE) columns still come out as ARRAY(DATE). #300 already does that
Before 6.5, we could cast to timestamp midnight UTC or smth, or raise an exception - I think raising is better, since it shows crate doesn't support that at compile time, rather than maybe introducing a new timezone bug. But this ofc means we don't have casts to date before 6.5.
florinutz
left a comment
florinutz
left a comment
There was a problem hiding this comment.
good work! got some observations
| "BOOLEAN": lambda: sa.BOOLEAN, | ||
| "CHAR": lambda: sa.CHAR(5), | ||
| "CLOB": lambda: sa.CLOB, | ||
| "DATE": lambda: sa.DATE, |
There was a problem hiding this comment.
This test can't catch a mistake in the DATE mapping. CI only uses the nightly build (the unreleased 6.5), and 6.5 can store a DATE column. So the DATE row passes even without visit_DATE. I checked it: I removed the compiler change and kept the tests. CLOB, NCHAR, NVARCHAR and DATETIME failed, but DATE passed.
Could we test the five mappings in tests/create_table_test.py instead, next to test_table_time_type? Those tests compare the exact CREATE TABLE text through a fake cursor. They need no server, so the result is the same for every CrateDB version. A cast case there would also cover the DATE/TIMESTAMP split.
| "TEXT": lambda: sa.TEXT, | ||
| "TIME": lambda: sa.TIME, | ||
| "TIMESTAMP": lambda: sa.TIMESTAMP, | ||
| "UUID": lambda: sa.UUID, |
There was a problem hiding this comment.
crate only added uuid in 6.2.0, so this would need a skip for versions < 6.2.0, if we ever move this CI to run against other versions than nightly
| @@ -246,6 +255,12 @@ def visit_datetime(self, type_, **kw): | |||
| def visit_date(self, type_, **kw): | |||
There was a problem hiding this comment.
visit_date and visit_datetime are now redundant, right?
2 participants
Summary of the changes / Why this is an improvement
tests/test_type_compilation.pytests to cover all CrateDB supported types, it tests all supported types to extend coverage. (it's a bit out of scope of this ticket but I wanted to extend the test coverage)Discovered while working on #12 and moved changes on this PR.
Checklist