[CALCITE-7819] RelMdTableReferences returns null for TableFunctionScan with relational inputs

[ehds]

Jira Link

CALCITE-7819
RelMetadataQuery#getTableReferences returns null for a plan containing a TableFunctionScan, even when the function has a relational input containing a TableScan. For example, a TUMBLE table function can produce:

    LogicalProject
      LogicalTableFunctionScan
        LogicalProject
          LogicalTableScan(table=[[hr, employee]])

For this plan, mq.getTableReferences(root) returns null instead of a reference to \ [hr, employee].#0. RelMdTableReferences has no handler for TableFunctionScan, so the call falls through to its catch-all RelNode handler.

Changes Proposed

Add a TableFunctionScan handler that collects table references from its relational inputs. It reuses the logic for SetOp, including the handling of repeated references to the same table.

[zzwqqq]

Just to make sure I’m following: are you saying getTableReferences doesn’t handle TableFunctionScan? If so, maybe the PR description could be clearer about the problem you’re trying to solve.

[zzwqqq]

Should we also handle a RexSubQuery inside rexCall of TableFunctionScan

[ehds]

Good catch. SubQueryRemoveRule already handles a RexSubQuery in TableFunctionScan.getCall(). I’ve added the corresponding table-reference handling and tests.

[Dwrite]

LGTM. Reusing the existing getTableReferences(List, RelMetadataQuery) helper keeps the behavior consistent with SetOp and correctly propagates metadata through TableFunctionScan.

[ehds]

Just to make sure I’m following: are you saying getTableReferences doesn’t handle TableFunctionScan? If so, maybe the PR description could be clearer about the problem you’re trying to solve.

Thanks for pointing that out. I’ve updated both the issue description and the implementation of the fix.

[mihaibudiu]

Please squash the commits for merging

[ehds]

Please squash the commits for merging

Done.

[vlsi]

@zzwqqq @mihaibudiu, could we hold the merge until we settle the sub-query question below? I think the relational-input part is ready as is.

Thanks, the relational-input part fixes the case from the Jira: for select * from table(tumble(table emp, descriptor(hiredate), interval '1' minute)) I now get [[CATALOG, SALES, EMP].#0] instead of null. I have concerns about the sub-query part, plus a few smaller comments.

Sub-queries in the call

I'd suggest removing the RexSubQuery handling from this PR and filing a separate Jira for it. The sub-query tables show up only in plans where the sub-query is still in the call, and only for TableFunctionScan:

• Project, Filter, Join, and Calc don't look at the sub-queries in their expressions. With expand=false, select (select max(sal) from emp) from dept and select * from dept where deptno in (select deptno from emp) both return [[CATALOG, SALES, DEPT].#0], with no EMP. DEDUP(CURSOR(dept), (select count(*) from emp)) now returns [DEPT.#0, EMP.#0], so where the sub-query sits changes the answer.
• After CoreRules.TABLE_FUNCTION_SCAN_SCALAR_QUERY_TO_CORRELATE, the same plan becomes Project(Correlate(Aggregate(sub-query), TableFunctionScan)), and getTableReferences returns null, because RelMdTableReferences has no Correlate handler. For the DEDUP plan above I get [DEPT.#0, EMP.#0] before the rule and null after it.

A follow-up could handle Correlate and RexSubQuery the same way in every node. The Jira title covers only relational inputs, and the sub-query change is not mentioned in the Jira, the PR description, or the squashed commit message.

A function scan with no inputs

TABLE(RAMP(3)) used to return null and now returns an empty set, while Values still returns null. RelMdColumnOrigins makes the same choice for a leaf TableFunctionScan, so I'm fine with it, but it should be in the Javadoc: a user-defined table function that reads a table on its own is now reported as reading none.

Javadoc

• Table references from TableFunctionScan. matches the other methods, but the new handler behaves differently from them in two ways that a caller can't guess: it returns an empty set when there are no inputs, and it returns null if the references of any input can't be determined. Please state both, plus the sub-query handling if it stays.
• The helper's comment, Returns table references from relational inputs and sub-queries., doesn't match what the helper does: it takes any list of nodes, and SetOp passes no sub-queries. What a reader needs to know is that repeated references to the same table get distinct entity numbers. Something like: Returns the union of the table references of {@code inputs}, numbering repeated references to the same table apart, or null if the references of any input cannot be determined.

Tests

• Against the code before the fix, …SameTableInputs and …InputAndScalarSubQuery fail with a bare NullPointerException from Sets.newTreeSet(null), so the report doesn't say that null came back. sortsAs(...), which the neighboring tests use, would print it.
• Missing cases, if the sub-query part stays: a sub-query with no relational input (for example RAMP((select count(*) from emp))), a sub-query over an unknown node, which should return null, and a sub-query nested inside an expression rather than passed as a direct operand.
• …InputAndScalarSubQuery passes a scalar sub-query to DEDUP, whose operands are cursors. The plan builds only because RelBuilder doesn't validate operands. RAMP would give a realistic plan.
• A SQL-level test for the TUMBLE case from the Jira would match the shape of the neighboring tests (sql(...).toRel()).
• Minor: in …ZeroInputs, assertThat(tableReferences, empty()) can replace the notNullValue() and hasSize(0) pair.

Description

Nit: there is a stray backslash before `[hr, employee].#0`.

[mihaibudiu]

@ehds please take a look at the last review and try to address the questions

[ehds]

• A SQL-level test for the TUMBLE case from the Jira would match the shape of the neighboring tests (sql(...).toRel()).

@vlsi Thanks for the thorough review. As you pointed out, other operators don’t account for sub-queries in their expressions, and Correlate isn’t handled either, so I think a separate Jira makes sense. I’ve removed the RexSubQuery handling from this PR and updated the Javadocs and tests based on your feedback.

@zzwqqq @mihaibudiu what do you think?

[zzwqqq]

• A SQL-level test for the TUMBLE case from the Jira would match the shape of the neighboring tests (sql(...).toRel()).

@vlsi Thanks for the thorough review. As you pointed out, other operators don’t account for sub-queries in their expressions, and Correlate isn’t handled either, so I think a separate Jira makes sense. I’ve removed the RexSubQuery handling from this PR and updated the Javadocs and tests based on your feedback.

@zzwqqq @mihaibudiu what do you think?

@ehds I think it’s ok to separate the subquery-related issues from this one. Could you please also file a Jira ticket so we don’t miss it？

[ehds]

• A SQL-level test for the TUMBLE case from the Jira would match the shape of the neighboring tests (sql(...).toRel()).

@vlsi Thanks for the thorough review. As you pointed out, other operators don’t account for sub-queries in their expressions, and Correlate isn’t handled either, so I think a separate Jira makes sense. I’ve removed the RexSubQuery handling from this PR and updated the Javadocs and tests based on your feedback.
@zzwqqq @mihaibudiu what do you think?

@ehds I think it’s ok to separate the subquery-related issues from this one. Could you please also file a Jira ticket so we don’t miss it？

I’ve opened a separate Jira issue for the sub-query and Correlate cases: CALCITE-7825.

[vlsi]

Thanks for the updates, my earlier comments are addressed. The sub-query handling moved to CALCITE-7825, the Javadoc states when the handler returns an empty set and when it returns null, and there is now a SQL-level test for the TUMBLE query from the Jira.

I rebased the branch on current main: RelMetadataTest, the materialized view tests, and SqlToRelConverterTest pass. Against the old RelMdTableReferences, three of the four new tests fail with but: was null.

The PR has two commits again, and the second one, Restrict TableFunctionScan table references to relational inputs, has no [CALCITE-7819] prefix. The commits should be squashed into one before the merge, either by @ehds in this PR or by the committer when merging.

[mihaibudiu]

please squash the commits so we can merge

[ehds]

please squash the commits so we can merge

Done.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud