Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CALCITE-6254] Support table function calls in FROM clause without TABLE() wrapper #3693

Merged
merged 1 commit into from
Feb 22, 2024

Conversation

barrkel
Copy link
Contributor

@barrkel barrkel commented Feb 16, 2024

CALCITE-6254 Support table function calls in FROM clause without TABLE wrapping.

Currently, when selecting from a table function, the function call needs to be wrapped in TABLE() like this:

    SELECT * FROM TABLE(table_func('args'));

This PR enables this syntax:

    SELECT * FROM table_func('args');

Three tokens of lookahead are necessary to avoid a conflict in the grammar with the dynamic columns feature used by Phoenix

    SELECT * FROM EventLog(lastGCTime TIME)

Three tokens of lookahead means looking past the ( and identifier to the presence of a type production. The table extension clause for dynamic columns requires at least one field, so the empty case of () is not ambiguous either.

@JiajunBernoulli
Copy link
Contributor

  1. CI should be successful.
  2. Commit message should start with [CALCITE-6254]
  3. Maybe we need validate foo table and foo function(Discussion in JIRA).

@barrkel barrkel changed the title CALCITE-6254 Support table function calls in FROM clause without TABLE() wrapper. [CALCITE-6254] Support table function calls in FROM clause without TABLE() wrapper. Feb 20, 2024
@barrkel barrkel force-pushed the support-bare-table-function-call branch from 8c95a09 to b7cebdd Compare February 20, 2024 13:00
@barrkel
Copy link
Contributor Author

barrkel commented Feb 20, 2024

  1. CI should be successful.

Ack.

2. Commit message should start with `[CALCITE-6254]`

Done. I think the CI failure was because of a . at the end of the commit message.

3. Maybe we need validate foo table and foo function(Discussion in JIRA).

I don't think that makes sense. The identifier scopes for tables and functions are still disjoint, so the existing validation logic in SqlValidatorImpl still applies. The updated grammar produces the same SQL AST as TABLE(foo()). This is why there are no changes outside the grammar and its test.

If the goal is upgraded to being able to handle ambiguous identifiers - e.g. supporting nullary table functions - then things get much harder, both for design and implementation. As I mentioned on the JIRA ticket I think that's a dubious cost/benefit tradeoff.

Copy link

sonarcloud bot commented Feb 20, 2024

Copy link
Contributor

@macroguo-ghy macroguo-ghy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@barrkel barrkel changed the title [CALCITE-6254] Support table function calls in FROM clause without TABLE() wrapper. [CALCITE-6254] Support table function calls in FROM clause without TABLE() wrapper Feb 21, 2024
@macroguo-ghy macroguo-ghy added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Feb 22, 2024
@macroguo-ghy macroguo-ghy merged commit 331f985 into apache:main Feb 22, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM-will-merge-soon Overall PR looks OK. Only minor things left.
Projects
None yet
3 participants