Skip to content

Simplify expressions passed to table functions #16388

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

Merged
merged 1 commit into from
Jun 15, 2025

Conversation

simonvandel
Copy link
Contributor

@simonvandel simonvandel commented Jun 12, 2025

Which issue does this PR close?

Rationale for this change

Table functions don't need to special case Casts of literals or other constructs that could be simplified.

What changes are included in this PR?

Apply expression simplification to table functions.

Are these changes tested?

Yes, added a sqllogictest

Are there any user-facing changes?

Yes, more queries should be supported.

@github-actions github-actions bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Jun 12, 2025
Copy link
Contributor

@jonathanc-n jonathanc-n left a comment

Choose a reason for hiding this comment

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

We can either support this for now and have a comment to delete it later when we support references to existing columns or not go forward with it.

@@ -1675,6 +1675,13 @@ impl ContextProvider for SessionContextProvider<'_> {
.get(name)
.cloned()
.ok_or_else(|| plan_datafusion_err!("table function '{name}' not found"))?;
let dummy_schema = DFSchema::empty();
Copy link
Contributor

@jonathanc-n jonathanc-n Jun 13, 2025

Choose a reason for hiding this comment

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

I think we eventually want to reference existing columns

For example:
SELECT * FROM t, range(t.id, t.id + 5);

I think the solution to this instead would be to have the a ExprSimplifier optimizer rule where it can reference the column values from the LATERAL statement. @Lordworms WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree long term it would be great to have table functions support referencing other relations.

However, we would need to update the API more than just this -- right now TableFunctions have no way of getting inputs (they return TableProviders)

We would have to work out what the API would look like if they got an input (perhaps they get an input ExecutonPlan)

As you mention, we would also probably have to figure out planning for LATERALs, etc

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @simonvandel and @jonathanc-n

@comphead comphead merged commit 869acf9 into apache:main Jun 15, 2025
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Table function supports non-literal args
4 participants