Skip to content

Support parameters in lambda expressions for prepare statement#16184

Merged
highker merged 1 commit intoprestodb:masterfrom
yuanzhanhku:second
Jun 8, 2021
Merged

Support parameters in lambda expressions for prepare statement#16184
highker merged 1 commit intoprestodb:masterfrom
yuanzhanhku:second

Conversation

@yuanzhanhku
Copy link
Contributor

@yuanzhanhku yuanzhanhku commented Jun 1, 2021

fixes #16005

Override visitLambdaExpression in ParameterExtractingVisitor to support
extracting parameters form lambda expressions. This commit does not add the
override function to DefaultTraversalVisitor because there are other
visitors which do not expect to visit lambdas. For example,
VariablesExtractor doesn't want to extract variables from lambdas and
treat it as dependencies from the source plan.

Test plan

  • Added query tests in presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java.
  • Also started a presto server locally and tested the change manually.
== RELEASE NOTES ==

General Changes
* Fix a bug that parameters are not supported in lambda expressions for prepare statements.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 1, 2021

CLA Signed

The committers are authorized under a signed CLA.

  • ✅ Zhan Yuan (522f0c6f19bbe76171e32af3e1622cdb649ddfcb)

Copy link

@highker highker left a comment

Choose a reason for hiding this comment

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

test

@highker highker self-requested a review June 1, 2021 20:41
@highker highker removed their assignment Jun 1, 2021
@yuanzhanhku yuanzhanhku force-pushed the second branch 2 times, most recently from 522f0c6 to 3518d58 Compare June 2, 2021 16:45
@highker highker requested a review from rschlussel June 3, 2021 07:44
Comment on lines 5728 to 5742
Copy link

Choose a reason for hiding this comment

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

accidental change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is changed by the formatter automatically. The previous format seems incorrect because there is a + in the previous line.

Comment on lines 60 to 61
Copy link

Choose a reason for hiding this comment

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

I think it is a legit change; but really would love to have @rschlussel to double check. If prepared statement is going to collect ? in sequence, this visitor shouldn't mess the sequence up or make any damage to the scope (which I assume parameters don't care about scopes).

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, this looks good.

Copy link
Contributor

@rschlussel rschlussel left a comment

Choose a reason for hiding this comment

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

Thanks so much for fixing. Just fix the checkstyle errors, and this looks good.

Comment on lines 60 to 61
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, this looks good.

Override visitLambdaExpression in ParameterExtractingVisitor to support
extracting parameters form lambda expressions. This commit does not add the
override function to DefaultTraversalVisitor because there are other
visitors which do not expect to visit lambdas. For example,
VariablesExtractor doesn't want to extract variables from lambdas and
treat it as dependencies from the source plan.
@yuanzhanhku yuanzhanhku requested a review from highker June 7, 2021 17:11
@highker highker merged commit 9f238ee into prestodb:master Jun 8, 2021
@mayankgarg1990 mayankgarg1990 mentioned this pull request Jun 18, 2021
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Prepared statement parameters in lambda expressions throws exception

3 participants