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

Use fully qualified functions in planner and optimizer #19035

Merged
merged 10 commits into from
Sep 16, 2023

Conversation

dain
Copy link
Member

@dain dain commented Sep 14, 2023

Description

This PR changes the planner and optimizer to consistenly handle fully qualified function names. To reduce the burden on this code this PR adds a simplified method to resolve builtin functions. This new function uses a simple string function name, and removes the need for a session, as session was only need for PATH.

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.

@dain dain requested review from electrum and martint September 14, 2023 02:56
@cla-bot cla-bot bot added the cla-signed label Sep 14, 2023
@github-actions github-actions bot added the iceberg Iceberg connector label Sep 14, 2023
@dain dain force-pushed the resolve-functions branch 2 times, most recently from 356ab3c to 452135e Compare September 15, 2023 17:56
return FunctionCallBuilder.resolve(session, metadata)
.setName(QualifiedName.of(nullAllowed ? NullableFunction.NAME : Function.NAME))
return BuiltinFunctionCallBuilder.resolve(metadata)
.setName(nullAllowed ? NullableFunction.NAME : Function.NAME)
Copy link
Member

Choose a reason for hiding this comment

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

We should rename Function to DynamicFilterFunction

@@ -45,7 +69,10 @@ public void testPruneOneChild()
p.values(a, unused),
p.values(b, r),
ImmutableList.of(a, b, r),
expression("ST_Distance(a, b) <= r"));
Copy link
Member

Choose a reason for hiding this comment

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

Can we handle this by adding a rewriteFunctionCall to io.trino.sql.ExpressionUtils#rewriteIdentifiersToSymbolReferences?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is possible, but after talking with Martin he wants to move away from this method. This change sets up the code for the new IR.

dain added 10 commits September 15, 2023 18:07
Use TranslationMap to apply casts, desugar expression, and preform
other rewrites in evaluateConstantExpression
Move CatalogSchemaFunctionName to SPI and add to BoundSignature
Operators and coercions must be global functions
Most uses of resolve function in analyzer and optimizers are hard coded
to lookup builtin functions, and these can use the simpler
resolveBuiltin method that does not need a session.
Split FunctionCallBuilder into BuiltinFunctionCallBuilder for building
new function call nodes for builtin functions, and
ResolvedFunctionCallBuilder for building a function call node for
previously resolved function.
@dain dain force-pushed the resolve-functions branch from 452135e to 503fd2a Compare September 16, 2023 03:51
@dain dain merged commit 846a8e9 into trinodb:master Sep 16, 2023
@dain dain deleted the resolve-functions branch September 16, 2023 18:08
@github-actions github-actions bot added this to the 427 milestone Sep 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed iceberg Iceberg connector
Development

Successfully merging this pull request may close these issues.

2 participants