Skip to content

Avoid getFunctionMetadata for connector provided functions#17661

Closed
weijiii wants to merge 1 commit intotrinodb:masterfrom
weijiii:func-metadata-on-worker
Closed

Avoid getFunctionMetadata for connector provided functions#17661
weijiii wants to merge 1 commit intotrinodb:masterfrom
weijiii:func-metadata-on-worker

Conversation

@weijiii
Copy link
Copy Markdown
Member

@weijiii weijiii commented May 27, 2023

I am trying to use SQL path in production and have bumped into some issues. At the moment we only need to support scalar functions via this way so I did not look into the parts where getAggregationFunctionMetadata is used. The changes in this PR may not align with the community's plan to resolve them but I would like to collect some thoughts so that we can move along with it internally for the short term. Thanks.

Description

  • Using catalog provided functions with SQL paths in the session causes query fail on worker node on code paths involving operations not allowed on workers
  • deterministic is included in ResolvedFunction but not used in ExpressionOptimizer and ExpressionInterpreter. Instead we make other method calls to get function metadata.
  • Similar to deterministic I think we can carry deprecated in ResolvedFunction, and description only when the function is deprecated, so for most cases the size of the corresponding QualifiedName object would not increase much.
  • Related to Make worker not fail on getOptionalCatalogMetadata during coercion resolution #17644

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label May 27, 2023
@weijiii weijiii changed the title [WIP] Avoid getFunctionMetadata for catalog provided functions Avoid getFunctionMetadata for catalog provided functions May 27, 2023
@weijiii weijiii marked this pull request as ready for review May 28, 2023 07:28
@weijiii weijiii marked this pull request as draft May 28, 2023 18:27
@weijiii weijiii changed the title Avoid getFunctionMetadata for catalog provided functions Avoid getFunctionMetadata for connector provided functions May 29, 2023
@hashhar
Copy link
Copy Markdown
Member

hashhar commented May 30, 2023

cc: @dain @martint

@weijiii weijiii marked this pull request as ready for review June 6, 2023 21:47
@raunaqmorarka raunaqmorarka requested review from dain and martint June 11, 2023 06:50
@raunaqmorarka
Copy link
Copy Markdown
Member

@weijiii please rebase this one to master

@weijiii weijiii force-pushed the func-metadata-on-worker branch from 6746c32 to 00e4e90 Compare June 11, 2023 07:44
@weijiii weijiii requested a review from raunaqmorarka June 11, 2023 09:10
@weijiii
Copy link
Copy Markdown
Member Author

weijiii commented Jun 11, 2023

@raunaqmorarka Rebased. Thanks.

@weijiii
Copy link
Copy Markdown
Member Author

weijiii commented Jun 17, 2023

Sync'ed offline with Martin; deprecation and description is not in the purpose of ResolvedFunction; instead we should be able to short-circuit the deprecation check when in worker's context, or better to find why worker is re-analyzing function nodes for types

@dain
Copy link
Copy Markdown
Member

dain commented Aug 19, 2023

@weijiii given the comment above, should this PR be closed?

@dain
Copy link
Copy Markdown
Member

dain commented Aug 25, 2023

@weijiii I created a different patch that I think works around this issue. If it doesn't fix it for you, let me know

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