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

[CALCITE-4910] Enhance simplify to reduce ((A OR D) and (A OR C) AND A AND B) to (A AND B) (Ziwei Liu) #2673

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

xiayutianwei1994
Copy link

@xiayutianwei1994 xiayutianwei1994 commented Jan 4, 2022

Enhance simplify to reduce ((A OR D) AND (A OR C) AND A AND B) to (A AND B) (Ziwei Liu)

@xiayutianwei1994 xiayutianwei1994 force-pushed the master branch 2 times, most recently from 9930411 to 46337f7 Compare January 5, 2022 05:31
@xiayutianwei1994 xiayutianwei1994 changed the title [CALCITE-4910] Enhance simplify to reduce ((a or d) and (a or c) and a and b) to (a and b) (Ziwei Liu) [CALCITE-4910] Enhance simplify to reduce ((A OR D) and (A OR C) AND A AND B) to (A AND B) (Ziwei Liu) Jan 5, 2022
@NobiGo
Copy link
Contributor

NobiGo commented Jan 5, 2022

@xiayutianwei1994 Until we have a final state, please don't force push the commit before that.

@xiayutianwei1994
Copy link
Author

@xiayutianwei1994 Until we have a final state, please don't force push the commit before that.

Ok, Thanks

// package-protected only to support a deprecated method; treat as private
RexNode simplifyAnd2(List<RexNode> terms, List<RexNode> notTerms) {
Set<RexNode> termsSet = new HashSet<>(terms);
Copy link
Member

Choose a reason for hiding this comment

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

I think we already have similar logic here - based on the predicates - and most likely we only miss the handling of an existing predicate somewhere around in the simplifyIsNull/simpliftIsNotNull methods

I think this approach is a little bit too much by the way as at first glance I would be afraid that it would make "unsafe" simplifications as well

let me know if you think otherwise - I'll try to take a closer look tomorrow

Copy link
Author

@xiayutianwei1994 xiayutianwei1994 Jan 6, 2022

Choose a reason for hiding this comment

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

Thanks for your comment.
If use unknown as unknown, some predicate will not be simplified to IS NOT NULL or IS NULL like the third test case I added.
There also has another question: If (A OR B) AND A, predicate A is nondeterministic, whether it can be simplified to A. I think is can not be simplified.
Now we only pull up predicate which is deterministic.
If the upper question's answer is can not simplify, change some code in simplifyIsNull/simpliftIsNotNull may handle this logic when unknown as false.

Copy link
Author

Choose a reason for hiding this comment

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

I have updated the code, please review it again.
Thanks very much.

@xiayutianwei1994
Copy link
Author

I have updated the code, please review it again.
Thanks very much.

@NobiGo NobiGo added the discussion-in-jira There's open discussion in JIRA to be resolved before proceeding with the PR label Jan 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion-in-jira There's open discussion in JIRA to be resolved before proceeding with the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants