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

Support functionc call rewrite by Presto Verifier #22783

Merged
merged 1 commit into from
May 28, 2024

Conversation

gggrace14
Copy link
Contributor

@gggrace14 gggrace14 commented May 18, 2024

Description

Enable Presto Verifier to substitute function call expressions in a source query, according to the user specified substitution pattern, by the config function-substitutes. Support function call substitutions when rewriting Query, CreateTableAsSelect and Insert source queries.

Motivation and Context

Allow the Presto Verifier users to substitute functions, ex., nondeterministic functions, to make the test query comparable with TEST and CONTROL.

Impact

The feature is enabled only when the parameter --function-substitutes is set.

Test Plan

Unit tests in TestQueryRewriter.

Release Notes

== RELEASE NOTES ==

Verifier Changes
* Support function call substitution based on the specified substitution pattern passed by the parameter --function-substitutes.

Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

Nice work, thanks! Nits of formatting suggested to improve readability.

presto-docs/src/main/sphinx/admin/verifier.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/admin/verifier.rst Outdated Show resolved Hide resolved
@gggrace14 gggrace14 marked this pull request as ready for review May 22, 2024 06:24
@gggrace14 gggrace14 requested a review from a team as a code owner May 22, 2024 06:24
@gggrace14
Copy link
Contributor Author

Nice work, thanks! Nits of formatting suggested to improve readability.

Hi @steveburnett , I revised the .rst doc based on my understanding of the comments. Let me know if I miss anything.

Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

Looks good! One tiny nit.

where ``foo(c0, _)`` would be substituted by ``bar(c0)``,
with the declared arguments applied to the corresponding positions.

Concatenate function substitutions with comma.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Concatenate function substitutions with comma.
Concatenate function substitutions with a comma.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revised

@gggrace14 gggrace14 force-pushed the newfunction branch 2 times, most recently from 154a141 to 20d3a04 Compare May 25, 2024 08:54
arhimondr
arhimondr previously approved these changes May 25, 2024
@gggrace14 gggrace14 changed the title Support functionc all rewrite by Presto Verifier Support functionc call rewrite by Presto Verifier May 26, 2024
Enable Presto Verifier to substitute function call expressions in a source query,
according to the user specified substitution pattern, by the config
function-substitutes. Support function call substitutions when rewriting Query,
CreateTableAsSelect and Insert source queries.
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

LGTM! (docs)

Pull updated branch, new local build, everything looks good. Thanks!

@gggrace14 gggrace14 merged commit f3ac97f into prestodb:master May 28, 2024
57 checks passed
@gggrace14 gggrace14 deleted the newfunction branch May 28, 2024 16:09
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.

None yet

3 participants