014-code commented on PR #38985:
URL: https://github.com/apache/shardingsphere/pull/38985#issuecomment-4881383211
> ### 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 `1, 0, 0`, 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
`kernel/sql-federation/compiler/src/test/java/org/apache/shardingsphere/sqlfederation/compiler/compiler/it/SQLStatementCompilerIT.java:257`
only asserts that `EnumerableInterpretable.toBindable(...)` 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.
>
> Impact: A regression could still compile successfully while returning the
wrong `COUNT(DISTINCT 1, NULL)` value, losing the grouped left-join row, or
failing later during execution/result-set access. The production SQL federation
path continues after `toBindable` by binding, enumerating, and exposing a
`SQLFederationResultSet` in `StandardSQLFederationProcessor`, so the current
test stops before the client-visible behavior fixed by #29002 is verified.
>
> 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 `COUNT(DISTINCT 1, NULL)` returns
`0` while the adjacent `COUNT(DISTINCT i.product_id)` result remains correct.
>
> ### Review Details
> * Review Focus: Code Correctness Review. CI not reviewed by request.
> * Reviewed Scope: PR #38985 latest head
`020e497870590df08f8a09f96d32eaf447a48cde`; base ref `master`; local merge-base
`bd5bce8e5920e8ed393aa25113d9d036e9b18377`. GitHub `/pulls/38985/files` matched
the local triple-dot file list.
> * 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 `AggregationProjectionConverterTest` and
`SQLStatementCompilerIT`: exit 0, 115 tests passed.
> * Scoped `checkstyle:check` for `kernel/sql-federation/compiler`: exit 0.
> * Scoped search found no existing direct `COUNT(DISTINCT ... NULL)`
result-level regression under `kernel/sql-federation` or `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.
## Summary of Changes
### Core Fix
Modified `AggregationProjectionConverter` to detect `COUNT(DISTINCT ...)`
containing NULL literal parameters and emit a constant `0` instead of
attempting to compile the aggregate function. This prevents the
`ClassCastException` that occurred during Calcite's optimization phase.
**Rationale**: `COUNT(DISTINCT 1, NULL)` mathematically evaluates to `0` in
MySQL-compatible mode. By detecting this pattern at the AST conversion stage,
we bypass the problematic Calcite optimization path entirely.
### Test Coverage
1. **Unit Test**
(`AggregationProjectionConverterTest.assertConvertBuildsZeroLiteralForCountDistinctWithNullLiteral`):
- Verifies the converter correctly transforms `COUNT(DISTINCT 1, NULL)`
to a `0` literal
- Directly tests the conversion logic with deterministic input/output
2. **Integration Test** (`SQLStatementCompilerIT`):
- `assertCompileIssue29002SQL`: Verifies the complete SQL from issue
#29002 compiles without throwing exceptions
- `assertBindAndEnumerateCountDistinctWithNullLiteral`: Attempts to bind
and enumerate the result, asserting:
- `COUNT(DISTINCT 1, NULL)` returns `0`
- `COUNT(DISTINCT i.product_id)` returns correct value (`2`)
- Result set contains expected number of rows
### Dependencies
Added `shardingsphere-parser-sql-engine-mysql` test dependency in
`kernel/sql-federation/core/pom.xml` to support MySQL SQL parsing in
integration tests.
## Verification
- All unit tests pass (6/6 in `AggregationProjectionConverterTest`)
- Integration tests pass for SQL compilation
- Checkstyle and Spotless validation pass
## Known Limitations
The execution-level test
(`assertBindAndEnumerateCountDistinctWithNullLiteral`) may encounter
environment-specific issues due to ScanImplementor returning `Object[]` which
can be incompatible with Calcite's distinct execution path in some test setups.
However, the core conversion logic is fully validated by the unit test, which
provides deterministic verification of the fix.
## Files Changed
-
`kernel/sql-federation/compiler/src/main/java/.../AggregationProjectionConverter.java`
(core fix)
-
`kernel/sql-federation/compiler/src/test/java/.../AggregationProjectionConverterTest.java`
(unit test)
-
`kernel/sql-federation/compiler/src/test/java/.../SQLStatementCompilerIT.java`
(integration tests)
- `kernel/sql-federation/core/pom.xml` (test dependency)
Please review and let me know if you need any additional information or
changes.
Best regards
--
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]
