Support evaluation of IFNULL/COALESCE scalar wrappers (with literal or nested-aggregation fallbacks) over merged aggregation results#38990
Conversation
|
@terrymanu Can you review this pr? |
yx9o
left a comment
yx9o
left a comment
There was a problem hiding this comment.
Summary
Review Result: Not Mergeable
Feedback Mode: Change Request
Reason: The PR direction is aligned with the linked issue, but the new expression-derived aggregation path breaks expression-wrapped AVG(...). The binder creates COUNT/SUM derived projections for AVG, and the merge layer requires those derived columns, but the sharding projection rewrite only appends the expression-derived AVG(...) column itself. As a result, queries such as IFNULL(AVG(price), 0) or COALESCE(AVG(price), 0) can fail before merge/index setup or cannot compute the correct cross-shard average.
Issues
P1: Expression-derived AVG does not rewrite its required COUNT/SUM derived projections
Problem: ProjectionEngine now extracts aggregations from IFNULL / COALESCE expressions and creates expression-derived aggregation projections. For nested AVG, it correctly calls appendAverageDerivedProjection(...), which attaches the required COUNT(...) and SUM(...) derived projections to the expression-derived AVG (infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/segment/select/projection/engine/ProjectionEngine.java:143-148, :230-240).
However, ShardingProjectionsTokenGenerator only appends each expression-derived aggregation itself to the actual SQL (features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/rewrite/token/generator/impl/ShardingProjectionsTokenGenerator.java:103-105). It does not also append that aggregation's getDerivedAggregationProjections() children. This differs from the existing top-level aggregation path, which explicitly appends derived aggregation projections (ShardingProjectionsTokenGenerator.java:92-95).
The downstream contract requires those child columns to exist. SelectStatementBaseContext#setIndexForAggregationProjection iterates projectionsContext.getAggregationProjections(), sets the expression-derived aggregation index, and then requires every child derived aggregation alias to be present in the result-set label map (infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/statement/type/dml/SelectStatementBaseContext.java:293-302). The merge layer then reads those child values for any aggregation whose derived projections are non-empty (features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/merge/dql/groupby/GroupByMemoryMergedResult.java:93-102, features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/merge/dql/groupby/GroupByStreamMergedResult.java:111-121). AverageAggregationUnit also expects exactly count and sum values (features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/merge/dql/groupby/aggregation/AverageAggregationUnit.java:37-49).
Impact: A sharding query like SELECT IFNULL(AVG(price), 0) FROM t_order or SELECT COALESCE(AVG(price), 0) FROM t_order GROUP BY user_id needs the actual routed SQL to return COUNT(price) and SUM(price) for correct cross-shard AVG merging. This PR only adds the expression-derived AVG(price) column, so the result-set labels do not contain AVG_DERIVED_COUNT_* / AVG_DERIVED_SUM_*. That can fail with Can't find index during setIndexes(...); if bypassed, it still cannot compute the correct merged AVG from shard-level partial averages.
Required Change: Please make the expression-derived aggregation rewrite/index path preserve the normal derived aggregation semantics recursively. For expression-derived AVG / AVG DISTINCT, the routed SQL must include the generated COUNT and SUM derived projections, those projections must receive result-set indexes, and the merge/evaluator path should use the final merged AVG value. Please also add a regression test for expression-wrapped AVG, for example IFNULL(AVG(...), 0) and/or COALESCE(AVG(...), 0), covering the rewritten projection text and the group-by merge path.
Review Details
Review Focus: Code Correctness Review. CI not reviewed by request.
Reviewed Scope: PR #38990 latest head 9de5f81a92a0fda3aac0bd7e019ebdd927631e0f, base master at 7a03f99acdc277dd809665699e296072e5e9176d. GitHub /pulls/38990/files returned 14 files, matching the public diff file list. Reviewed the binder projection changes, sharding projection rewrite, group-by memory/stream merge changes, evaluator, release note, and related tests.
Not Reviewed Scope: GitHub Actions/check-runs and full Maven verification were not reviewed because this was a code-correctness review and a static code-path blocker was confirmed.
Verification: Used public GitHub REST metadata/files and public PR diff/raw source for latest-head review. No local Maven command was run; the blocker is proven from the projection creation -> SQL rewrite -> index assignment -> merge aggregation path.
Release Note / User Docs: Release note entry is present for the new IFNULL/COALESCE aggregation-expression support. No additional user docs were required for this blocker beyond fixing the behavior and adding regression coverage.
|
@yx9o Thanks for the detailed review. I've addressed the issues you pointed out. The rewrite logic now preserves the derived aggregation semantics for expression-derived aggregations by appending the derived projections of expression-wrapped I've also added regression tests covering:
Additionally, the failing CI jobs appear to be unrelated to these changes, as they fail during Maven download before the build or tests are executed. |
|
Please fix GitHub actions |
|
@terrymanu This Spotless fix is clearly failing due to an internal server error, especially since it passed previously. 
|
terrymanu
left a comment
terrymanu
left a comment
There was a problem hiding this comment.
Summary
Review Result: Not Mergeable
Feedback Mode: Change Request
Reason: The PR direction is aligned with the linked issue, and the current head appears to fix the previous expression-wrapped AVG derived-column rewrite concern. However, the latest expression-derived aggregation implementation still has two code-level blockers: it downgrades DISTINCT aggregations to non-distinct merge units, and it bypasses the existing guard that keeps windowed aggregations out of sharding aggregation merge.
Issues
P1: DISTINCT aggregations inside IFNULL/COALESCE lose DISTINCT semantics during merge
Problem: AggregationDistinctProjectionSegment extends AggregationProjectionSegment, so the new extractor accepts SUM(DISTINCT ...) / AVG(DISTINCT ...) inside IFNULL or COALESCE. But ProjectionEngine always wraps extracted segments as plain AggregationProjection at infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/segment/select/projection/engine/ProjectionEngine.java#L136-L145. Later, both group-by merge paths choose the aggregation unit with input instanceof AggregationDistinctProjection (features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/merge/dql/groupby/GroupByMemoryMergedResult.java#L86-L88, features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/merge/dql/groupby/GroupByStreamMergedResult.java#L92-L94). Because the expression-derived projection is no longer an AggregationDistinctProjection, IFNULL(SUM(DISTINCT price), 0) is merged as a normal SUM over each shard's SUM(DISTINCT price) result.
Impact: This can return incorrect cross-shard results. For example, if two shards both contain price = 10, each shard returns SUM(DISTINCT price) = 10; the correct global SUM(DISTINCT price) is 10, but the current merge path accumulates 20 and the evaluator writes that value back to the expression column. AVG(DISTINCT ...) has the same class of problem when per-shard distinct count/sum pairs contain duplicate values across shards.
Required Change: Preserve AggregationDistinctProjectionSegment as an AggregationDistinctProjection in the expression-derived aggregation path, including its existing distinct rewrite and distinct aggregation-unit contract, or explicitly exclude distinct aggregations from this new supported path instead of merging them as non-distinct. Please add regression coverage with duplicate distinct values across shards for IFNULL or COALESCE.
P1: Windowed aggregations inside IFNULL/COALESCE are reclassified as sharding merge aggregations
Problem: Existing projection binding deliberately treats windowed aggregation projections as expressions: createProjection(AggregationProjectionSegment) checks projectionSegment.getWindow().isPresent() and returns an ExpressionProjection at infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/segment/select/projection/engine/ProjectionEngine.java#L174-L177; the current test suite covers that contract at infra/binder/core/src/test/java/org/apache/shardingsphere/infra/binder/context/segment/select/projection/engine/ProjectionEngineTest.java#L124-L132. The new nested extractor does not carry the same guard: it adds any AggregationProjectionSegment found under IFNULL/COALESCE at ProjectionEngine.java#L188-L204. Once added, ShardingDQLResultMerger sees non-empty aggregation projections and routes the query into group-by merge at features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/merge/dql/ShardingDQLResultMerger.java#L101-L122.
Impact: A parsed expression such as IFNULL(SUM(price) OVER (...), 0) can now get a derived projection and aggregation merge even though the existing binder contract treats windowed aggregation as an expression, not as a cross-shard aggregate to accumulate. That can change result shape and values for already-supported window aggregation projections when users wrap them with the newly supported functions.
Required Change: Keep the existing window guard in the expression-derived extraction path, for example by skipping AggregationProjectionSegment instances whose getWindow() is present. Please add a regression test proving IFNULL/COALESCE around a windowed aggregation does not populate expressionDerivedAggregations and does not trigger the new derived projection rewrite.
Multi-Round Comparison
The previous public change request about expression-wrapped AVG missing its derived COUNT/SUM rewrite appears fixed in the current head: ShardingProjectionsTokenGenerator now appends expression-derived aggregations and their generated derived aggregation projections at features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/rewrite/token/generator/impl/ShardingProjectionsTokenGenerator.java#L103-L106. The two issues above are newly identified current-head blockers in adjacent expression-derived aggregation cases.
Review Details
- Review Focus: Code Correctness Review. CI not reviewed by request.
- Reviewed Scope: Latest PR head
927a30cea2aa2d25e8537e218685c6e1946b4445; GitHub PR base SHA7a03f99acdc277dd809665699e296072e5e9176d; local merge-base4004670ef7044e7bea76d4d3d29c425d4a8a0b37. Reviewed all 15 files from GitHub/pulls/38990/filesacrossinfra/binder/core,features/sharding/core, andRELEASE-NOTES.md; local triple-dot file list matched GitHub. - Not Reviewed Scope: GitHub Actions/check-runs/logs, full Maven reactor, E2E/native-client verification, and areas outside the PR changed-file list.
- Verification: Fetched PR metadata/files/comments/reviews and local refs successfully; compared GitHub file list with local triple-dot diff successfully; performed static code-path review from binder extraction through SQL rewrite, index setup, group-by merge, and expression evaluation. No Maven command was run because the blockers are established by static current-head code paths; CI was not reviewed for this code-correctness review.
- Release Note / User Docs: Release note entry is present. Additional user docs are not required for this targeted SQL merge capability once the semantic blockers are fixed.
|
@terrymanu Thanks for the detailed review. I've addressed both issues you pointed out:
I also added regression tests covering both cases:
All tests are passing after these changes. |
terrymanu
left a comment
terrymanu
left a comment
There was a problem hiding this comment.
Summary
Review Result: Not Mergeable
Feedback Mode: Change Request
Reason: The PR direction is aligned with issue #37644, and the latest head appears to fix the previous AVG-derived-column and window-aggregation concerns. The previous DISTINCT concern is also partially fixed by preserving AggregationDistinctProjection, but one required part remains unresolved: expression-derived DISTINCT aggregations still do not enter the existing distinct SQL rewrite path.
Issues
P1: Expression-derived DISTINCT aggregations still miss the DISTINCT rewrite path
Problem: The latest head now preserves nested distinct aggregations as AggregationDistinctProjection under IFNULL / COALESCE (infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/segment/select/projection/engine/ProjectionEngine.java#L183-L202). That fixes the type-loss part of the previous review comment.
However, these projections are stored only in expressionDerivedAggregations, while ProjectionsContext#createAggregationDistinctProjections still collects only top-level projections (infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/segment/select/projection/ProjectionsContext.java#L65-L86). The distinct rewrite generators still depend solely on getAggregationDistinctProjections() (features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/rewrite/token/generator/impl/ShardingDistinctProjectionPrefixTokenGenerator.java#L34-L40, features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/rewrite/token/generator/impl/ShardingAggregationDistinctTokenGenerator.java#L41-L58), so they are not triggered for expression-derived distinct aggregations.
The only rewrite that handles these nested projections is the append-only expression-derived projection path, which appends the distinct inner expression as a normal derived column (features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/rewrite/token/generator/impl/ShardingProjectionsTokenGenerator.java#L103-L115). That is not equivalent to the existing top-level distinct aggregation rewrite contract, where the routed SQL must provide raw distinct values for the distinct merge units.
Impact: Queries such as SELECT IFNULL(SUM(DISTINCT price), 0) FROM t_order or SELECT COALESCE(AVG(DISTINCT price), 0) FROM t_order GROUP BY user_id can still get an incorrect routed result shape for cross-shard distinct aggregation merge. This is the remaining part of the previous DISTINCT blocker, not a separate new issue.
Required Change: Please make expression-derived AggregationDistinctProjections participate in the same distinct rewrite contract as top-level distinct aggregations, or explicitly exclude distinct aggregations from this new expression-derived support path. Please also add regression coverage for final SQL-token generation and a two-shard duplicate-value merge case for IFNULL or COALESCE around SUM(DISTINCT ...) and/or AVG(DISTINCT ...).
Multi-Round Comparison
The previous AVG-derived rewrite blocker appears fixed in the current head: expression-derived AVG projections and their generated COUNT/SUM children are now appended by ShardingProjectionsTokenGenerator (features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/rewrite/token/generator/impl/ShardingProjectionsTokenGenerator.java#L103-L107).
The previous window-aggregation concern also appears fixed: windowed nested aggregations are skipped during extraction (infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/segment/select/projection/engine/ProjectionEngine.java#L205-L224) and covered by a focused test.
The previous DISTINCT concern is partially fixed: nested distinct aggregations now remain AggregationDistinctProjections. The remaining issue is that these expression-derived distinct projections still bypass the existing distinct SQL rewrite path, so the previous DISTINCT change request is not fully resolved yet.
Review Details
- Review Focus: Code Correctness Review. CI not reviewed by request.
- Reviewed Scope: PR #38990 latest head
4b9afb429d73e3eb978025815740703aa040889c; GitHub PR base SHA7a03f99acdc277dd809665699e296072e5e9176d; localapache/masterSHAaef7d13c0a39570e31b6b38a70239df86ec4636d; local merge-base4004670ef7044e7bea76d4d3d29c425d4a8a0b37. Reviewed all 15 files returned by GitHub/pulls/38990/filesacrossinfra/binder/core,features/sharding/core, andRELEASE-NOTES.md; the local triple-dot file list matched GitHub. - Not Reviewed Scope: GitHub Actions/check-runs/logs, full Maven reactor, E2E verification, and files outside the PR changed-file list.
- Verification: Used public GitHub REST metadata/files/comments/reviews and issue #37644; fetched local PR/base refs; compared GitHub file list with local triple-dot diff; performed static code-path review from binder extraction through SQL-token generation, index setup, group-by merge, and expression evaluation. No Maven command was run because the blocker is established by current-head static code paths, and CI was intentionally not reviewed for this Code Correctness Review.
- Release Note / User Docs: Release note entry is present. Additional user docs are not required for this targeted capability once the distinct semantics are fixed or explicitly excluded.
|
@terrymanu Thanks for the detailed review. I've addressed the remaining DISTINCT rewrite concern by making expression-derived AggregationDistinctProjections participate in the same rewrite path as top-level DISTINCT aggregations. Specifically, ProjectionsContext#getAggregationDistinctProjections() now also includes DISTINCT projections from expressionDerivedAggregations, allowing both ShardingDistinctProjectionPrefixTokenGenerator and ShardingAggregationDistinctTokenGenerator to process them through the existing rewrite contract instead of relying only on the append-only expression-derived projection path. I've also added regression tests covering: SQL token generation for expression-derived SUM(DISTINCT ...) / AVG(DISTINCT ...) wrapped by IFNULL/COALESCE. |
terrymanu
left a comment
terrymanu
left a comment
There was a problem hiding this comment.
Sorry, I did not make the scope and trade-off clear enough in the previous review rounds. As a result, you kept fixing the issue according to each local comment, but each fix exposed the next problem in the same DISTINCT support path. That was not a good review experience, so I want to clarify the full context and the recommended direction now.
The original issue #37644 is about non-DISTINCT aggregation expressions, for example:
IFNULL(SUM(CASE ...), 0)The core root cause is that when an aggregation function is wrapped by IFNULL / COALESCE, it was not correctly extracted, rewritten, and merged as a sharding merge aggregation. Therefore, the cross-shard result could be incorrect.
The main direction of this PR is correct: extract aggregations from expressions, append derived columns, and evaluate the wrapper expression after merge. For the original non-DISTINCT scenario in #37644, this direction can address the core problem.
However, during the review, we also touched a broader capability: IFNULL / COALESCE wrapping SUM(DISTINCT ...) or AVG(DISTINCT ...). This is no longer just the minimum fix for the original SQL in #37644; it is part of the broader expression-wrapped aggregation support.
The DISTINCT issue has been exposed in several stages:
- Initially, expression-derived
DISTINCTaggregation was merged as a normal aggregation, so DISTINCT semantics were lost. - Then the type issue was fixed, so it remained an
AggregationDistinctProjection. - Then we found that it did not enter the existing DISTINCT rewrite path.
- The latest change exposes expression-derived distinct projections through
getAggregationDistinctProjections(). - The remaining issue is that the top-level DISTINCT rewrite token cannot be directly reused inside a nested wrapper expression.
For a top-level projection, rewriting:
SUM(DISTINCT price)to:
price AS aliascan be valid.
But inside a nested expression, for example:
COALESCE(SUM(DISTINCT price), 0)if only the inner SUM(DISTINCT price) segment is replaced with alias-bearing text, the result can become:
COALESCE(price AS EXPR_DERIVED_0, 0)which is not valid SQL.
Therefore, I do not think we should keep fixing DISTINCT through small local patches. Since this PR has already started handling expression-derived DISTINCT aggregation, and users may reasonably expect SUM(DISTINCT ...) / AVG(DISTINCT ...) to work when we say IFNULL / COALESCE aggregation expressions are supported, I think the better direction is to continue supporting it, but first clarify the rewrite design.
In other words, I suggest choosing Option B: continue supporting expression-wrapped DISTINCT aggregation, but implement an expression-aware DISTINCT rewrite design instead of directly reusing the top-level DISTINCT aggregation token replacement logic.
Concretely, the fix should satisfy the following requirements:
- the final rewritten SQL must remain syntactically valid;
- it should still expose the raw distinct value column required by the distinct merge unit;
- it must not generate alias-bearing text such as
price AS aliasinside the wrapper expression; - tests should verify the final rewritten SQL string through the actual rewrite pipeline;
- tests should not only assert individual token text.
Please add final SQL rewrite tests for scenarios such as:
COALESCE(SUM(DISTINCT price), 0)
IFNULL(SUM(DISTINCT price), 0)
COALESCE(AVG(DISTINCT price), 0)and verify that the final SQL both provides the raw values required by distinct merge and preserves valid IFNULL / COALESCE wrapper expression syntax.
In summary, the main direction of this PR is correct, but DISTINCT wrapper support should not continue through local patches based on the top-level DISTINCT rewrite path. I suggest clarifying an expression-aware DISTINCT rewrite design first, then continuing the implementation.
|
@terrymanu Thanks for the detailed explanation and for clarifying the overall design direction. I understand the distinction now between the original scope of #37644 (expression-wrapped non-DISTINCT aggregations) and the broader problem of expression-wrapped DISTINCT aggregations. Based on your feedback, I've removed the partial DISTINCT support that attempted to reuse the existing top-level DISTINCT rewrite path, since that approach cannot correctly rewrite nested expressions and may generate invalid SQL. The current changes are focused on the original non-DISTINCT use case by extracting aggregations from wrapper expressions, appending the required derived projections, and evaluating the wrapper expression after the merge phase. I agree that supporting IFNULL/COALESCE with SUM(DISTINCT ...) or AVG(DISTINCT ...) requires an expression-aware DISTINCT rewrite design rather than incremental fixes on the existing token replacement logic, so I'd prefer to address that separately once the rewrite strategy is clearly defined. |
terrymanu
left a comment
terrymanu
left a comment
There was a problem hiding this comment.
### Summary
**Review Result: Not Mergeable**
**Feedback Mode: Change Request**
**Reason:** The non-DISTINCT `IFNULL/COALESCE + aggregation` direction is correct for #37644, but the current code still leaves DISTINCT wrapper support active even though the PR discussion says the scope was narrowed back to non-DISTINCT only. There is also a concrete memory-merge column mapping bug for duplicated expression projections.
### Issues
1. P1: DISTINCT wrapper support is still active, but this PR does not implement it correctly
The latest comment says this PR is now focused on the original non-DISTINCT case, such as:
\```sql
IFNULL(SUM(price), 0)
COALESCE(SUM(price), 0)
\```
However, the code still extracts DISTINCT aggregations from expression wrappers.
In `ProjectionEngine#createDerivedAggregationProjection`, `AggregationDistinctProjectionSegment` is still handled as an expression-derived aggregation. This means SQL such as the following is still treated as supported by this PR:
\```sql
IFNULL(AVG(DISTINCT price), 0)
COALESCE(SUM(DISTINCT price), 0)
\```
The test `assertCreateProjectionWithDistinctAggregationInExpression` also explicitly verifies that `IFNULL(AVG(DISTINCT price), 0)` produces expression-derived aggregation metadata.
This is not just “extra support”. DISTINCT wrapper support needs a complete expression-aware rewrite and merge design. The current implementation appends the raw distinct inner expression through `ShardingProjectionsTokenGenerator#getDerivedProjectionText`, but `AVG(DISTINCT price)` and `SUM(DISTINCT price)` cannot be safely handled by this partial path.
Please choose one clear scope:
- If this PR only fixes #37644’s non-DISTINCT case, please remove expression-wrapped DISTINCT support completely:
- Do not extract `AggregationDistinctProjectionSegment` from `IFNULL/COALESCE` expressions.
- Remove `assertCreateProjectionWithDistinctAggregationInExpression`.
- Ensure `ShardingProjectionsTokenGenerator` cannot append expression-derived DISTINCT projections.
- Keep the release note wording scoped to non-DISTINCT aggregation wrappers.
- If this PR still wants to support DISTINCT wrappers, please implement the full design first:
- The final rewritten SQL must remain valid.
- The wrapper expression must not contain alias-bearing text such as `price AS alias`.
- The raw value required by DISTINCT merge must be exposed correctly.
- Please add final SQL rewrite tests and merge-result tests for `SUM(DISTINCT ...)` and `AVG(DISTINCT ...)`.
2. P2: Memory merge maps duplicate expression projections to the wrong output column
`GroupByMemoryMergedResult#setExpressionValueToMemoryRow` locates the output column by comparing expression text:
\```java
if (expandProjections.get(i).getExpression().equalsIgnoreCase(exprProj.getExpression())) {
index = i;
break;
}
\```
This fails when the SELECT list contains the same expression more than once with different aliases, for example:
\```sql
SELECT
IFNULL(SUM(price), 0) AS amount1,
IFNULL(SUM(price), 0) AS amount2
FROM t_order
\```
Both `ExpressionProjection` instances have the same expression text, so memory merge maps both of them to the first matching output column. The second output column may keep the original shard value instead of the merged value.
The stream merge path already avoids this by using the actual `ExpressionProjection` object position:
\```java
expandProjections.indexOf(each) + 1
\```
Please make the memory merge path use the actual projection instance as well, or another alias/object-safe mapping. Also add a regression test with two identical `IFNULL(SUM(...), 0)` expressions using different aliases, where the shard value and merged value differ, and verify both output columns are updated correctly.
### Conclusion
The main non-DISTINCT fix direction is aligned with the issue, but the PR still contains a partial DISTINCT wrapper path and a concrete memory-merge mapping bug. Please fix these two points before the PR can be considered mergeable.
|
@terrymanu Thanks for the detailed review. I've addressed both points in this revision. For P1, I narrowed the scope of the PR to the original non-DISTINCT use case in #37644. Expression-wrapped For P2, I updated the memory merge path to use the actual |
terrymanu
left a comment
terrymanu
left a comment
There was a problem hiding this comment.
Summary
Review Result: Not Mergeable
Feedback Mode: Change Request
Reason: Thanks for the latest update. The two issues from the previous review look addressed: this PR has removed the expression-wrapped DISTINCT aggregation path, and duplicate expression projections are now mapped by the actual ExpressionProjection instance instead of matching by expression text.
I also want to clarify one thing first: the remaining issue below was not introduced by your latest commit. This is an empty-result boundary case that I should have caught in the previous review round when checking the non-DISTINCT merge path. Sorry for the extra round here.
Issues
- P1: Empty non-grouped wrapper aggregations are still not evaluated correctly
Problem: For SQL like:
SELECT IFNULL(SUM(price), 0) FROM t_orderwhen all routed result sets are empty and the SQL has no GROUP BY, SQL aggregation semantics should still return one row, and IFNULL(SUM(price), 0) should evaluate to 0.
The current flow does not cover this boundary yet. ProjectionsContext#getAggregationProjections() adds expression-derived aggregations to the aggregation list, so this SQL enters the group-by merge path. However, in GroupByMemoryMergedResult, setExpressionValueToMemoryRow(...) only iterates existing dataMap rows. When dataMap is empty, wrapper expression evaluation is skipped. After that, getMemoryResultSetRows(...) creates the synthetic aggregate row through generateReturnData(...), but that synthetic row currently only initializes top-level COUNT projections and does not re-evaluate IFNULL / COALESCE expression projections. As a result, the visible expression column remains null.
Relevant paths:
infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/segment/select/projection/ProjectionsContext.java:150features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/merge/dql/groupby/GroupByMemoryMergedResult.java:74features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/merge/dql/groupby/GroupByMemoryMergedResult.java:145features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/merge/dql/groupby/GroupByMemoryMergedResult.java:161
Impact: The original issue is about evaluating IFNULL / COALESCE based on merged aggregation results. This empty-result boundary still returns null instead of the wrapper fallback value, so the non-DISTINCT support is not complete yet.
Required Change: Please make the synthetic empty aggregate row go through the same wrapper expression evaluation path, or otherwise generate the correct expression value for this no-row / no-GROUP BY aggregation case. Please also add a regression test covering empty routed result sets with a projection such as IFNULL(SUM(price), 0) or COALESCE(SUM(price), 0), and verify that the fallback value is returned.
Positive Feedback
The latest commit correctly narrows the PR back to the non-DISTINCT use case and fixes the duplicate-expression column mapping issue from the previous review. The remaining issue is an empty-result boundary in the same non-DISTINCT merge path, not a new scope expansion.
|
@terrymanu I have addressed P1 in the latest revision:
|
terrymanu
left a comment
terrymanu
left a comment
There was a problem hiding this comment.
Summary
Review Result: Not Mergeable
Feedback Mode: Change Request
Reason: The feature direction is still aligned with #37644, but this PR touches the binder projection model, SQL rewrite, and group-by merge path, which are core execution components. Because of that, I need to apply a stricter design bar here. Please understand that this is to protect shared execution behavior, not to block the feature itself.
Issues
P1: The new expression evaluator is not a clear design boundary
Problem: LightweightExpressionEvaluator is introduced as a public production type under the group-by merge package, but its name and contract are too vague for core merge code.
The class name says Lightweight, but that is not a ShardingSphere domain concept and does not explain the supported behavior. The class Javadoc says it is an evaluator “for post-merge aggregations” (features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/merge/dql/groupby/LightweightExpressionEvaluator.java#L31-L34), but the class does not actually evaluate aggregations. Aggregation values have already been produced by the merge units; this helper only reevaluates a small set of wrapper projection expressions, currently IFNULL and COALESCE, using merged aggregation columns.
The public evaluate(...) methods also look generic (LightweightExpressionEvaluator.java#L43-L55), while the implementation silently returns null for unsupported expression/function shapes (LightweightExpressionEvaluator.java#L58-L68, #L71-L82). The callers then skip setting the output cell when the result is null (GroupByMemoryMergedResult.java#L194-L204, GroupByStreamMergedResult.java#L164-L175). That makes unsupported or accidentally misclassified expressions look like a normal nullable result instead of an explicit unsupported path.
Impact: This reads like a tactical patch rather than a stable merge-layer design. Future reviewers and contributors may reasonably assume this is a general post-merge expression evaluator and extend it locally, which would spread SQL expression semantics into the group-by merge package without a clear contract.
Required Change: Please make the abstraction explicitly scoped to the behavior this PR supports. I am not asking for a full expression-evaluation framework. A minimal fix is enough: rename the type and Javadocs to describe “supported aggregation wrapper projection evaluation” rather than “lightweight expression evaluation,” keep the supported function set explicit, and make unsupported paths explicit instead of silently looking like valid NULL results.
P1: Expression-derived aggregation metadata is carried through hidden mutable side state
Problem: ProjectionEngine now stores expressionDerivedAggregations as mutable engine state (infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/segment/select/projection/engine/ProjectionEngine.java#L66-L69). createProjection(ExpressionProjectionSegment) mutates that side map while returning only the ExpressionProjection (ProjectionEngine.java#L126-L149), and ProjectionsContextEngine later copies the side map into ProjectionsContext (infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/segment/select/projection/engine/ProjectionsContextEngine.java#L48-L68).
That means projection creation is no longer self-contained: the caller must know that creating one projection also updates hidden engine-level state. It also makes ProjectionsContextEngine#createProjectionsContext(...) rely on state retained by its ProjectionEngine field. Even if current production construction usually creates a fresh engine per statement, the public engine method itself does not express a one-shot contract.
Impact: This is risky in shared binder code because stale or cross-call expression-derived aggregation metadata would be very hard to diagnose. The changed path is not isolated to this feature; ProjectionsContext and aggregation projection discovery are shared by rewrite, merge, pagination, and other select-statement behavior.
Required Change: Please keep the state local to one createProjectionsContext(...) invocation, or otherwise make the lifecycle explicit and reset-safe. This does not need a broad redesign. The goal is simply to avoid hidden cross-call mutable state for expression-derived aggregation metadata.
P2: Some implementation details still look generated rather than intentional
Problem: A few small code-quality details make the core-path change harder to trust:
LightweightExpressionEvaluator#evaluateFunction(...)copiesfunctionSegment.getParameters()into a newArrayListonly to iterate it (LightweightExpressionEvaluator.java#L71-L80). There is no mutation or snapshot reason for this allocation.ProjectionEngineuses an inline fully-qualifiednew java.util.LinkedHashMap<>()for the new field (ProjectionEngine.java#L68-L69) instead of a normal import, which goes against the repository coding standard for avoiding inline fully-qualified class names.- The evaluator returns plain
nullfor every unsupported path, which makes control flow compact but hides the distinction between “the SQL expression evaluated to NULL” and “this evaluator does not support the expression shape.”
Impact: These are not large architectural problems by themselves, but this PR is modifying core binder/rewrite/merge behavior. Small generated-looking shortcuts in that area make the design harder to maintain and review safely.
Required Change: Please remove the unnecessary collection copy, use normal imports instead of inline fully-qualified class names, and make unsupported evaluator behavior explicit as part of the scoped evaluator contract. Please keep the fix narrow; do not introduce a general-purpose SQL expression engine in this PR.
Review Details
- Review Focus: Code Correctness Review. CI not reviewed by request.
- Reviewed Scope: Latest PR code around
ProjectionEngine,ProjectionsContextEngine,ProjectionsContext,ShardingProjectionsTokenGenerator,GroupByMemoryMergedResult,GroupByStreamMergedResult, andLightweightExpressionEvaluator. - Not Reviewed Scope: GitHub Actions/check-runs/logs and full E2E validation.
- Verification: This review is based on static code-path review of the latest PR head and the public changed files. The requested changes are design and maintainability corrections in the current implementation, not requests for broader feature scope.
- Release Note / User Docs: No additional docs are requested here. The release note can remain scoped to the supported non-DISTINCT
IFNULL/COALESCEaggregation-wrapper capability after the design issues above are fixed.
|
@terrymanu Thanks for the detailed review. I've updated the implementation to address the concerns:
|
terrymanu
left a comment
terrymanu
left a comment
There was a problem hiding this comment.
Summary
Review Result: Not Mergeable
Feedback Mode: Change Request
Reason: Latest-head analysis found five correctness blockers and three required quality/completeness changes. This result is code-scope only; CI not reviewed by request.
Issues
P1: Derived aggregations bypass route and parameter rewriting
Problem: ShardingProjectionsTokenGenerator.java:106-123 appends AggregationProjection#getExpression() as raw SQL. ProjectionsToken.java:46-52 emits it verbatim, while RouteSQLRewriteEngine.java:155-169 retains the original parameter list.
Impact: IFNULL(SUM(CASE WHEN status = ? THEN amount ELSE 0 END), 0) gains another ? without another bound value. Qualified expressions such as SUM(t_order.amount) can also retain the logical owner after routing.
Required Change: Generate derived aggregations through the normal route-aware SQL/parameter rewrite path. Add prepared-CASE and qualified-owner multi-route tests.
P1: Wrapper eligibility is broader than evaluator capability
Problem: ProjectionEngine.java:124-147 registers a wrapper whenever any supported aggregation is found. AggregationWrapperExpressionEvaluator.java:57-81 then throws for parameter markers, columns, casts, binary expressions, and other legal operands.
Impact: Valid SQL such as IFNULL(SUM(price), ?) succeeds when SUM is non-null but throws IllegalArgumentException when the fallback is evaluated. A reviewer-added exact-head regression reproduced this failure.
Required Change: Make registration an all-or-nothing whole-tree capability check, or support every admitted operand with its bound/runtime value. Cover null and non-null parameter fallbacks plus mixed DISTINCT/window operands.
P1: Scalar-subquery metadata leaks into the outer query
Problem: ProjectionEngine.java:91-107 passes the outer aggregation map while recursively binding a scalar subquery. That conflicts with the separately owned subquery contexts created by SelectStatementBaseContext.java:187-195.
Impact: An inner SUM(t2.price) can be appended at the outer projection boundary, where t2 is not visible, producing invalid routed SQL or evaluating against the wrong query level.
Required Change: Keep expression-derived aggregation metadata inside the child SelectStatementContext. Add a scalar-subquery binder and final-rewrite regression proving the outer map remains empty.
P1: Empty COUNT wrappers return the fallback instead of zero
Problem: The synthetic empty-row path in GroupByMemoryMergedResult.java:145-176 initializes zero only when the visible projection is an AggregationProjection. For IFNULL(COUNT(*), 99), the visible projection is an expression, so its derived COUNT cell remains null.
Impact: IFNULL(COUNT(*), 99) returns 99 on an empty/no-route result, although COUNT must return 0. The exact-head regression produced expected 0, actual 99. See MySQL aggregate semantics.
Required Change: Initialize expression-derived COUNT cells by their actual result-set indices. Add empty-result IFNULL and COALESCE COUNT tests with nonzero fallbacks.
P1: Re-evaluation loses SQL result-type coercion
Problem: AggregationWrapperExpressionEvaluator.java:57-87 returns either the raw merged aggregation object or the parser’s literal object. Both merge implementations overwrite the backend-coerced wrapper value with that object.
Impact: IFNULL(SUM(decimal_column), 0) can expose BigDecimal when SUM wins and Integer when the fallback wins, despite one resolved SQL/JDBC column type. ShardingSphereResultSet#getObject returns this raw merged value. Existing tests compare only toString(), masking the mismatch. See MySQL IFNULL type rules and PostgreSQL COALESCE type resolution.
Required Change: Preserve or apply the database/result-metadata coercion for both branches. Assert exact returned classes and metadata in memory and stream regressions.
P2: Required root-path, stream, and documentation coverage is missing
Problem: The linked maintainer acceptance plan requests the two-shard first-empty/second-populated scenario, integration coverage, and merge documentation. The PR adds memory tests only; the modified stream path has no IFNULL/COALESCE test, and neither merge documentation file changes.
Impact: Parser → binder → routed rewrite → index mapping → stream/memory merge is not protected. The tests also omit the issue’s order-sensitive SUM scenario while COUNT remains correct.
Required Change: Add the exact two-shard integration regression, direct stream tests, and update merge.en.md/merge.cn.md with the supported operand boundary. Narrow the broad RELEASE-NOTES.md:75 claim if some legal forms remain unsupported.
P2: New Stream API use violates the high-frequency contract
Problem: ShardingProjectionsTokenGenerator.java:57,111-113 adds a Stream pipeline inside an @HighFrequencyInvocation class. CODE_OF_CONDUCT.md:76-79 explicitly forbids this.
Required Change: Replace the newly added stream/collect with ordinary iteration. The pre-existing stream elsewhere in the class does not justify adding another violation.
P2: Projection collections are copied without ownership or mutation need
Problem: GroupByMemoryMergedResult.java:163-175,186-203 copies expanded projections twice, including once per evaluated group, and indexes through a LinkedList. GroupByStreamMergedResult.java:65-70 makes another unmodified copy.
Impact: The memory path adds avoidable per-group allocation, and the synthetic-row indexed LinkedList scan is quadratic in projection count.
Required Change: Use the existing expanded-projection list directly or precompute the required expression-to-index mapping once.
Multi-Round Comparison
The latest head correctly resolves earlier requests around evaluator naming, explicit unsupported failures, invocation-local expression maps, AVG-derived projections, DISTINCT/window exclusion, duplicate expression mapping, and empty SUM synthesis. The findings above are latest-head issues; no stale prior request has been carried forward.
Review Details
- Review Focus: Code Correctness Review. CI not reviewed by request.
- Reviewed Scope: Exact head
a9ef85f3b50567c9c413dddcc69e3cbfea512d14, merge-base4004670ef7044e7bea76d4d3d29c425d4a8a0b37, all 20 changed files, linked issue requirements, prior review rounds, and direct binder/rewrite/merge/JDBC call paths. - Verification: Selected binder, OpenGauss, PostgreSQL, sharding API, and sharding-core tests passed. Scoped Spotless and Checkstyle checks passed with zero violations.
- Reviewer Regressions: Two temporary exact-head tests produced one failure and one error: empty COUNT returned the fallback, and a parameter fallback threw
IllegalArgumentException. The tests were run only in an isolated temporary clone and were fully reverted. - Workspace Integrity: No PR source file was modified. The shared workspace currently has 158 pre-existing dirty paths, with zero overlap against the PR’s 20 files. The isolated review clone is clean; no commit or push was performed.
- Coverage Audit: 20/20 changed files reviewed; eight independent findings confirmed; the final fresh-context adversarial pass produced zero additional findings.
- Not Reviewed Scope: GitHub Actions status, check runs, and CI logs. CI not reviewed by request.
somiljain2006
changed the title
|
@terrymanu Thanks for the detailed review. I've addressed the reported issues in the latest revision:
|
3 participants
Fixes #37644
Support evaluating IFNULL/COALESCE expression projections after aggregation merge
Add support for evaluating expression projections that wrap aggregation functions, such as IFNULL(SUM(...), 0) and COALESCE(...), after aggregation results are merged. Track derived aggregations for supported expression projections in ProjectionEngine, evaluate them during both memory and stream group-by merging, and limit expression extraction to supported functions to avoid unsupported in-memory SQL evaluation. Also add regression tests covering projection extraction, expression evaluation, and end-to-end group-by merge behavior.
Before committing this PR, I'm sure that I have checked the following options:
./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.