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

feat(optimizer): support logical filter expression simplify rule #15275

Merged
merged 47 commits into from
Mar 19, 2024

Conversation

xzhseh
Copy link
Contributor

@xzhseh xzhseh commented Feb 26, 2024

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

What's changed and what's your intention?

This PR resolves the LogicalFilter simplification espeically under LogicalShare.

e.g., the rule will apply in LogicalFilter -> LogicalShare -> LogicalFilter.

close #14133.

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.

@xzhseh xzhseh self-assigned this Feb 26, 2024
@xzhseh
Copy link
Contributor Author

xzhseh commented Feb 27, 2024

P.S. It's strange that only in StreamFilter (presumably in LogicalShare) will these kind of predicate exist, so the current implementation is essentially checking the input in LogicalShare, I have no idea why this is the case at present.

@st1page
Copy link
Contributor

st1page commented Feb 27, 2024

We have had some expressions simplify logic here and it could be called every time a new Condition is constructed.

fn simplify(self) -> Self {

I am not sure which one is better to organize the logic, implement them as a optimization rule or implement in Conditon.. but maybe we should maintain them in the same place 🤔

@xzhseh
Copy link
Contributor Author

xzhseh commented Feb 27, 2024

We have had some expressions simplify logic here and it could be called every time a new Condition is constructed.

fn simplify(self) -> Self {

I am not sure which one is better to organize the logic, implement them as a optimization rule or implement in Conditon.. but maybe we should maintain them in the same place 🤔

Thanks for pointing this out!
I haven't noticed this before and I'll take a look tomorrow. 😄

@xzhseh xzhseh force-pushed the xzhseh/expression-simplify-rule branch from fbc1f9a to beb65c4 Compare February 27, 2024 19:22
@xzhseh
Copy link
Contributor Author

xzhseh commented Mar 13, 2024

Now we only consider e that contains at most one column as the optimizable pattern, in Not(e) [op] e, if the column exists, it will be converted to IsNotNull(col) under specific context.
Two special patterns will be checked and try elimating first, e.g., true or (...) => true, false and (...) => false.
cc @chenzl25, @st1page.

@xzhseh xzhseh changed the title feat(optimizer): support basic expression simplify rule feat(optimizer): support expression simplify rule in StreamFilter Mar 13, 2024
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.

Rest LGTM

@xzhseh xzhseh changed the title feat(optimizer): support expression simplify rule in StreamFilter feat(optimizer): support logical filter expression simplify rule Mar 15, 2024
@xzhseh xzhseh requested a review from chenzl25 March 15, 2024 21:53
true
}

fn rewrite_exprs(&self, r: &mut dyn ExprRewriter) -> PlanRef {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please correct me this is not intended for Share, i.e., rewrite input: RefCell<PlanRef>.
cc @chenzl25 @st1page.

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.

Rest LGTM

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

@xzhseh xzhseh enabled auto-merge March 18, 2024 13:23
@xzhseh xzhseh added this pull request to the merge queue Mar 19, 2024
Merged via the queue into main with commit e4f5eb4 Mar 19, 2024
26 of 27 checks passed
@xzhseh xzhseh deleted the xzhseh/expression-simplify-rule branch March 19, 2024 06:19
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.

feat: Support expression simplify rule
3 participants