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

Simplify SimplifyRedundantCase rule #23846

Conversation

Praveen2112
Copy link
Member

Description

Instead of handling the transformed expression as Optional<Expression> and replace them with FALSE if it doesn't exist - we could simplify them.

Additional context and related issues

#23796

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

## Section
* Fix some things. ({issue}`issuenumber`)

@Praveen2112 Praveen2112 force-pushed the praveen/simplify_boolean_cleanup branch from 8f0a4e7 to 5c0e465 Compare October 21, 2024 09:36
Copy link
Member

@martint martint left a comment

Choose a reason for hiding this comment

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

Returning Optional<Expression> was intentional handle the base case. Otherwise you end up with a spurious OR expression for any CASE that has a default clause that returns false, as you've been able to observe in the tests you had to modify.

E.g., for

CASE a 
    WHEN x THEN true
    ELSE false
END

Turns into:

(x ≡ true) OR false

instead of:

x ≡ true

@Praveen2112
Copy link
Member Author

Yes - But we do have a different rule EvaluateLogical which would remove this redundant
(x ≡ true) OR false => (x ≡ true) right ? Won't that rule would help us here ?

For my understanding, when we write a new rule should it focus on one optimization which it tries to achieve or try to come up with a best plan ?

@martint
Copy link
Member

martint commented Oct 21, 2024

That's correct. But in this case, the OR with false would be spurious, and only an artifact of how the code is structured. There's no reason for the rule to produce that structure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants