014-code commented on PR #38985: URL: https://github.com/apache/shardingsphere/pull/38985#issuecomment-4880566159
> ### Summary > **Review Result: Not Mergeable** > > **Feedback Mode: Change Request** > > Reason: The implementation direction is aligned with the reported root cause, and the focused tests/checkstyle pass on the latest PR head. However, the regression coverage still stops at the old bindable-compilation failure and does not prove the linked issue's SQL federation result contract. > > ### Issues > #### P1 Missing result-level regression for the reported SQL federation behavior > Problem: The linked issue discussion states that the reported MySQL-compatible SQL should return , and explicitly says the fix should not only catch the runtime exception because the expected result still needs to be produced. The new compiler IT in only asserts that does not throw for the issue-shaped query. That covers the old Calcite/Janino compilation symptom, but it does not bind/enumerate rows or assert the returned aggregate values.`1, 0, 0``kernel/sql-federation/compiler/src/test/java/org/apache/shardingsphere/sqlfederation/compiler/compiler/it/SQLStatementCompilerIT.java:257``EnumerableInterpretable.toBindable(...)` > > Impact: A regression could still compile successfully while returning the wrong value, losing the grouped left-join row, or failing later during execution/result-set access. The production SQL federation path continues after by binding, enumerating, and exposing a in , so the current test stops before the client-visible behavior fixed by #29002 is verified.`COUNT(DISTINCT 1, NULL)``toBindable``SQLFederationResultSet``StandardSQLFederationProcessor` > > Required Change: Please add a deterministic regression that executes or bind/enumerates the reported left join / group by SQL with data matching #29002 and asserts the row values, especially that returns while the adjacent result remains correct.`COUNT(DISTINCT 1, NULL)``0``COUNT(DISTINCT i.product_id)` > > ### Review Details > * Review Focus: Code Correctness Review. CI not reviewed by request. > * Reviewed Scope: PR #38985 latest head ; base ref ; local merge-base . GitHub matched the local triple-dot file list.`020e497870590df08f8a09f96d32eaf447a48cde``master``bd5bce8e5920e8ed393aa25113d9d036e9b18377``/pulls/38985/files` > * Reviewed files: > > * `kernel/sql-federation/compiler/src/main/java/org/apache/shardingsphere/sqlfederation/compiler/sql/ast/converter/segment/projection/impl/AggregationProjectionConverter.java` > * `kernel/sql-federation/compiler/src/test/java/org/apache/shardingsphere/sqlfederation/compiler/compiler/it/SQLStatementCompilerIT.java` > * `kernel/sql-federation/compiler/src/test/java/org/apache/shardingsphere/sqlfederation/compiler/sql/ast/converter/segment/projection/impl/AggregationProjectionConverterTest.java` > * Not Reviewed Scope: GitHub Actions/check-runs and live Proxy/database smoke execution. > * Verification: > > * Focused Maven tests for and : exit 0, 115 tests passed.`AggregationProjectionConverterTest``SQLStatementCompilerIT` > * Scoped for : exit 0.`checkstyle:check``kernel/sql-federation/compiler` > * Scoped search found no existing direct result-level regression under or .`COUNT(DISTINCT ... NULL)``kernel/sql-federation``test/e2e/sql` > * Release Note / User Docs: Not required; this is an internal SQL federation compiler fix with no user-facing configuration, syntax, migration, or compatibility note needed. Thanks for the detailed review. You're right that the current regression only verifies the compilation path (`EnumerableInterpretable.toBindable()`) and does not cover the execution path through `SQLFederationResultSet`. I'll add a deterministic execution-level regression that executes the issue scenario with representative test data and asserts the returned aggregate values, including that `COUNT(DISTINCT 1, NULL)` returns `0` while `COUNT(DISTINCT i.product_id)` remains correct. Thanks for pointing out the missing result-level verification. -- 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]
