Skip to content

Use ResolvedFunction#isDeterministic#17664

Merged
raunaqmorarka merged 1 commit intotrinodb:masterfrom
weijiii:use-isdeterministic
Jun 11, 2023
Merged

Use ResolvedFunction#isDeterministic#17664
raunaqmorarka merged 1 commit intotrinodb:masterfrom
weijiii:use-isdeterministic

Conversation

@weijiii
Copy link
Copy Markdown
Member

@weijiii weijiii commented May 28, 2023

Description

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 28, 2023
@weijiii weijiii marked this pull request as ready for review May 28, 2023 19:37
@weijiii weijiii requested review from dain and removed request for dain May 28, 2023 19:48
@raunaqmorarka raunaqmorarka requested a review from martint May 29, 2023 15:00
@weijiii
Copy link
Copy Markdown
Member Author

weijiii commented Jun 6, 2023

@martint As I am trying to manifest the error we faced in a test case, could you please comment if this is a reasonable fix? I did not see why we use the Metadata API to get deterministic information while DeterminismEvaluator uses the ResolvedFunction object to obtain that. For the two cases in this PR in ExpressionOptimizer and ExpressionInterpreter the intermediate FunctionMetadata object is not needed afterwards either. Thanks!

Copy link
Copy Markdown
Member

@raunaqmorarka raunaqmorarka left a comment

Choose a reason for hiding this comment

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

@weijiii can you please rebase this to latest master ?
This lgtm given that the deterministic in ResolvedFunction is also computed from FunctionMetadata

Copy link
Copy Markdown
Member

@martint martint left a comment

Choose a reason for hiding this comment

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

Apologies for the delay. Yes, this fix is good.

@raunaqmorarka raunaqmorarka merged commit 3b4f6a9 into trinodb:master Jun 11, 2023
@github-actions github-actions bot added this to the 420 milestone Jun 11, 2023
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.

3 participants