[CALCITE-7669] Uncollect should support the Trino semantics of UNNEST - #5118
Conversation
xuzifu666
left a comment
xuzifu666
left a comment
There was a problem hiding this comment.
I think the overall implementation is fine; I've left some comments for reference.
| ? typeFactory.enforceTypeWithNullability(componentType, true) | ||
| RelDataType elementType = componentType.isStruct() | ||
| ? typeFactory.builder().kind(componentType.getStructKind()) | ||
| .addAll(componentType.getFieldList()).build() |
There was a problem hiding this comment.
The return type here is NOT NULL, so the isNullable mentioned above(line 101) isn't actually used in this branch; it might be worth verifying whether this is appropriate.
In Trino, the output columns of UNNEST(array of nullable row) are nullable.
| @SuppressWarnings({"rawtypes", "unchecked"}) | ||
| private static final Function1<List<Object>, Enumerable<Object>> STRUCT_LIST_AS_ENUMERABLE = | ||
| a0 -> a0 == null ? Linq4j.emptyEnumerable() | ||
| : Linq4j.asEnumerable(a0).select(e -> (Object) ((List) e).toArray()); |
There was a problem hiding this comment.
Arrays containing NULL elements, such as UNNEST(ARRAY[ROW(1,'x'), CAST(NULL AS ROW(...))]), trigger a direct NPE (the SCALAR path avoids this issue as it performs no conversion).
Trino currently produces a row of NULLs for this case.
It is recommended to change the logic to e == null ? null : ... and add a test case that includes NULL elements.
There was a problem hiding this comment.
An attempt to fix this bug has revealed some independent problems in RelToLixTranslator, which need to be fixed fix. I will file a separate issue and mark this as draft until the other one is fixed.
|
Blocked by #5128 |
Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
|
@xuzifu666 I think that #5128 (merged) has fixed the two problems you have identified, so I have rebased this PR and marked it ready for review. |
Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
|
|
You may have forgotten to squash the commits. Would it be possible to force-update the "main" branch? Since there haven't been any new merges yet, it should be safe to do so. |
|
No, I meant this as two separate commits, they have different functions |
|
I noticed the second commit didn't have the [CALCITE-xxxx] prefix, so I thought you might have forgotten it. If you've confirmed it's fine, then it's good to go. |
3 participants
Jira Link
CALCITE-7669
Changes Proposed
There are two separate commits, if independent interest. The first commit only strengthens the validator for rejecting UNNEST on the right side of some JOIN operators.
The semantics of the UNNEST operator changes depending on conformance: for Presto/Trino UNNEST(a) where a is a ROW ARRAY produces a single column; for other conformances it produces as many columns as there are fields in ROW.
This PR makes the choice of semantics explicit at all levels of the representation: the Uncollect operator, and the Enumerable representation by adding a new boolean flag to control the behavior. The flag clearly affects type inference too.
This PR is a stand-alone fragment extracted from #5031