Skip to content

fix(parser): honor HIGH_NOT_PRECEDENCE sql mode - #28670

Merged
XuPeng-SH merged 8 commits into
matrixorigin:mainfrom
iamlinjunhong:m-28601
Sep 11, 2026
Merged

XuPeng-SH merged 8 commits into
matrixorigin:mainfrom
iamlinjunhong:m-28601

Conversation

@iamlinjunhong

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #28601

What this PR does / why we need it:

fix(parser): honor HIGH_NOT_PRECEDENCE sql mode

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@matrix-meow matrix-meow added the size/XXL Denotes a PR that changes 2000+ lines label Sep 11, 2026

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed current head 924d454 against base ee0c2bf. One concrete grammar-compatibility blocker is detailed inline: the high-precedence NOT token is not admitted in the same expression positions as !.

The intended BETWEEN/IN parse-tree changes, explicit-parenthesis controls and duplicated non-unary NOT grammar contexts are coherent. Mode flags are per lexer/session; cache eviction compares parser-relevant flags, and prepared reparsing retains its saved scheduling SQL mode. Existing expression/plan representations are reused without a new distributed opcode. Added BVT covers mode toggles, prepared statements, views and column predicates; no additional shared-state, resource-lifecycle or asymptotic execution-cost blocker identified.

Evidence: full source diff, grammar/lexer and AST-producing actions, cache/prepared consumers and test oracles; compared the relevant MySQL 8.4 grammar and documented !/NOT equivalence under HIGH_NOT_PRECEDENCE. The finding is source-derived, not a claimed live SQL or fresh UT run. CI was not awaited; head/base rechecked before submission.

Comment thread pkg/sql/parsers/dialect/mysql/mysql_sql.y Outdated

@aptend aptend left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Deep re-review completed on exact head 3995098be04ec0e0e6da2f01edc035855246df4a.

I read the prior REQUEST_CHANGES review and its unresolved/outdated inline thread, checked the delta from the previously reviewed head 924d454f617b0389ed0393616558bf332875af50, and re-audited the complete PR diff. The previous blocker is closed: HIGH_NOT_PRECEDENCE is now handled at simple_expr, so unary/LIKE/other predicate contexts follow the intended precedence, while grammar positions where NOT is a keyword retain explicit HIGH_NOT alternatives.

I found no remaining blocking correctness, compatibility, lifecycle, or test-quality issue.

Local validation:

  • full MySQL parser package tests
  • focused HIGH_NOT/parser-mode tests under -race
  • focused frontend plan-cache invalidation/helper tests
  • deterministic parser regeneration (make -B mysql_sql.go, no diff)
  • git diff --check

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed exact head 3995098 against main db4edb4. The prior P2 is fixed; no concrete blocker remains.

HIGH_NOT now derives simple_expr, beside the ! unary production. The previously rejected - NOT 0, ! NOT 0, and LIKE NOT 0 paths therefore have the intended AST; focused tests assert those shapes. The default NOT production remains separate, and explicit alternatives preserve NOT as a keyword in NOT IN/LIKE/BETWEEN/REGEXP, IS NOT, constraints, IF NOT EXISTS and MERGE WHEN NOT MATCHED. I rechecked precedence, parentheses, mode-token membership, per-session lexer flags and parser-sensitive plan-cache invalidation. Prepared/view creation-mode behavior remains covered by public SQL controls; no new expression opcode or wire/catalog representation is introduced.

Validation: reviewed the full non-generated change and current/previous review history, grammar-producing actions, session/cache consumers and expected SQL results. Reused the external exact-head report of full parser tests, focused race tests, frontend cache tests and deterministic generated-parser regeneration (no diff), rather than manually treating regenerated parser tables as independent semantics. I did not independently rerun native/frontend/BVT tests or a live server, and did not wait for CI. Head/base were rechecked before submission.

@mergify mergify Bot added the queued label Sep 11, 2026
@mergify

mergify Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Merge Queue Status

  • Entered queue2026-09-11 10:21 UTC · Rule: main · triggered by rule Automatic queue on approval for main
  • 🟠 Checks running · in-place
  • 🚫 Left the queue2026-09-11 10:22 UTC · at 23bc3a74b18b7898dc2c762ca86e24d001785843

This pull request spent 54 seconds in the queue, with no time running CI.

Waiting for any of
  • check-neutral = CI Required
  • check-skipped = CI Required
  • check-success = CI Required
All conditions

Reason

Pull request #28670 has been dequeued

Pull request from fork cannot be queued. This pull request comes from a fork, and Mergify needs the author's permission to update its branch.

The author needs to enable "Allow edits from maintainers" on this pull request.

Hint

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.
If you do update this pull request, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio queue comment.

Tick the box to put this pull request back in the merge queue (same as @mergifyio queue).

  • Requeue this pull request

@XuPeng-SH
XuPeng-SH merged commit 23b20ea into matrixorigin:main Sep 11, 2026
19 of 21 checks passed
@mergify mergify Bot added dequeued and removed queued labels Sep 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dequeued size/XXL Denotes a PR that changes 2000+ lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants