datetime_coercion rule
#1194
datetime_coercion rule
#1194
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #1194 +/- ##
=======================================
Coverage 85.55% 85.55%
=======================================
Files 77 77
Lines 4257 4258 +1
Branches 758 758
=======================================
+ Hits 3642 3643 +1
Misses 446 446
Partials 169 169 ☔ View full report in Codecov by Sentry. |
sarahyurick
requested review from
ayushdg,
charlesbluca,
galipremsagar and
jdye64
as code owners
|
This PR is intended to help query 72, which was erroring with an |
dask_planner/src/sql/preoptimizer.rs
Outdated
| | Operator::Minus | ||
| | Operator::Multiply | ||
| | Operator::Divide | ||
| | Operator::Modulo = &b.op |
There was a problem hiding this comment.
It doesn't really make sense to multiply/divide/modulo a datetime, so should maybe remove these.
| """ | ||
| SELECT * FROM d_table d1, d_table d2 | ||
| WHERE d2.x < d1.x + (1 + 2) | ||
| AND d2.d_date > d1.d_date + (2 + 3) |
There was a problem hiding this comment.
The parentheses don't matter here. This is just to demonstrate that this PR can handle something like d1.d_date + 1, d1.d_date + 2 + 3, d1.d_date + 4 + 5 + 6, etc. for as many mathematical operands as the user provides.
There was a problem hiding this comment.
I really like this approach of introducing a "pre-processing" phase instead of trying to recover from errors from Datafusion and potentially attempt to re-run them. This is much more clean for our purposes. Thanks for adding this @sarahyurick
|
GPU CI failures seem to be unrelated, and should be resolved with #1220 |
|
Hi @charlesbluca I wanted to ask about this getting merged in? |
|
Sorry this has sat on the backburner for a while - might make sense to merge in the ongoing work in #1249, as I imagine we'll want to prioritize that for release and it includes a bump from ADP 28 -> 32 that could affect things here |
|
Thanks @charlesbluca ! Tested locally with the latest main and can confirm that this PR still adds functionality that did not pass before. Should be ready to go. |
|
@charlesbluca looks like #1265 is related to the failures here? They're all giving a: |
|
rerun tests |
|
Thanks for the work here @sarahyurick! |
Closes #1189