yx9o commented on PR #38985: URL: https://github.com/apache/shardingsphere/pull/38985#issuecomment-4877910084
### Summary **Review Result: Mergeable** Reason: This is code-scope only. The latest PR head repairs the linked SQL federation compile path for `COUNT(DISTINCT ..., NULL)` and adds focused converter plus compiler coverage. CI not reviewed by request. ### Evidence The linked issue #29002 reproduces a SQL federation failure for a MySQL query containing `count(distinct 1,NULL)`, with the failure occurring during Calcite/Janino bindable generation. `AggregationProjectionConverter` now detects `COUNT DISTINCT` aggregates containing a direct `NULL` literal parameter and emits `COUNT(NULL)` without the `DISTINCT` quantifier. That preserves the expected result for the issue case because a multi-expression `COUNT(DISTINCT ...)` tuple containing `NULL` contributes no counted row, while avoiding the invalid distinct key generation path. The new tests cover both levels needed for this fix: - `AggregationProjectionConverterTest` verifies the converter emits `COUNT(NULL)` with no distinct quantifier. - `SQLStatementCompilerIT` parses and compiles the issue-shaped left-join/group-by query through `EnumerableInterpretable.toBindable`, matching the reported Janino failure path. ### Review Details Review Focus: Code Correctness Review. CI not reviewed by request. Reviewed Scope: PR #38985 latest head `020e497870590df08f8a09f96d32eaf447a48cde`; base/merge-base `bd5bce8e5920e8ed393aa25113d9d036e9b18377`; 3 changed files under `kernel/sql-federation/compiler`. Local scope matches the GitHub files page. Not Reviewed Scope: GitHub Actions/check-runs, full repository CI, and live Proxy/database execution. Verification: - `git diff --name-status bd5bce8..020e497`: exit 0; confirmed the 3-file scope. - Byte-level scan of changed files at `020e497`: exit 0; LF line endings, no bare CR, no non-ASCII control characters. - Local Maven tests were not rerun successfully in this shell because current-head focused runs are blocked by local generated-source/JDK environment constraints; the review result is based on current-head code/test path inspection. Release Note / User Docs: Not required; this is an internal SQL federation compiler fix with no user-facing configuration, syntax, or migration change. -- 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]
