-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add support for DataType::Timestamp casts in unwrap_cast_in_comparison optimizer pass
#4148
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
| ] { | ||
| let utc = Some("+0:00".to_string()); | ||
| // No timezone, utc timezone | ||
| let (lit_tz_none, lit_tz_utc) = match time_unit { |
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.
A note on the timezone handling is that expect_cast added in #4147 and used in this PR explicitly verifies that calling the arrow cast kernel produces the same result as the code in this PR.
| // pushed down / pruned | ||
| let expected = | ||
| "Projection: test.col_int32\n Filter: CAST(test.col_ts_nano_none AS Timestamp(Nanosecond, Some(\"+00:00\"))) < TimestampNanosecond(1666612093000000000, Some(\"+00:00\"))\ | ||
| "Projection: test.col_int32\ |
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.
It is finally fixed! Note there is no cast on the test.col_ts_nano_none column
|
If your pr is ready, please ping me. |
0f64cfc to
8b67ded
Compare
|
This PR is now ready for review @liukun4515 As an aside, I found working with this code to be quite straightforward -- thank you to @liukun4515 for the nice structure and tests |
|
@liukun4515 if you have a moment to review this PR I would appreciate it (as I have another PR with support for unsigned type support backed up behind it). 🙏 |
liukun4515
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.
LGTM
very happy to hear that |
|
Benchmark runs are scheduled for baseline = 129654c and contender = bcd8af8. bcd8af8 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Draft as it builds on tests in #4147Which issue does this PR close?
Closes #3938
Rationale for this change
I want predicates that compare timestamp columns and
now()to be fully optimized / pruned. See #3938 and https://github.com/influxdata/influxdb_iox/issues/5875 for more detailsWhat changes are included in this PR?
DataType::Timestampinunwrap_cast_in_comparisonoptimizer passAre these changes tested?
Yes (I wrote a whole new set of tests just for this :) )-- #4147
Are there any user-facing changes?
Hopefully faster queries