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

planner: fix predicate simplifier #12650

Merged
merged 6 commits into from
Mar 21, 2023
Merged

Conversation

systay
Copy link
Collaborator

@systay systay commented Mar 17, 2023

Description

In the predicate rewriting that we added recently, a bug snuck in that can mangle predicates in a bad way.

This PR fixes that, and also introduces fuzz testing that should find any other problems

Related Issue(s)

Fixes #12645

Checklist

  • "Backport to:" labels have been added if this change should be back-ported
  • Tests were added or are not required
  • Did the new or modified tests pass consistently locally and on the CI
  • Documentation was added or is not required

Copy link
Member

@frouioui frouioui left a comment

Choose a reason for hiding this comment

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

Left a few comments, mostly nit and something about a test that fails both in CI and locally. Otherwise looks to me 🙏🏻

Nicer panic message

Co-authored-by: Florent Poinsard <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
@systay systay merged commit 7af519e into vitessio:main Mar 21, 2023
@systay systay deleted the in-rewrite-fix branch March 21, 2023 14:08
frouioui added a commit to planetscale/vitess that referenced this pull request Mar 28, 2023
* planner: fix predicate simplifier

Signed-off-by: Andres Taylor <[email protected]>

* predicate rewriting: add debug logging

Signed-off-by: Andres Taylor <[email protected]>

* add debug logging to make the code easier to understand

Signed-off-by: Andres Taylor <[email protected]>

* address review feedback

Signed-off-by: Andres Taylor <[email protected]>

* Update go/vt/vtgate/planbuilder/predicate_rewrite_test.go

Nicer panic message

Co-authored-by: Florent Poinsard <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>

---------

Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Co-authored-by: Florent Poinsard <[email protected]>
Signed-off-by: Florent Poinsard <[email protected]>
frouioui added a commit that referenced this pull request Mar 28, 2023
* planner: fix predicate simplifier



* predicate rewriting: add debug logging



* add debug logging to make the code easier to understand



* address review feedback



* Update go/vt/vtgate/planbuilder/predicate_rewrite_test.go

Nicer panic message




---------

Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Florent Poinsard <[email protected]>
Co-authored-by: Andres Taylor <[email protected]>
@hmaurer hmaurer mentioned this pull request Mar 21, 2024
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.

Bug Report: Predicate rewriting of non-trivial or out-of-order OR conditions goes wild
3 participants