-
Notifications
You must be signed in to change notification settings - Fork 1k
feat(duckdb): Add transpilation support for nanoseconds used in date/time functions. #6617
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
Conversation
SQLGlot Integration Test ResultsComparing:
By Dialect
Overallmain: 6173 total, 5610 passed (pass rate: 90.9%), sqlglot version: sqlglot:RD-1069375-timediff: 6173 total, 5610 passed (pass rate: 90.9%), sqlglot version: Difference: No change |
VaggelisD
left a comment
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.
Nice work @fivetran-amrutabhimsenayachit! Any time related transpilation is tricky by nature, leaving a few comments:
- Remove redundant TIMESTAMP_NS casting in _timediff_sql (lines 268-271)
* Previously cast to TIMESTAMP_NS before calling _handle_nanosecond_diff
* _handle_nanosecond_diff already handles casting via exp.cast (which avoids recasting)
* Now passes expressions directly, matching _date_diff_sql pattern
- Fix TIMEDIFF test case to use valid Snowflake syntax
* Changed from TIME literal '10:00:00.000000000'
* To TIMESTAMP with CAST: CAST('2023-01-01 10:00:00.000000000' AS TIMESTAMP)
* TIME literals don't work with TIMEDIFF in Snowflake
- Fix TIMEADD test case to use valid Snowflake syntax
* Changed from TIME literal '10:00:00.000000000'
* To TIMESTAMP with CAST: CAST('2023-01-01 10:00:00.000000000' AS TIMESTAMP)
* TIME literals don't work with TIMEADD in Snowflake
Addresses review comments from VaggelisD on PR #6617
2296a92 to
c8a9ce5
Compare
- Use expression.unit property accessor instead of expression.args.get('unit')
* Updated in _timediff_sql, date_delta_to_binary_interval_op, _date_diff_sql
* More concise and idiomatic
- Remove unnecessary docstring from _is_nanosecond_unit
* Function name is self-explanatory
- Keep _unwrap_cast helper function
* Necessary to avoid nested casts like CAST(CAST(x AS TIMESTAMP) AS TIMESTAMP_NS)
* exp.cast only avoids recasting to SAME type, not DIFFERENT types
* Example: CAST('2023-01-01' AS TIMESTAMP) → without unwrap → CAST(CAST(...) AS TIMESTAMP_NS)
* With unwrap: extracts '2023-01-01' → CAST('2023-01-01' AS TIMESTAMP_NS)
- cast parameter not needed for NANOSECOND handling
* NANOSECOND operations require EPOCH_NS/make_timestamp_ns
* These functions require TIMESTAMP_NS type
* Must always cast regardless of cast parameter
* cast parameter only applies to base implementation's interval operations
Addresses review comments from georgesittas on PR #6617
…g tests for simplicity
VaggelisD
left a comment
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.
Nice work once again @fivetran-amrutabhimsenayachit, leaving a few nits to consider, feel free to merge afterwards:
| def date_delta_to_binary_interval_op( | ||
| cast: bool = True, | ||
| ) -> t.Callable[[DuckDB.Generator, DATETIME_DELTA], str]: | ||
| """DuckDB override to handle NANOSECOND operations; delegates other units to base.""" | ||
| base_impl = base_date_delta_to_binary_interval_op(cast=cast) |
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.
Lets prefix the function with _ since its local to DuckDB, by doing so we can also leave the import name as is e.g:
```suggestion
def _date_delta_to_binary_interval_op(
cast: bool = True,
) -> t.Callable[[DuckDB.Generator, DATETIME_DELTA], str]:
"""DuckDB override to handle NANOSECOND operations; delegates other units to base."""
base_impl = date_delta_to_binary_interval_op(cast=cast)| if _is_nanosecond_unit(unit): | ||
| return _handle_nanosecond_diff(self, expression.this, expression.expression) |
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.
[Nit] We can inline the implementations of both _handle_nanosecond_diff and _handle_nanosecond_add directly under their branches now that they're both small enough after the recent changes, leaving it up to you
| """DuckDB override to handle NANOSECOND operations; delegates other units to base.""" | ||
| base_impl = base_date_delta_to_binary_interval_op(cast=cast) | ||
|
|
||
| def duckdb_date_delta_sql(self: DuckDB.Generator, expression: DATETIME_DELTA) -> str: |
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.
[Nit] Lets also prefix this with _
|
Forgot you'll be away on holidays, I'll merge this and apply the comments |
Issue: DuckDB does not support
NANOSECONDas a unit inDATE_DIFF()orINTERVALoperations.Solution: To enable Snowflake → DuckDB transpilation with nanosecond precision, following workaround implemented:
EPOCH_NS(): ConvertsTIMESTAMP_NSto nanoseconds since Unix epoch (returns INT64)make_timestamp_ns(): Converts nanoseconds (INT64) toTIMESTAMP_NSSnowflake:
Duckdb (after transpilation):
Function Mapping Summary: