[CALCITE-7608] Enhance Uncollect

[mihaibudiu]

Jira Link

CALCITE-7608

Changes Proposed

This PR has several improvements to the Uncollect operator:

• support LEFT JOIN UNNEST
• support Trino semantics (no structure expansion)
• support for carrying over other input fields

The first two changes are actually bug-fixes which could be applied independently to the original Uncollect operator. Only the last modification is a true change of the operator's semantics, and it is strictly backwards-compatible: if the set of carried input fields is empty this operator behaves like the old version of Uncollect.

[iwanttobepowerful]

We believe this PR is quite important, so I’d really appreciate it if you could take another look when you have a moment. @zabetak @julianhyde

[iwanttobepowerful]

how about add order by a

[mihaibudiu]

to make the output deterministic?

[iwanttobepowerful]

yes

[iwanttobepowerful]

Generating LogicalSelectMany directly within SqlToRelConverter would feel cleaner and more natural, I think. I’m fully on board with deprecating Uncollect in favor of this new operator.
Also, would it be reasonable to rename LogicalSelectMany to LogicalUnnest? The current name feels a bit too LINQ-specific for a core Calcite relational operator.

[iwanttobepowerful]

Thanks a lot for introducing SelectMany — this is a much cleaner abstraction than the current Correlate + Uncollect pattern.

You note in the description that "in the future this rewrite could be moved into SqlToRelConverter as well." I'd like to make the case for doing (at least an initial version of) that in this PR, because I think the rule-based approach has an inherent limitation:

A pattern-matching rule only fires when the plan matches the exact shape it expects, and that shape is easily perturbed — by an intervening Project or pushed-down Filter, a trait change, or decorrelation running first. When that happens, a perfectly convertible query silently misses the rewrite and falls back to the less efficient Correlate form, and trying to generalize the pattern to cover every shape tends to become an endless game of whack-a-mole.

Generating LogicalSelectMany directly in convertUnnest() sidesteps this. The converter already has everything it needs — the input row, the array expressions, withOrdinality, and the join type. By projecting both the pass-through columns and the collection columns onto the same input row, the array expressions stay as ordinary input references (no correlation variable is ever introduced), and SelectMany can be emitted deterministically — before any optimization can disturb the shape. This makes FROM t CROSS/LEFT JOIN UNNEST(t.arr) skip both the Correlate intermediate form and the subsequent decorrelation step entirely, and guarantees the operator is produced whenever it's applicable, rather than "whenever the rule happens to match."

The two approaches are complementary, not competing: the rule is still valuable for plans that already contain Correlate + Unnest (e.g. coming from other frontends), while direct generation covers the SQL path. To stay backwards-compatible, the sql2rel path could be gated behind a SqlToRelConverter.Config flag (off by default), mirroring how you disabled the rewrite rule by default.

Happy to help with this if you think it's in scope — otherwise it could make a good follow-up.

[mihaibudiu]

There so many config flags in Calcite...

Thinking of this, having a way to do it in SqlToRelConverter is probably very useful, so we should offer this possibility.
Deprecating Uncollect is a big move, it may cause problems for other people who depend on it.
We should probably give it some time.

SelectMany is actually not a very good name, the "real" selectMany is actually much more general, it takes an arbitrary function which returns an iterator. We could call it LogicalUnnest.

Let's see if there are other comments which I can address in an updated commit. I will try to add it to SqlToRel, but not sure I will get to it during the week-end. Happy to get other naming suggestions as well.

[mihaibudiu]

Converted to draft pending discussion in Jira.

[mihaibudiu]

I have completely reworked this PR to enhance Uncollect to support passing through fields.
There is now a single commit, with changes to all relevant classes.

The only thing I have not yet done is to build this new form of the operator directly in SqlToRelConverter. We can do that in a separate PR, this one is big enough.

[mihaibudiu]

This will need a bit more work, I found out that Presto/Trino actually give a different semantics to Uncollect (do not expand struct fields), so that needs to be modeled as well.

[snuyanzin]

I played locally and seems found a case where current implementation fails

    final String sql = "with t1 as (select array['a', 'b'] as sa, array[1, 2] as ia)\n"
        + "select u.x, u.y\n"
        + "from t1, unnest(t1.ia, t1.sa) as u(x, y)";
    sql(sql)
        .withPreRule(CoreRules.PROJECT_REMOVE)
        .withRule(CoreRules.CORRELATE_UNCOLLECT_MERGE)
        .check();

currently fails as

    java.lang.AssertionError: Cannot add expression of different type to set:
    set type is RecordType(INTEGER X, CHAR(1) Y) NOT NULL
    expression type is RecordType(CHAR(1) SA, INTEGER IA) NOT NULL
    set is rel#302523:LogicalProject.(input=HepRelVertex#302522,exprs=[$2, $3])
    expression is Uncollect
      LogicalProject(SA=[ARRAY('a', 'b')], IA=[ARRAY(1, 2)])
        LogicalValues(tuples=[[{ 0 }]])
    Type mismatch:
    rowtype of original rel: RecordType(INTEGER X, CHAR(1) Y) NOT NULL
    rowtype of new rel: RecordType(CHAR(1) SA, INTEGER IA) NOT NULL
    Difference:
    X: INTEGER -> CHAR(1)
    Y: CHAR(1) -> INTEGER

[mihaibudiu]

I will add this as a test, thank you.

[asolimando]

This will need a bit more work, I found out that Presto/Trino actually give a different semantics to Uncollect (do not expand struct fields), so that needs to be modeled as well.

@mihaibudiu I have been following the discussion with lots of interest but I missed this comment, can you elaborate a bit more on the different semantics you found in Trino?

[mihaibudiu]

@asolimando UNNEST(a) of an array of structs will create as many columns as there fields in the struct. But apparently Trino's semantics requires that only one column is created, of struct type. So I fixed UNNEST to support both semantics, as a function of the conformance.

[mihaibudiu]

I apologize for the large PR, but I thought that changing the semantics of a core logical operator should be done ideally in a single change rather than several smaller changes. This PR has several improvements to UNNEST:

• support LEFT JOIN UNNEST
• support Trino semantics (no structure expansion)
• support for carrying over other input fields

The first two changes are actually bug-fixes which could be applied independently to the original UNNEST operator. Only the last modification is a true change of the operator's semantics.

[sonarqubecloud]

Quality Gate passed

Issues
14 New issues
0 Accepted issues

Measures
0 Security Hotspots
73.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud