Support internal binding and extraction for SQL Server UPDATE table variable targets#39093
Support internal binding and extraction for SQL Server UPDATE table variable targets#39093ClaireLytt wants to merge 4 commits into
Conversation
|
This PR fixes the SQL Server error “org.apache.shardingsphere.infra.exception.kernel.metadata.TableNotFoundException: Table or view '@MyTableVar' does not exist.” when running UPDATE test |
terrymanu
left a comment
There was a problem hiding this comment.
Summary
Review Result: Not Mergeable
Feedback Mode: Change Request
Reason: The unquoted SQL Server table-variable path is implemented and tested, but the prefix-only detection also classifies valid delimited physical table names beginning with @ as variables. This can bypass Encrypt rewriting.
Issues
-
P1: Delimited table names beginning with
@are misclassified as table variablesProblem: Both
TableExtractor.isVariableTableTargetandSimpleTableSegmentBinder.isVariableTableinspect only the unwrapped identifier value withstartsWith("@"); neither checks itsQuoteCharacter. SQL Server distinguishes regular identifiers beginning with@from delimited identifiers such as[@MyTableVar], which remain object names (Microsoft identifier documentation). A latest-head production parser probe confirmed thatUPDATE [@MyTableVar] ...producesIdentifierValue("@MyTableVar", BRACKETS), butTableExtractornow emits no target table.UpdateStatementBaseContextbuildsTablesContextfrom that extractor result. When the target is the only table,EncryptSQLRewriteContextDecoratorconsequently sees no Encrypt table and returns without rewriting. The binder also suppresses physical-table existence validation for the same quoted name.Impact: A valid update against a physical SQL Server Encrypt table whose delimited name begins with
@can skip Encrypt rewriting or receive incorrect metadata validation. This is a regression from the merge-base behavior, which extracted every non-alias update target.Required Change: Make table-variable recognition quote-aware at both detection sites: the prefix must identify a variable only when the identifier is unquoted (
QuoteCharacter.NONE). Add regression coverage proving that bracket-delimited and double-quoted physical names beginning with@remain inTablesContext, retain normal binder validation, and activate Encrypt rewriting, while unquoted@MyTableVarcontinues through the table-variable path.
Review Details
- Review Focus: Code Correctness Review. CI not reviewed by request.
- Reviewed Scope: All 20 files at head
f395ec505f78, covering SQL Server metadata, parser statement extraction, binder behavior, SQL federation conversion tests, Encrypt rewriter integration fixtures, build configuration, release notes, and repository ignore configuration. The local triple-dot inventory matched GitHub’s latest PR file list. - Not Reviewed Scope: Live execution against a native SQL Server instance and unrelated DML/MERGE work tracked by the broader umbrella issue.
- Verification: 58 focused connector/parser/binder/converter tests passed; all 186
EncryptSQLRewriterITcases passed; scoped Spotless and Checkstyle checks passed with no violations. A production parser-to-extractor probe reproduced the quoted-identifier regression while confirming the unquoted variable path. - Release Note / User Docs: The release note is present and appropriately scoped. No additional user documentation is required for this change.
terrymanu
left a comment
There was a problem hiding this comment.
Summary
Review Result: Not Mergeable
- Feedback Mode: Needs Discussion
- Reason: The tested rewrite is not reachable through a valid ShardingSphere execution topology for SQL Server table variables, and binder precedence can still confuse an unquoted variable with a same-named delimited physical table.
Issues
P1 — The claimed Encrypt path is not runnable in the current execution topology
- Problem: SQL Server table variables exist only within the batch, stored procedure, or function where they are declared (Microsoft documentation). However, the new test supplies only a standalone
UPDATE @MyTableVar. ShardingSphere’s SQL Server grammar accepts one statement followed by EOF (SQLServerStatement.g4); the JDBC path parses each submitted SQL independently (ShardingSphereStatement.java), and statement batching executes each SQL separately (BatchStatementExecutor.java). A procedure does not close the gap because the SQL Server visitor currently discards its body (SQLServerDDLStatementVisitor.java). - Impact: The rewrite fixture proves an isolated internal stage, but no current public execution path can both keep
@MyTableVarin scope and apply the Encrypt rewrite. TheRELEASE-NOTES.mdclaim therefore advertises support that applications cannot use. - Discussion Needed: Please pause this implementation direction and align with maintainers on the supported topology: multi-statement batch rewriting, routine-body rewriting, or explicitly scoped internal groundwork without a user-facing support claim. Recommend
type: discussion.
P1 — Physical metadata can shadow an unquoted table variable
- Problem: SQL Server distinguishes unquoted
@MyTableVar, which is necessarily a variable, from the delimited physical identifier[@MyTableVar](Microsoft identifier rules). InSimpleTableSegmentBinder.java, physical metadata lookup occurs beforeisVariableTable. Because metadata retains the name@MyTableVarwithout delimiter identity,IdentifierIndex.findmatches it and returns a physical-table binder context. The table-variable flag is never set, soColumnSegmentBindervalidates the variable’s columns against the unrelated physical table. - Impact: A valid variable update can raise a false
ColumnNotFoundExceptionor receive incorrect physical-table column ownership whenever a delimited physical table with the same text exists. - Discussion Needed: Please include identifier-kind ownership and precedence in the reopened design. An unquoted
@target must remain a variable even when identical text exists in physical metadata, while a delimited target must remain a physical object.
Multi-Round Comparison
The previous quoted-identifier blocker is fixed at head 481389309471: both classification sites now require QuoteCharacter.NONE, and regression tests cover bracket- and quote-delimited physical names. The blockers above are newly discovered: the same-name metadata precedence case remains uncovered, and the standalone rewrite still lacks a runnable production topology.
Review Details
- Review Focus: Code Correctness Review — CI not reviewed by request.
- Reviewed Scope: All 20 changed files at head
481389309471509462978368c495c3853df18521; local merge-base299926f1bae87123f75cdf31e27245b06b758f8f; local changed-file list matched GitHub/pulls/39093/files20/20. Reviewed database connector metadata, SQL Server activation, parser table extraction, binder contexts and column binding, federation conversion coverage, Encrypt rewrite fixtures, test-scope dependencies, release notes, and the latest delta from the previous public review. Target dialect: SQL Server; no branch dialect applies. Syntax validation used Microsoft’sUPDATE,table, and identifier documentation, plusdocs/document/content/features/encrypt/limitations.en.mdanddocs/document/content/dev-manual/sql-parser.en.md. - Not Reviewed Scope: GitHub Actions/check runs and live execution against a SQL Server backend.
- Verification: Scoped
spotless:checkandcheckstyle:checkcompleted with exit code 0. Focused SQL Server metadata, extractor, binder, context, and federation tests completed with 62/62 tests passing and exit code 0.EncryptSQLRewriterITcompleted with 186/186 tests passing and exit code 0. The GitHub app connector was unavailable because no session was logged in; read-only GitHub REST provided the PR metadata, files, commits, comments, and reviews, so this did not affect the findings. - Release Note / User Docs: A release-note entry is present, but its support claim is not accurate until the execution-topology discussion is resolved. Existing Encrypt documentation explicitly states that simultaneous semicolon-separated statements are unsupported.
|
Could maintainers please advise which direction I should take?
For this PR, I lean toward option 3: keep the binder/table-extractor groundwork and tests, but avoid a user-facing Encrypt support claim until a runnable path exists. I’m open to maintainer guidance if another direction is preferred. Also, should I remove or soften the RELEASE-NOTES / Encrypt “supported” wording until a runnable path exists? btw, @terrymanu Could you please add a tag |
|
Thanks for clarifying. Please proceed with option 3 and keep this PR limited to internal support for binding, extracting, and rewriting SQL Server Please address the remaining items:
The quoted-identifier issue is already fixed. With the scope clarified, a |
for #30227 #38904
official document:https://learn.microsoft.com/en-us/sql/t-sql/queries/update-transact-sql?view=sql-server-ver17#m-specifying-a-table-variable-as-the-target-object
Changes proposed in this pull request:
Before committing this PR, I'm sure that I have checked the following options:
./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.