strongduanmu commented on PR #40342:
URL: https://github.com/apache/shardingsphere/pull/40342#issuecomment-6358545920
### Result
Review Result: Mergeable
The code correctness review passed for commit
`a2496a0d4c802a02a14730dc8a15c7a58e6e9e75`, rebased onto upstream master
`50cd58771c484f5f6b281a349aff9c3f7c27ffbb`. No blocking issues or recommended
changes remain within the reviewed scope.
This records the completed Codex-assisted review and local verification. It
is a code-scope assessment, not a maintainer approval or confirmation that all
CI checks have passed.
### Behavior and Compatibility
- **ORDER BY resolution:** Projection matches retain precedence. When the
projection scope has no matching column, the current query's FROM scope is
searched before correlated outer scopes. Reviewed qualified owner lookup, local
alias shadowing, ambiguous local columns, genuine outer references, and
external table contexts. Non-ORDER BY resolution paths retain their existing
behavior.
- **Temporary scope lifecycle:** ORDER BY binding installs the current FROM
scope and restores the previous scope in a `finally` block, including failure
paths. Nested subqueries create their own binder context, preventing the
temporary scope from leaking into nested SELECT binding.
- **SELECT metadata:** The `intoVariables` flag survives both statement
copying through `withSubqueryType` and SELECT binding. The rebase conflict was
resolved by retaining upstream's inline builder and Optional FROM handling
while preserving the flag. Existing callers retain the default `false` behavior.
- **LOAD DATA metadata:** The shared statement model retains the existing
MySQL metadata and inherited accessor contracts. The MySQL statement remains
final with its existing public constructor, and the binder copies the newly
exposed statement and column-list index metadata. This does not introduce new
SQL syntax support or activate another dialect.
- **Allocation and lookup changes:** Reverse outer-scope traversal preserves
lookup order; variable-name matching remains case-insensitive and
short-circuiting. Reusing immutable table-bound metadata does not introduce
mutable aliasing. The qualified-function expression retains upstream's
equivalent string construction.
- **Release Notes:** The current development version contains two
enhancement entries describing ORDER BY scope resolution and statement metadata
preservation, both linked to this PR.
### Local Verification
The following focused reactor test command completed with exit code `0` and
`BUILD SUCCESS`:
```sh
./mvnw -B \
-pl
parser/sql/statement/core,parser/sql/statement/dialect/mysql,infra/binder/core,infra/binder/dialect/mysql
\
-am -DskipITs -Dspotless.skip=true -Dcheckstyle.skip=true \
-Dtest=ColumnSegmentBinderTest,OrderBySegmentBinderTest,SelectStatementBinderTest,SelectStatementTest,MySQLLoadDataStatementBinderTest
\
-Dsurefire.failIfNoSpecifiedTests=false test
```
Actual executed tests: **64 passed, 0 failures, 0 errors**:
- `ColumnSegmentBinderTest`: 40
- `OrderBySegmentBinderTest`: 3
- `SelectStatementBinderTest`: 20
- `SelectStatementTest`: 1
The MySQL statement and binder modules compiled from current reactor
sources. No dedicated `MySQLLoadDataStatementBinderTest` matched the requested
filter; the test count above does not claim otherwise.
Spotless and Checkstyle completed with exit code `0` using exact file
filters for all 12 Java files changed by the PR. All four owning modules
reported zero Checkstyle violations:
- `infra/binder/core`: `ColumnSegmentBinder`, `OrderBySegmentBinder`,
`SQLStatementBinderContext`, `SelectStatementBinder`, and the corresponding
changed `ColumnSegmentBinderTest`, `OrderBySegmentBinderTest`, and
`SelectStatementBinderTest` files.
- `infra/binder/dialect/mysql`: `MySQLLoadDataStatementBinder`.
- `parser/sql/statement/core`: `LoadDataStatement`, `SelectStatement`, and
`SelectStatementTest`.
- `parser/sql/statement/dialect/mysql`: `MySQLLoadDataStatement`.
The repository license check also passed with exit code `0`:
```sh
./mvnw apache-rat:check -Pcheck -T1C
```
The only source edit after the functional test and RAT runs joined an
expression onto one line without changing its tokens or license header; the
final exact-file style checks passed afterward.
### Review Coverage
Two mutually blind independent assessments covered all 13 changed files,
including `RELEASE-NOTES.md`, through root-cause and behavior analysis,
affected contracts and consumers, tests and runtime behavior, and a final
convergence pass. Their reports were substantively checked and reconciled; no
unresolved differences or outcome-sensitive evidence gaps remain.
The comparison with the original PR commit
`dcd60e594a85cc58f03733f8247f654bd4720351` confirmed that the synchronization
preserved the proposed behavior while adapting to upstream's SELECT builder
relocation and qualified-function expression changes. The additional task
changes were the enhancement-focused commit/title and Release Note entries.
Direct static mocks in the ORDER BY tests were examined: those tests verify
static interactions using handles that `AutoMockExtension` does not expose, and
the mocks are closed through try-with-resources.
**Limits:** The full `./mvnw clean install -B -T1C -Pcheck` build was not
run. GitHub Actions and CI status were outside this code correctness review.
--
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]
