[SPARK-49756][SQL][FOLLOWUP] Use correct pgsql datetime fields when pushing down EXTRACT #50101
Conversation
|
and also cc @yaooqinn |
| case "DAY_OF_WEEK" => s"EXTRACT(DOW FROM $source)" | ||
| case _ => super.visitExtract(field, source) | ||
| // SECOND, MINUTE, HOUR, QUARTER, YEAR, DAY are identical on postgres and spark | ||
| // MONTH is different, postgres returns 0-11, spark returns 1-12. |
There was a problem hiding this comment.
I checked the doc you provided, but it says 1-based for datetimes, 0-based for intervals on the pg side.
There was a problem hiding this comment.
you are right, sorry I misread the doc.
Now looking into it again, Spark DS v2 pushdown does not support extracting fields from interval type yet (expressions like ExtractANSIIntervalMonths are not included in V2ExpressionBuilder). But even if we support it later, its semantic is the same as pgsql.
There was a problem hiding this comment.
Yeah, I believe they shall be identical as they all came from the work within the Postgres feature parity umbrella ticket.
I appreciate the confirmation.
| // SECOND, MINUTE, HOUR, QUARTER, YEAR, DAY are identical on postgres and spark | ||
| // MONTH is different, postgres returns 0-11, spark returns 1-12. | ||
| // Postgres also returns 1-12 but just for interval columns, so without source | ||
| // data type, we cannot know how to push down it |
| withClue("dayofweek") { | ||
| val dow = sql(s"SELECT dayofweek(date1) FROM $tbl WHERE name = 'alex'") | ||
| .collect().head.getInt(0) | ||
| val df = sql(s"SELECT name FROM $tbl WHERE dayofweek(date1) = $dow") |
There was a problem hiding this comment.
A better idea is probably having a test infra that automatically checks the result between JDBC pushdown on and off. I'll leave it to followups.
| // but in V2ExpressionBuilder they converted DAY_OF_WEEK to DAY_OF_WEEK_ISO, | ||
| // so we need to push down ISODOW | ||
| // (ISO and standard day of weeks differs in starting day, | ||
| // Sunday is 0 on standard DOW extraction, while in ISO it's 7) |
There was a problem hiding this comment.
Got it. Thank you for the explanation.
beliefer
left a comment
beliefer
left a comment
There was a problem hiding this comment.
LGTM. I will check other dialects.
|
the streaming test failure is unrelated, thanks for the review, merging to master/4.0! |
…ushing down EXTRACT ### What changes were proposed in this pull request? This is a followup of #48210 to fix correctness issues caused by pgsql filter pushdown. These datetime fields were picked wrongly before, see https://neon.tech/postgresql/postgresql-date-functions/postgresql-extract ### Why are the changes needed? bug fix ### Does this PR introduce _any_ user-facing change? Yes, query result is corrected, but this bug is not released yet. ### How was this patch tested? updated test ### Was this patch authored or co-authored using generative AI tooling? no Closes #50101 from cloud-fan/pgsql. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 208a7ee) Signed-off-by: Wenchen Fan <[email protected]>
…ushing down EXTRACT ### What changes were proposed in this pull request? This is a followup of apache#48210 to fix correctness issues caused by pgsql filter pushdown. These datetime fields were picked wrongly before, see https://neon.tech/postgresql/postgresql-date-functions/postgresql-extract ### Why are the changes needed? bug fix ### Does this PR introduce _any_ user-facing change? Yes, query result is corrected, but this bug is not released yet. ### How was this patch tested? updated test ### Was this patch authored or co-authored using generative AI tooling? no Closes apache#50101 from cloud-fan/pgsql. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit e65434e) Signed-off-by: Wenchen Fan <[email protected]>
3 participants
What changes were proposed in this pull request?
This is a followup of #48210 to fix correctness issues caused by pgsql filter pushdown. These datetime fields were picked wrongly before, see https://neon.tech/postgresql/postgresql-date-functions/postgresql-extract
Why are the changes needed?
bug fix
Does this PR introduce any user-facing change?
Yes, query result is corrected, but this bug is not released yet.
How was this patch tested?
updated test
Was this patch authored or co-authored using generative AI tooling?
no