-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
fix(sql): breaking change💥 - fix expression parser operator precedence #4443
fix(sql): breaking change💥 - fix expression parser operator precedence #4443
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting the hold on it before I make some changes.
To be merged after #4441 |
…between old & new precedence tables
… mismatch in logs
ecdd683
to
449c1e7
Compare
core/src/test/java/io/questdb/test/griffin/ExpressionParserFuzzTest.java
Show resolved
Hide resolved
This part of the PR description seems no longer relevant and should be updated. |
core/src/test/java/io/questdb/test/griffin/ExpressionParserFuzzTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for another solid contribution!
There is a conflict due to a recent test change.
Looking to resolve conflicts now, bear with. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for all your effort sorting out this and its tandem PR, we really appreciate the help and attention to detail!
@sivukhin our build doesn't like your javadocs. :) Is it ok if I send a PR to fix that against your fork? |
@puzpuzpuz, yes, sure. You can also push directly to my fork (sent you an invite; but feel free to decline and create PR into the fork) |
…or-precedence-fix-only
Corresponding docs PR: https://github.com/questdb/questdb.io/pull/2286 |
Context
This is second part of the PR #4386
This PR fixes expression parser operator precedence rules. Now,
QuestDB
will use precedence order which is close to the PostgreSQL (https://www.postgresql.org/docs/7.2/sql-precedence.html; the precedence rules validated extensively with ExpressionParserFuzzTest.java):In order to provide migration path for users
QuestDB
introduces temporary boolean flag in the config:cairo.sql.legacy.operator.precedence
orQDB_CAIRO_SQL_LEGACY_OPERATOR_PRECEDENCE
env var which enables legacy operator precedence in a switched-on (true
) state + withcairo.sql.legacy.operator.precedence = true
QuestDB will log mismatch in parsing behaviour between old & new precedence table: