terrymanu commented on PR #39093:
URL: https://github.com/apache/shardingsphere/pull/39093#issuecomment-5462556257
After rechecking the cross-module changes, I’m keeping the scope concern.
The common binding, extraction, and rewriting paths do need changes, but the
current patch has grown beyond what is necessary.
Please simplify the PR as follows:
1. Keep `SQLServerStatementVisitor` as the single owner of
`targetTableIsFromAlias`.
`SimpleTableSegmentBinder` and `TableExtractor` should trust this value.
They should not walk the FROM clause and re-evaluate whether the target, alias,
and original source have the same table-variable classification.
In particular, please simplify or remove:
- `SimpleTableSegmentBinder#isTargetTableMatchedFromAlias`
- The table-variable identity filtering in
`findUpdateTargetFromTableContexts`
- `TableExtractor#isFromAliasTarget`
- `TableExtractor#isTargetMatchedInFrom`
- `TableExtractor#matchesFromAlias`
- `TableExtractor#matchesFromTableNameWithAlias`
- The related recursive owner comparison
Fixing only `SQLServerStatementVisitor#isAliasInFromClause` is not
enough. The current Binder and Extractor checks would still reject:
`UPDATE [@MyTable] SET Remark = 'x' FROM @MyTable AS [@MyTable]`
2. Remove the new
`UpdateStatementConverterTest#assertConvertWithTableVariableTarget` test.
SQL Federation production code is unchanged, and this test only verifies
that the identifier string `@MyTableVar` is copied unchanged. It would already
pass without this PR and does not test the new binding, extraction, or
rewriting behavior.
3. Reduce the duplicated test matrix.
The PR currently adds:
- 34 tests to `UpdateStatementBinderTest`
- 27 tests to `TableExtractorTest`
- 20 tests to `UpdateStatementContextTest`
- 47 Encrypt rewrite assertions
- 8 parser cases
Please keep one representative test for each distinct production branch
instead of testing the full combination of bracket/double-quote,
schema/no-schema, alias/no-alias, JOIN order, and literal/parameter at every
layer.
Most of the new `UpdateStatementContextTest` cases duplicate
`TableExtractorTest`, because `UpdateStatementBaseContext` simply delegates
table collection to `TableExtractor`. Only a small number of integration cases
are needed there.
For Encrypt rewriting, literal and parameter cases must both remain
because they use different rewriters, but they do not both need every quoting,
schema, alias, and JOIN combination.
4. Avoid adding unrelated test fixtures where existing ones can be reused.
The same-named encrypted `@MyTable` fixture is useful because it verifies
that an unquoted table variable takes precedence over physical metadata.
However, the additional `HumanResources.Employee` metadata and Encrypt
configuration are not essential if an existing encrypted table such as
`ScrapReason` can cover the same physical-source and column-collision behavior.
Please keep the necessary shared changes:
- `DialectDatabaseMetaData#isTableVariableIdentifier`, its SQL Server
implementation, delegation, and contract tests
- SQL Server UPDATE-specific table-variable handling in the Binder
- Binding SET columns to the table-variable target while preserving normal
validation for physical targets
- Excluding table variables from `TableExtractor`
- Skipping table-variable target columns in both Encrypt token and parameter
rewriting
- The test-scope SQL Server dependencies required to load the dialect
metadata SPI
The goal is not to move everything back into the SQL Server modules. It is
to keep the necessary shared pipeline changes while removing duplicated SQL
semantics, unrelated coverage, and repeated test combinations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
