Skip to content

fix(mysql): resolve comma-separated relations in dot-scoped completion - #2088

Merged
openai0229 merged 4 commits into
OtterMind:mainfrom
mvanhorn:fix/1949-chat2db-mysql-second-alias-completion
Jul 27, 2026
Merged

fix(mysql): resolve comma-separated relations in dot-scoped completion#2088
openai0229 merged 4 commits into
OtterMind:mainfrom
mvanhorn:fix/1949-chat2db-mysql-second-alias-completion

Conversation

@mvanhorn

Copy link
Copy Markdown
Contributor

Related issue

Closes #1949

Summary

Issue #1949 asks for an explicit regression contract around MySQL dot-scoped
completion in multi-relation queries: after typing the second relation's alias,
b. should offer only that relation's columns. Writing the contract surfaced a
real gap in MysqlSqlCompletionRelationScopeResolver.

isRelationBoundary did not treat a , as a relation separator, so a
comma-separated relation list was only partially resolved: in
select * from users u join orders o on u.id = o.user_id, payments p where p.
the payments p relation was never registered, and p. fell back to the first
relation's columns. The new isFromRelationSeparator helper accepts a comma
only when it sits inside the FROM clause at the same paren depth: it finds the
nearest preceding FROM and refuses when a clause boundary (ON, WHERE,
GROUP, HAVING, ORDER, LIMIT, UNION) intervenes. That keeps commas in
select lists, function arguments, and WINDOW definitions from being mistaken
for relations.

The change stays inside the shared community MySQL plugin. No completion
pipeline rework, no UI rendering change, no Local or commercial-edition code.

Affected surfaces

  • Frontend / Web
  • Backend / API / Storage
  • Database plugin / Driver
  • JCEF / Desktop packaging
  • CI / Build / Release
  • Documentation only

Verification

  • Commands and results:
mvn -B -f chat2db-community-server/pom.xml \
  -pl chat2db-community-plugins/chat2db-community-mysql -am \
  -Dmaven.test.skip=false -DskipTests=false \
  -Dtest='MysqlSqlCompletionProviderTest,MysqlSqlCompletionRelationScopeResolverTest' \
  -Dsurefire.failIfNoSpecifiedTests=false test

Tests run: 164, Failures: 0, Errors: 0, Skipped: 0 -- MysqlSqlCompletionProviderTest
Tests run: 19,  Failures: 0, Errors: 0, Skipped: 0 -- MysqlSqlCompletionRelationScopeResolverTest
Tests run: 183, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS

Note: -DskipTests=false alone is not enough on this build; maven.test.skip
is set upstream and skips test compilation, so the tests silently do not run.
The command above sets both flags.

  • Manual verification: the new scenarios fail on main (the second relation's
    alias resolves to the first relation's columns) and pass with the resolver
    change.
  • UI evidence: N/A

Risk and compatibility

  • Public API or stored data: N/A - no public API or persisted shape changes.
  • Database or driver compatibility: scoped to the MySQL completion resolver; other database plugins are untouched.
  • Network, privacy, or security: N/A - no network or credential surface.
  • Community / Local / Pro boundary: entirely inside chat2db-community-mysql; no Local or commercial-edition code touched, per the issue's non-goals.
  • Backward compatibility: the comma is accepted as a relation separator only inside the FROM clause at the same paren depth, so select-list commas, function-argument commas, and WINDOW definition commas keep their existing behavior. A negative test pins the WINDOW case.

Reviewer map

  • Start here: MysqlSqlCompletionRelationScopeResolver.isFromRelationSeparator and the new MySqlLexer.COMMA case in isRelationBoundary.
  • Failure condition: if the depth or boundary-token check is wrong, a comma outside the FROM clause would be read as a relation separator and pollute the completion scope. doesNotTreatNamedWindowAsRelation is the guard for that class.
  • Rollback or disable path: revert the MySqlLexer.COMMA case in isRelationBoundary; the helper becomes unreachable and completion returns to the previous behavior.

Contributor declaration

  • I linked the Issue that defines this change.
  • I tested the affected behavior and reported the actual results above.
  • I did not include credentials, private data, or generated build output.
  • I disclosed substantial AI assistance below, or this PR contains no substantial AI-generated code.

AI assistance: AI was used for assistance.

@openai0229

Copy link
Copy Markdown
Contributor

Thank you, @mvanhorn, for contributing this fix and for adding focused regression coverage for #1949. The positive case is valid, and I re-ran both the targeted suite (183 tests) and the full MySQL reactor suite (610 MySQL tests); they pass.

Before this is merged, I found a few cases that need to be addressed in isFromRelationSeparator.

1. A same-depth FROM does not necessarily belong to the current query block

The helper currently searches backward for the most recent FROM at the comma's parenthesis depth and then checks a fixed list of boundary tokens. Parenthesis depth is not the same thing as a SELECT/statement scope: sibling subqueries and sibling CTE bodies can have the same depth. A new SELECT also does not reset the search. As a result, a projection comma can reuse a FROM from an earlier query block, and relationAfter(...) then registers the identifier after that comma as a physical table.

For example:

select (select id from users u),
       (select x, y, z| from orders o)

At the cursor, the scope incorrectly contains table=y. The same problem occurs in the second CTE body.

The fixed boundary list also does not describe all ways to leave a MySQL FROM clause. For example:

insert into dst (id, v)
select s.id, s.v from src s
on duplicate key update id = values(id), v = s.|

This incorrectly registers table=v. FOR UPDATE/SHARE OF u, o has the same class of problem: the second locking-clause identifier can be registered as a table and can collide with a real alias.

I converted the sibling-subquery, sibling-CTE, and ON DUPLICATE KEY UPDATE examples into temporary resolver tests. All three fail on the current head (05aff801). These false relations are observable: they participate in resolveOwner and can trigger metadata requests or mix columns from a non-existent table into completion results.

A possible implementation direction is:

  1. Classify commas within the owning statement/query-block token range, rather than searching all earlier tokens at the same depth.
  2. Prefer a single forward state scan for each query block: enter the table-reference state after its FROM, remain in it through JOIN ... ON/USING, and leave it at the actual MySQL FROM tail grammar or the query-block boundary.
  3. Recognize compound constructs such as ON DUPLICATE KEY UPDATE and FOR UPDATE/SHARE contextually. Treating every raw ON, FOR, or UPDATE token as a boundary would break join predicates, index hints, or standalone UPDATE statements.
  4. Add negative regression tests for sibling subqueries, the second CTE body, multiple statements, ON DUPLICATE KEY UPDATE, and FOR UPDATE/SHARE OF u, o.

This avoids continually extending a boundary allowlist, which is likely to miss another valid MySQL tail clause.

2. The new comma classification becomes quadratic on long statements

For every comma, isFromRelationSeparator calls depthAtOffset and performs two backward token scans. Since relation resolution visits every comma, a long select list becomes O(n^2). In a local warm micro-benchmark using an approximately 129 KB statement with 20,000 select-list commas, one resolver call increased from about 16 ms on the merge base to about 584 ms on this PR (over 36x).

The query-block state scan suggested above would also solve this: compute depth/scope transitions and the active table-reference state once while walking the tokens, then classify each comma in O(1). Precomputing the owning query-block and boundary state per token would be another workable approach.

Small test strengthening

The Issue specifically depends on the member prefix after b. being empty. assertOnlyOrdersAliasBColumns currently checks the returned columns but not the captured metadata request, so an incorrect non-empty prefix can still pass with the current fixture. Please also assert that the request is for COLUMN, its scope resolves to orders/b, and its prefix is exactly "".

Thanks again for the contribution. The core behavior is useful; the main change needed is to make relation-separator recognition query-block-aware before merging.

Addresses review feedback on OtterMind#2088:

- Replace the backward same-depth FROM search with a single forward state
  scan per query block, so sibling subqueries and sibling CTE bodies no
  longer reuse an earlier block's FROM. This also removes the O(n^2)
  comma classification: separators are precomputed once, then read in O(1).
- Recognize ON DUPLICATE KEY UPDATE, FOR UPDATE/SHARE and LOCK IN SHARE
  MODE contextually rather than treating raw ON/FOR/UPDATE as boundaries.
- Scope resolution to the statement containing the cursor.
- Add negative regression tests for sibling subqueries, the second CTE
  body, multiple statements, ON DUPLICATE KEY UPDATE and FOR UPDATE/SHARE.
- Assert the captured metadata request is COLUMN, scoped to orders/b, with
  an empty prefix.

Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
@mvanhorn

Copy link
Copy Markdown
Contributor Author

All of these are handled in 705e543 -- apologies, I pushed it without following up here.

The backward same-depth FROM search is gone. fromRelationSeparators now does a single forward pass tracking, per paren depth, whether we are inside a FROM clause; SELECT and ; reset that state, so a sibling subquery or a second CTE body cannot inherit an earlier block's FROM. Your select (select id from users u), (select x, y, z| from orders o) resolves to orders o alone now, and resolveOwner("y") comes back empty. Same for the second CTE body, and across statements. Resolution also starts at the statement containing the cursor rather than at token 0.

The boundary list is contextual rather than raw tokens: ON DUPLICATE KEY UPDATE, FOR UPDATE, FOR SHARE and LOCK IN SHARE MODE are matched as phrases, so join predicates and index hints are unaffected. Your on duplicate key update id = values(id), v = s.| example registers dst and src s and leaves v unowned.

One side effect worth mentioning: separators are precomputed once and then read in O(1), so the per-comma backward scan is gone as well.

Your three cases are in as negative tests, along with FOR UPDATE/FOR SHARE and a multi-statement variant. 589 tests pass in the mysql module here on JDK 17.

@openai0229 openai0229 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.

Thank you, @mvanhorn, for the thoughtful and thorough follow-up. I re-reviewed 705e543: the query-block-aware separator scan addresses the earlier scope and performance concerns, the requested negative cases and metadata-request assertions are present, and I verified the branch on top of the current main with the full MySQL reactor suite (618 tests) passing.

This is a solid, focused fix for #1949. Approved, and thank you again for the contribution.

@openai0229
openai0229 merged commit aa63775 into OtterMind:main Jul 27, 2026
9 of 10 checks passed
@openai0229 openai0229 moved this from In Review to Done in Chat2DB Community Jul 27, 2026
@mvanhorn

Copy link
Copy Markdown
Contributor Author

Thanks for merging the dot-scoped completion fix. Appreciate you taking the regression contract for multi-relation MySQL queries along with it.

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

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Add a MySQL second-alias completion regression contract

2 participants