-
Notifications
You must be signed in to change notification settings - Fork 590
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
fix(optimizer): allow fold_const on timezone-dependent exprs in logical plan #12633
Conversation
@@ -369,8 +369,7 @@ fn expr_to_kafka_timestamp_range( | |||
|
|||
match &expr { | |||
ExprImpl::FunctionCall(function_call) => { | |||
if let Some((timestampz_literal, reverse)) = extract_timestampz_literal(&expr).unwrap() | |||
{ | |||
if let Ok(Some((timestampz_literal, reverse))) = extract_timestampz_literal(&expr) { |
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.
Another bug fixed: _rw_kafka_timestamp >= 'aa'::timestamptz
shall not unwrap()
cdde515
to
4d728ff
Compare
Codecov Report
@@ Coverage Diff @@
## main #12633 +/- ##
=======================================
Coverage 69.28% 69.29%
=======================================
Files 1470 1470
Lines 241296 241324 +28
=======================================
+ Hits 167187 167214 +27
- Misses 74109 74110 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
ExprType::AddWithTimeZone | ExprType::SubtractWithTimeZone => { | ||
let args = f.inputs(); | ||
args[0].is_now_offset() && args[1].is_const() | ||
} |
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.
This implementation is too weak. The following optimizer rule actually requires is_now_offset
to be stricter than WatermarkAnalyzer
. This was true before the PR but is broken.
risingwave/src/frontend/src/optimizer/rule/stream/filter_with_now_to_join_rule.rs
Lines 44 to 52 in e53c9f7
if let Some((input_expr, cmp, now_expr)) = expr.as_now_comparison_cond() { | |
let now_expr = rewriter.rewrite_expr(now_expr); | |
// as a sanity check, ensure that this expression will derive a watermark | |
// on the output of the now executor | |
debug_assert_eq!( | |
try_derive_watermark(&now_expr), | |
WatermarkDerivation::Watermark(lhs_len) | |
); |
For example, interval '1' month
is allowed by is_now_offset
but rejected by WatermarkAnalyzer
. So the following query (inspired by previous temporal_filter.slt
failure in ci) would fail the debug_assert
above:
create table t1 (v1 timestamp);
create materialized view mv1 as select v1 from t1 where v1 between now() and now() + interval '1 month';
It is not hard to update implementation here to keep that invariant. But I prefer to fix the problem without touching too many different components.
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
WIP: temporal filter tests are failing by this change.
Fix #12523.
try_fold_const
panic due to missing timezone inLogicalSource::predicate_pushdown
.unwrap
whentry_fold_const
evaluates to an error (eg'abc'::timestamptz
) inLogicalSource::predicate_pushdown
Fix of the first issue is not perfect:
timezone: &str
as a required argument oftry_fold_const
. This guarantees timezone is always available when we are about to evaluate, but some direct callers do not have this info right now, and we lose the ability to notice user of the discouraged implicit usage.Binder::bind_expr
orFunctionCall::new
. These are not practical for similar reasons: cannot warn user or direct caller lacking info.try_fold_const
before rewrite.Checklist
./risedev check
(or alias,./risedev c
)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.