[CALCITE-7254] Add rule for sharing trivially equivalent RelNodes within Combine

[tjbanghart]

CALCITE-7254

This is a first pass, but I’m not yet convinced it’s the right approach. We may need to gather potential shared sub-expressions in a dedicated context and defer the rewrite until later. At the moment, the suggester identifies candidate spools and performs the replacement directly within the rule, which may unintentionally prevent the optimizer from exploring cheaper plans.

Likewise, certain rules may prevent shareable components from being recognized. For example, filter pushdown can eliminate a join tree that would otherwise be reusable.

[xiedeyantu]

I see that AssertQuery.returnsUnordered(String... lines) can return multiple lines. In JdbcTest, some examples use the format {column_name}={value}, such as returnsUnordered("DID2=1", "DID2=2"). I'm not sure if the current QUERY_0=xxx format is well-suited for future Quidem tests. Assuming that in the future I could use a setting like set enable_combine = true to allow multiple SQL statements to execute simultaneously, would we still want to obtain two separate result sets—just like when the two SQL statements are executed separately?

[tjbanghart]

Good questions, I've updated the PR so Combine now returns one row per input query. Each row holds an array of maps, where each map represents a row from that query with column names as keys.

We are constrained by JDBC's limitations of returning a single result set per query but this would allow iterating over individual query results with resultSet.next() and then iterating over each query's results via the array.

For example, combining:
SELECT name FROM depts;
and
SELECT empid, name, deptno FROM emps WHERE deptno = 10;

Yields:

-- Row 1
QUERY=[
   {name=Sales}, 
   {name=Marketing}, 
   {name=HR}
],
-- Row 2
QUERY=[
   {empid=100, name=Bill, deptno=10}, 
   {empid=150, name=Sebastian, deptno=10} , 
   {empid=110, name=Theodore, deptno=10}
]

Later, we should probably allow for aliases for queries so they don't have the generic QUERY column label but that may be more challenging (see the syntax for MULTI described in CALCITE-6188).

[xiedeyantu]

Thank you for the very detailed explanation. I think the current format of the result presentation looks quite good.

[tjbanghart]

To later accommodate named queries I've changed the format once more. From the comment in EnumerableCombine:

The output format is a wide table where each column corresponds to a query (named QUERY_0, QUERY_1, etc.) and each row contains a struct with that query's column values for that row index. The number of output rows equals the maximum row count across all input queries. Queries with fewer rows have null values for the additional rows.

Example output for two queries:

QUERY_0                  | QUERY_1
------------------------ | ------------------------
{empno=100, name=Bill}   | {deptno=10, name=Sales}
{empno=110, name=Eric}   | {deptno=20, name=HR}
{empno=120, name=Ted}    | null

[xiedeyantu]

I think it looks good overall, just left a few minor comments.

[xiedeyantu]

Can you add a small test for this method?

[xiedeyantu]

Would it be better to use AssertThat for positive validation? I don't understand why the throw new AssertionError approach is used here.

[xiedeyantu]

There are still some minor issues in the CI that need to be fixed.

[xuzifu666]

The @Nullable annotation is required here; otherwise CI will fail.

[tjbanghart]

Awesome, thank you - that was it.

[xiedeyantu]

LGTM. I think we can squash all the commits, and I'll leave the rest of the merging to you.

[tjbanghart]

Thank you so much for the reviews @xiedeyantu, much appreciated!

[sonarqubecloud]

Quality Gate passed

Issues
3 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud