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

Process function path #11476

Merged
merged 5 commits into from
Apr 22, 2022
Merged

Process function path #11476

merged 5 commits into from
Apr 22, 2022

Conversation

dain
Copy link
Member

@dain dain commented Mar 14, 2022

Description

  • Process function path session.
  • Add session to function metadata calls, so funciton path is available.

Related issues, pull requests, and links

Relates to #8

Documentation

There are no actual function namespaces yet, so this does not change anything user visible yet.

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

There are no actual function namespaces yet, so this does not change anything user visible yet.

(x) No release notes entries required.
() Release notes entries required with the following suggested text:

@findepi
Copy link
Member

findepi commented Mar 14, 2022

cc @kasiafi


private Collection<FunctionMetadata> getFunctions(CatalogSchemaFunctionName name)
{
if (name.getCatalogName().equals(GLOBAL_CATALOG)) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we prevent creating a catalog with this name?

Copy link
Member Author

Choose a reason for hiding this comment

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

The "system" catalog is the built in catalog that is information like the nodes in the cluster, and metadata for the jdbc driver.

Comment on lines 208 to 210
String catalog = sqlPathElement.getCatalog().map(Identifier::getValue).or(session::getCatalog)
.orElseThrow(() -> new IllegalArgumentException("Session default catalog must be set to resolve a partial function name: " + null));
names.add(new CatalogSchemaFunctionName(catalog, sqlPathElement.getSchema().getValue(), functionName));
Copy link
Member

Choose a reason for hiding this comment

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

Could you use getCanonicalValue() instead of getValue()?
I think it's no difference at the moment, as all the names are eventually lowercased, but it would simplify the future transition to SQL identifier semantics.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. Didn't know that method existed.

@dain dain force-pushed the function-refactor-path branch from 6b7d00a to b09cfbc Compare March 21, 2022 05:07
@dain dain force-pushed the function-refactor-path branch from b09cfbc to bb44aeb Compare April 21, 2022 00:53
@dain dain force-pushed the function-refactor-path branch from bb44aeb to 461d3ca Compare April 21, 2022 21:23
@findepi
Copy link
Member

findepi commented Apr 22, 2022

@dain does this PR have any impact on ConnectorExpression representation of function calls, as presented to a connector?

cc @assaf2 @wendigo @hashhar

@findepi
Copy link
Member

findepi commented Apr 22, 2022

Does this conflict anyhow with #9831?

@dain
Copy link
Member Author

dain commented Apr 22, 2022

@dain does this PR have any impact on ConnectorExpression representation of function calls, as presented to a connector?

At this point, I don't think so, but in the future maybe. Functions in SQL have a absolute name expressed just like a table with catalog and schema. We will eventually want to express full names to connectors so they can differentiate between functions in different schemas. That said, there is only one schema that has functions right now.

Does this conflict anyhow with #9831?

Yes, any pending functions will need to be updated to use the new builders. This translation is easy, so it shouldn't be too much work.

@dain dain merged commit 2d78bc9 into trinodb:master Apr 22, 2022
@dain dain deleted the function-refactor-path branch April 22, 2022 18:40
@github-actions github-actions bot added this to the 379 milestone Apr 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants