fix: handle nested arrays in UNNEST by checking for grouping in left query (#17949) - #20094
fix: handle nested arrays in UNNEST by checking for grouping in left query (#17949)#20094zhang-arvin wants to merge 1 commit into
Conversation
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 0 |
| P3 | 0 |
| Total | 1 |
Reviewed 1 of 1 changed files.
This is an automated review by Codex GPT-5.6-Luna(max)
| final RowSignature leftSignature = DruidRels.dataSourceSignature(newLeftDruidRel); | ||
| if (whereFilter == null) { | ||
| if (computeLeftRequiresSubquery(newLeftDruidRel)) { | ||
| if (computeLeftRequiresSubquery(newLeftDruidRel) || updatedLeftQuery.getQuery() instanceof GroupByQuery) { |
There was a problem hiding this comment.
[P1] GroupBy detection is unreachable
This check runs only when the partial-query stage is SCAN, WHERE_FILTER, or SELECT_PROJECT. DruidQuery emits GroupByQuery only when an aggregate exists, which places the stage at AGGREGATE or later. Therefore the new condition cannot change the data-source choice for the reported grouped-left-side case, leaving the fix ineffective.
ce94d2c to
45a1b0c
Compare
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 0 |
| P3 | 0 |
| Total | 1 |
Reviewed 1 of 1 changed files.
The review found one high-confidence P1 correctness issue in the grouped-left UNNEST rewrite.
This is an automated review by Codex GPT-5.6-Luna(max)
| final RowSignature leftSignature = DruidRels.dataSourceSignature(newLeftDruidRel); | ||
| if (whereFilter == null) { | ||
| if (computeLeftRequiresSubquery(newLeftDruidRel)) { | ||
| if (updatedLeftQuery.getQuery() instanceof GroupByQuery) { |
There was a problem hiding this comment.
[P1] GroupByQuery check is too late to preserve grouped rows
The rewrite above can rebuild a grouped left relation from only its scan/project before this check, making updatedLeftQuery a ScanQuery rather than a GroupByQuery. A query that groups an array column and then UNNESTs it can therefore still read raw pre-group rows and return duplicates or incorrect results. Detect and preserve grouping before rebuilding the left query, with a regression test.
Description
Fixes #17949: UNNEST with nested arrays fails when the left side of the correlate contains GROUP BY aggregation.
Root Cause
DruidCorrelateUnnestRel.toDruidQuery()usescomputeLeftRequiresSubquery()to determine whether to wrap the left query in aQueryDataSource. However,computeLeftRequiresSubquery()only checks thePartialDruidQuerystage (SCAN), but when UNNEST is pulled up above a GROUP BY, the left side may contain grouping/aggregation pushed into theDruidQueryitself. In this case,getDataSource()returns the raw table scan instead of a subquery that wraps the aggregation.Fix
Added an additional check in
toDruidQuery(): when the computedupdatedLeftQueryis aGroupByQuery(i.e., contains grouping), wrap it in aQueryDataSourceeven ifcomputeLeftRequiresSubquery()returns false. This ensures the grouping/aggregation logic is preserved in the UNNEST data source.Changes
sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidCorrelateUnnestRel.java: Addedinstanceof GroupByQuerycheck in thetoDruidQuery()method to detect when the left query contains grouping, and useQueryDataSourceaccordingly.