fix(postgres): transpile DAY/MONTH/YEAR to EXTRACT [CLAUDE] - #7798
fix(postgres): transpile DAY/MONTH/YEAR to EXTRACT [CLAUDE]#7798vjpovlitz wants to merge 2 commits into
Conversation
|
@vjpovlitz thanks for the PR, in order to transpile For example the input (tsql) can be a string value, which we can't transpile in postgres as is (we need on the parsing side of tsql to wrap the function argument with a Let's first make an analysis on all of these cases/inputs ^ in order to know the exact semantics of these functions, then we can come up with a robust implementation that covers theses cases (or a part of them). |
|
Thank you very much for the kind and detailed feedback @geooo109. This is one of my first open source contributions, so I really appreciate the guidance. Please keep it coming, it genuinely helps me grow as a programmer. I made assumptions, and thank you for pointing out that the naive form breaks as soon as the argument isn't already a date. I dug into the semantics and here's what I found. T-SQL
Proposed approach: wrap the argument on the T-SQL parse side in
The existing One scope question before I push: the integer-as-days-since-1900 case ( Would you prefer this PR to: (a) cover date, datetime, and string inputs and treat the integer case as unsupported (smaller and or (b) also emit the I'd like to do (b) if you're open to it, since handling all three input types keeps the transpilation faithful to T-SQL and feels more useful for the project, and I'll make sure it's well tested. If you'd rather keep this PR small, I'm glad to land (a) first and do (b) as a follow-up. |
|
@vjpovlitz great work!! thanks for the analysis. Let's focus on (a) for this PR, and we can do a following PR for the integer values. Keep in mind that the input can be a column, so the value of the argument isn't known (non-literal) on parse time. For example: So, we should use the sqlglot/sqlglot/generators/duckdb.py Line 3007 in 7521a08 You can assume that we always run in order the parsing-type annotation-generation, check for example: sqlglot/tests/dialects/test_duckdb.py Line 1511 in 7521a08 annotate_types and it's part of sqlglot's optimizer).
Before doing this ^, can you verify that it works for the tsql formats ? For example the docs state the following (for the This ^ will break the transpilation. |
|
@geooo109 Thanks, this was really helpful guidance. I implemented (a) and verified it against a local Postgres 15 on a MacBook and a Linux VM I have. Changes:
One note on the pipeline: On the formats, I ran each through real Postgres:
Committing and pushing to the PR branch and going through the GitHub actions checks right now, pausing for your review. Ty! |
|
|
||
| return self.sql(exp.Extract(this=exp.var(part), expression=this)) | ||
|
|
||
| return func |
There was a problem hiding this comment.
No need to return the function here (inline it and make it 1), moreover we can pass the expression which is exp.Day/Month/Year and use the sql_name for the exp.var creation to minmize the code.
Moreover, in the tsql parser we should wrap the argument with TsOrDsDate, thus
if not this.is_type(exp.DType.DATE):
this = exp.cast(this, exp.DType.DATE)
This ^ is redundant here.
There was a problem hiding this comment.
Done. Inlined into a single _day_month_year_sql that takes the expression and builds the EXTRACT part from sql_name(). The T-SQL parser now wraps the argument in TsOrDsToDate, so the explicit cast here was removed. The helper still unwraps TsOrDsToDate to check for integer-typed arguments and emit the unsupported message. Applied in #7984.
| this = expression.this | ||
| if this.is_type(*exp.DataType.INTEGER_TYPES): | ||
| self.unsupported(f"Cannot transpile {part} of an integer value to Postgres") | ||
| if not this.is_type(exp.DType.DATE): |
There was a problem hiding this comment.
We don't want integer literals to fall through to this branch, right?
| if not this.is_type(exp.DType.DATE): | |
| elif not this.is_type(exp.DType.DATE): |
There was a problem hiding this comment.
Good catch, thanks. This ended up moot: with the cast moved to the T-SQL parser via TsOrDsToDate, the cast branch is gone from the generator entirely, so integers cannot fall through to it anymore. Applied in #7984.
| return func | ||
|
|
||
|
|
||
| def _extract_sql(part: str) -> t.Callable[[PostgresGenerator, exp.Func], str]: |
There was a problem hiding this comment.
Let's rename this to something reflecting that this works on date parts specifically. Right now, it seems like a generic EXTRACT generator, so the cast to DATE in L86 can raise some eyebrows, e.g., I thought "is it safe to cast to DATE? what about timestamp inputs?"
There was a problem hiding this comment.
Renamed to _day_month_year_sql. The CAST AS DATE also moved out of this function (it now comes from TsOrDsToDate on the parse side), so the function no longer reads like a generic EXTRACT generator. Applied in #7984.
|
Closing this due to inactivity. Feel free to reopen with the suggestions applied. |
|
Sorry for the gap here, I started a new job and moved, and I'm closing the loop on loose ends now. As things sit: the last round of review asked for three changes. Inline the factory helper and build the extract part from All three are now applied on the branch, and I replied on each review thread with the specifics. GitHub does not let me reopen this PR from my side since it was closed by a maintainer, so I opened #7984 from the same branch with the suggestions applied. Full unit suite, ruff, and mypy are clean, and the branch merges cleanly with current main. |
Summary
PostgreSQL has no scalar
DAY/MONTH/YEARfunctions — date parts are read viaEXTRACT(field FROM source). When transpiling from a dialect that does have them (e.g.T-SQL), SQLGlot emitted the functions unchanged, producing invalid Postgres SQL.
This adds Postgres generator rules so
exp.Day/exp.Month/exp.Yearrender asEXTRACT(<part> FROM ...).Reproduction
MONTHandYEARbehave the same.EXTRACTis the standard Postgres construct for theseparts: https://www.postgresql.org/docs/current/functions-datetime.html#FUNCTIONS-DATETIME-EXTRACT
Approach
exp.Day/exp.Month/exp.Yearnodes, so only the Postgres output was wrong. Parsing and otherdialects are untouched (
tsql -> tsqlstill yieldsDAY(d)).exp.Extractnode rather than hand-built SQL, and the threerules share a small
_extract_sql(part)factory in the style of the neighboring_date_add_sql(kind)helper.they likewise lack scalar
DAY/MONTH/YEAR.Test
tests/dialects/test_postgres.py::TestPostgres::test_extract_date_partsasserts theT-SQL -> Postgres transpilation and that the
EXTRACTform round-trips in Postgres.Fixes #6220