strongduanmu commented on PR #38724:
URL: https://github.com/apache/shardingsphere/pull/38724#issuecomment-4543077891
### Decision
- **Merge Verdict: Mergeable**
- **Reviewed Scope:** Reviewed PR #38724 latest checked head `930f40f`,
mainly covering E2E SQL DQL dataset initialization and native storage container
initialization refactor.
- **Not Reviewed Scope:** Local CI/E2E/Maven execution was not run. Parser
`UpdateStatementTestCase` with-clause assertion and formatting-only issues are
intentionally ignored in this round.
- **Need Expert Review:** No.
### Basis
- The previous DQL expected datasource blocker has been addressed.
`BaseDQLE2EIT` now fills both `Type.ACTUAL` and `Type.EXPECTED`, so non-XML DQL
assertions using `expected-data-source-name` can read rows from the expected
datasource
instead of comparing against an empty initialized schema.
- The restored expected dataset path matches the existing assertion flow
in `GeneralDQLE2EIT`, where actual query results are compared with queries
executed against the expected datasource.
- The native initialization cache concern is treated as non-blocking for
the current test execution model, based on the current passing test result and
the project’s actual E2E execution behavior.
- The change is scoped to the E2E test framework and parser expected-case
JAXB model. It does not affect production SQL execution, routing, parser
runtime behavior, JDBC/Proxy runtime paths, or user-facing configuration.
### Review Notes
- `BaseDQLE2EIT` now uses a guard-clause style around `FILLED_SUITES`,
which keeps the same “fill once per key + datasource-map identity” behavior
while making the flow easier to read.
- Restoring `Type.EXPECTED` is important because many DQL cases still rely
on `expected-data-source-name`, for example cases that compare actual results
against `read_dataset` or `expected_dataset`.
- `NativeStorageContainer` adds a static initialization cache to avoid
repeated init SQL execution in native mode. This is acceptable for the current
PR after the native lifecycle concern is considered non-blocking.
- The parser JAXB addition for update `with` expected data is kept as a
test data model extension only. Assertion coverage for `with` is intentionally
left out of scope for this review round.
### Pre-Merge Checks
- CI and E2E are considered passed based on the provided PR context.
- Local CI/E2E/Maven checks were intentionally skipped.
- No remaining blocker is raised in the reviewed scope.
### Multi-Round Comparison
- `DQL expected datasource rows are not filled`: Fixed.
- `Native initialization cache may outlive composer lifecycle`: Accepted
as non-blocking for this PR.
- `Update with-clause expected data is bound but not asserted`: Ignored by
review decision.
- Formatting-only findings: Ignored by review decision.
### Suggested Follow-up
- If native E2E execution later shows flaky behavior after switching among
SQL command types or scenarios, revisit the initialization cache lifecycle and
align it with `ContainerComposerRegistry.close()`.
--
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]
