Repository navigation
fix(cratedb): recognize reflected timestamp types for epoch milliseconds - #44720
Conversation
Code Review Agent Run #fcb6b3Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The flagged issue is correct. The current implementation of To resolve this, you should ensure that the @classmethod
def alter_new_orm_column(cls, orm_col: TableColumn) -> None:
if orm_col.type in {
"TIMESTAMP",
"TIMESTAMP WITHOUT TIME ZONE",
"TIMESTAMP WITH TIME ZONE",
} and not orm_col.python_date_format:
orm_col.python_date_format = "epoch_ms"This change ensures that existing formats are preserved while correctly setting the format for new timestamp columns. Would you like me to fetch all other comments on this PR to validate and implement fixes for them as well? superset/db_engine_specs/crate.py |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #44720 +/- ##
=======================================
Coverage 81.12% 81.13%
=======================================
Files 2956 2956
Lines 178434 178449 +15
Branches 41339 41343 +4
=======================================
+ Hits 144762 144776 +14
- Misses 30967 30969 +2
+ Partials 2705 2704 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code Review Agent Run #d694f6Actionable Suggestions - 0Additional Suggestions - 2
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
LGTM. Handling this in fetch_data instead of only the alter_new_orm_column hook is the right call, existing columns get fixed without a metadata migration. Good defensive coverage too, native datetimes and non-timestamp type codes pass through untouched, and the empty _result fallback doesn't crash. codeant's incomplete-implementation catch was real and you fixed it the right way. Approving.
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
Request changes: correct, well-tested fix with a clean RCA and no security-model impact — but two cheap robustness guards on the fetch path should land before merge.
🟡 Medium — Out-of-range timestamp now crashes the whole query
The new conversion in fetch_data (superset/db_engine_specs/crate.py:~120), datetime(1970,1,1) + timedelta(milliseconds=value), runs after super().fetch_data() returns, i.e. outside the base method's try/except (base.py:1522-1550). A single out-of-range epoch-ms value (sentinel/garbage, or anything past year 9999) raises OverflowError and aborts the entire result set. Previously the raw int flowed to normalize_dttm_col, which uses pd.to_datetime(..., errors="coerce") and degrades to NaT — so this converts graceful degradation into a query-killing crash. Fix: wrap the per-value conversion in try/except (OverflowError, ValueError, OSError) and fall back to leaving the raw value; add a regression test with an out-of-range value.
🟡 Medium — cursor._result present-but-None raises AttributeError
getattr(cursor, "_result", {}).get("col_types", []) only defends against the attribute being absent. The crate DBAPI cursor initializes _result = None, so a present-but-None value makes .get(...) raise AttributeError — again outside any try/except. Tests exercise {} but never None. Fix: (getattr(cursor, "_result", None) or {}).get("col_types", []), and add a _result = None test case.
🟢 Low — Fix silently no-ops if the private-driver contract changes
The whole fix rests on the undocumented private attribute cursor._result["col_types"] with type codes 11/15. If a driver upgrade renames/restructures it, the getattr default returns [] and the fix silently reverts to the buggy behavior with no signal. The access is unavoidable (CrateDB's DBAPI omits type codes from description — that's the root cause) and adequately guarded, so this isn't blocking. Suggestion: pin/comment the verified crate driver version and emit a logger.debug when description implies a timestamp column but no col_types is found, so silent no-ops are detectable.
🟢 Low — Array-of-timestamp columns remain unconverted
For array columns the type code is a list (e.g. [100, 15]), so type_code in (11, 15) is False and epoch-ms values inside arrays stay raw ints. This is a reasonable scope boundary, but the PR body implies arrays "retain appropriate behavior" — worth calling out explicitly as a known limitation rather than as fully handled.
🟢 Low — Magic wire-protocol constants 11 / 15
The type codes are inline literals documented only by an adjacent comment. Extracting named module constants (e.g. CRATE_TYPE_TIMESTAMP_WITH_TZ = 11, CRATE_TYPE_TIMESTAMP_WITHOUT_TZ = 15) would make intent self-documenting and reusable. Cosmetic.
🟢 Low — Potential IndexError if col_types is wider than a row
timestamp_indexes comes purely from col_types; values[index] assumes each index is valid for every row. A driver quirk where col_types reports more entries than the row tuple would raise IndexError (unhandled, same out-of-try position). Cheap guard: if index < len(values).
Nits (non-blocking): fetch_data is on the shared results path (SQL Lab, CSV export), so timestamp columns that previously surfaced as epoch-ms integers there will now surface as datetimes — likely desirable, but a user-visible change with no note in the PR body or a SQL Lab test. And the conversion rebuilds every row (list(row)→tuple(...)) as a second full pass on top of the base copy path; minor for large result sets.
Confirmed safe (checked, not issues): double-conversion for new datasets is safe (_process_datetime_column takes the already-formatted pd.Timestamp branch for datetime input); bool is correctly excluded from int/float conversion; the fast path skips untouched result sets; no migration/UPDATING.md needed; and the change is CLAUDE.md-style clean (type hints present, no any, flat test_* + parametrize, no Enzyme/describe nesting).
Net: solid, tightly-scoped bug fix with unusually thorough tests. The only blockers are the two cheap guards (OverflowError and _result is None) — both fail on the shared fetch path and both are a couple of lines. Everything else is optional polish.
EnxDev
left a comment
EnxDev
left a comment
There was a problem hiding this comment.
Late pass, since this already merged. The approach holds up: decoding in fetch_data fixes existing datasets without touching metadata, and datasets that do have epoch_ms don't get converted twice, because _process_datetime_column takes the non-numeric branch once the values are datetimes.
One follow-up worth doing, left inline: out-of-range timestamps raise here now instead of passing through.
On the other open point from the earlier review, I don't think a _result is None guard is needed. The crate cursor starts _result as {}, every execute() replaces it with the response dict, and fetchall() would already have raised before we reach this line if nothing ran.
| if isinstance(value, (int, float)) and not isinstance(value, bool): | ||
| # UTC-naive matches Superset's datetime normalization and | ||
| # avoids interpreting epoch milliseconds as nanoseconds. | ||
| values[index] = datetime(1970, 1, 1) + timedelta(milliseconds=value) |
There was a problem hiding this comment.
Python datetimes stop at years 1 to 9999, but CrateDB timestamps go well past that in both directions. A single row like '10000-01-01'::timestamp raises OverflowError here, so the whole SQL Lab query or chart fails where it used to just return the int.
Could a follow-up catch OverflowError and leave the raw value? A test row with an out-of-range value alongside the -1 case would pin it down.
try:
values[index] = datetime(1970, 1, 1) + timedelta(milliseconds=value)
except OverflowError:
pass
4 participants
SUMMARY
CrateDB returns timestamps as epoch-millisecond integers, while its DBAPI cursor description omits type codes. Dataset columns reflected as
TIMESTAMP WITHOUT TIME ZONEorTIMESTAMP WITH TIME ZONEdid not receiveepoch_ms, causing chart normalization to interpret the values as nanoseconds.Recognize all three timestamp spellings when initializing columns. More importantly, decode scalar timestamp results to UTC-naive Python datetimes in the CrateDB engine spec using the authoritative HTTP result type codes (11 and 15). This fixes existing datasets as well, independently of their stored
python_date_format. Integers, decimals, arrays, nulls, and already-converted datetimes retain their appropriate behavior; fetching and exception handling still delegate to the base implementation.Dataset refresh only invokes
alter_new_orm_columnfor new columns. It does not repair existing columns, so the result-conversion fix is necessary. No metadata migration or manual configuration is required.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Daily chart buckets for January 2024 were normalized into January 1970. The same saved charts now return January 2024, with dataset/column/chart IDs and null stored date formats unchanged.
TESTING INSTRUCTIONS
pytest tests/unit_tests/db_engine_specs/test_crate.py -q: the creation-hook regression initially gave 2 failures; the additional result-conversion regression gives 6 failed / 13 passed with the creation-only fix, and 19 passed with both fixes.ADDITIONAL INFORMATION