Skip to content
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

refactor(optimizer): unify watermark derivation and monotonicity derivation #17554

Merged
merged 8 commits into from
Jul 10, 2024

Conversation

stdrc
Copy link
Member

@stdrc stdrc commented Jul 3, 2024

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Previously we have

  1. Watermark derivation to derive output watermark columns for each plan node;
  2. Nondecreasing variant of WatermarkDerivation to support watermark on proctime column;
  3. Another attempt feat(optimizer): add monotonicity property #13984 to implement another property of columns of plan node called "monotonicity".

When I worked on the continuation of the "monotonicity" property, I realized that we can unify the watermark and monotonicity derivation process, by

  1. Analyze the WatermarkDerivation in the same recursive manner as we do for WatermarkDerivation;
  2. Derive watermark based on the WatermarkDerivation.

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • I have added test labels as necessary. See details.
  • I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
  • My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
  • All checks passed in ./risedev check (or alias, ./risedev c)
  • My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)
  • My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

  • My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

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.

Comment on lines 159 to 247
ExprType::AtTimeZone => match self.visit_binary_op(func_call.inputs()) {
(Constant, Constant) => Constant,
(any, Constant) => {
let time_zone = func_call.inputs()[1]
.as_literal()
.and_then(|literal| literal.get_data().as_ref())
.map(|tz| tz.as_utf8().as_ref());
// 1. For at_time_zone(timestamp, const timezone) -> timestamptz, when timestamp has some monotonicity,
// the result should have the same monotonicity.
// 2. For at_time_zone(timestamptz, const timezone) -> timestamp, when timestamptz has some monotonicity,
// the result only have the same monotonicity when the timezone is without DST (Daylight Saving Time).
if (func_call.inputs()[0].return_type() == DataType::Timestamp
&& func_call.return_type() == DataType::Timestamptz)
|| time_zone_is_without_dst(time_zone)
{
any
} else {
Unknown
}
}
_ => Unknown,
},
ExprType::DateTrunc => match func_call.inputs().len() {
2 => match self.visit_binary_op(func_call.inputs()) {
(Constant, any) => any,
_ => Unknown,
},
3 => match self.visit_ternary_op(func_call.inputs()) {
(Constant, Constant, Constant) => Constant,
(Constant, any, Constant) => {
let time_zone = func_call.inputs()[2]
.as_literal()
.and_then(|literal| literal.get_data().as_ref())
.map(|tz| tz.as_utf8().as_ref());
if time_zone_is_without_dst(time_zone) {
any
} else {
Unknown
}
}
_ => Unknown,
},
_ => unreachable!(),
},
ExprType::AddWithTimeZone | ExprType::SubtractWithTimeZone => {
// Requires time zone and interval to be literal, at least for now.
let time_zone = match &func_call.inputs()[2] {
ExprImpl::Literal(lit) => {
lit.get_data().as_ref().map(|tz| tz.as_utf8().as_ref())
}
_ => return Unknown,
};
let interval = match &func_call.inputs()[1] {
ExprImpl::Literal(lit) => lit
.get_data()
.as_ref()
.map(|interval| interval.as_interval()),
_ => return Unknown,
};
let quantitative_only = interval.map_or(
true, // null interval is treated as `interval '1' second`
|v| v.months() == 0 && (v.days() == 0 || time_zone_is_without_dst(time_zone)),
);
match (self.visit_expr(&func_call.inputs()[0]), quantitative_only) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small fixes related to these datetime functions are included when adapting the old WatermarkAnalyzer code to the new MonotonicityAnalyzer. PTAL cc @xiangjinwu

@stdrc stdrc requested review from chenzl25, xiangjinwu and st1page July 3, 2024 13:39
@stdrc stdrc marked this pull request as draft July 4, 2024 06:14
Copy link
Contributor

@st1page st1page left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

general LGTM

src/frontend/src/optimizer/property/monotonicity.rs Outdated Show resolved Hide resolved
src/frontend/src/optimizer/property/monotonicity.rs Outdated Show resolved Hide resolved
Copy link
Member Author

stdrc commented Jul 8, 2024

@stdrc stdrc force-pushed the rc/column-monotonicity branch from 9f264c4 to 6b9feea Compare July 9, 2024 08:32
@stdrc stdrc marked this pull request as ready for review July 9, 2024 08:32
Copy link
Contributor

@chenzl25 chenzl25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@stdrc stdrc added this pull request to the merge queue Jul 10, 2024
Merged via the queue into main with commit 96498a4 Jul 10, 2024
30 of 31 checks passed
@stdrc stdrc deleted the rc/column-monotonicity branch July 10, 2024 05:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants