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.

Sign up for GitHub

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

Jump to bottom

Add syntax support for CREATE FUNCTION #13481

Merged
merged 1 commit into from
Oct 8, 2019
Merged

Add syntax support for CREATE FUNCTION #13481

merged 1 commit into from
Oct 8, 2019

Conversation

caithagoras
Copy link
Contributor

@caithagoras caithagoras commented Sep 30, 2019 •
edited
Loading

Introduce CREATE FUNCTION syntax as according to #13254

== NO RELEASE NOTE ==

@caithagoras
Copy link
Contributor Author

This PR is ready for review.

@caithagoras caithagoras changed the title Add syntax support for CreateFunction Add syntax support for CREATE FUNCTION Oct 1, 2019
@rongrong
Copy link
Contributor

rongrong commented Oct 1, 2019

At least add another commit to hook this up to call FunctionManager.addFunction so it's complete.

@caithagoras caithagoras force-pushed the s5 branch 5 times, most recently from 9a7c177 to fce596f Compare October 1, 2019 21:31
@caithagoras
Copy link
Contributor Author

caithagoras commented Oct 1, 2019 •
edited
Loading

@rongrong Per offline discussion, removed SqlDataAccess. Looking for a committer approval. Thanks!

rongrong
rongrong reviewed Oct 2, 2019
View reviewed changes
Copy link
Contributor

@rongrong rongrong left a comment

Choose a reason for hiding this comment

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

Mostly look good except I think that function body should include RETURN. Maybe good to have another pair of eyes since this is a syntax change. @arhimondr @kaikalur

presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4 Outdated Show resolved Hide resolved
presto-parser/src/main/java/com/facebook/presto/sql/tree/ParameterDeclaration.java Outdated Show resolved Hide resolved
@rongrong rongrong requested review from arhimondr and kaikalur October 2, 2019 23:30
@caithagoras caithagoras force-pushed the s5 branch 3 times, most recently from 2996871 to cbbfa56 Compare October 3, 2019 06:40
@caithagoras
Copy link
Contributor Author

comments addressed

kaikalur
kaikalur approved these changes Oct 3, 2019
View reviewed changes
presto-parser/src/main/java/com/facebook/presto/sql/tree/RoutineCharacteristics.java
@@ -0,0 +1,124 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

A general comment, not specific to this - for things like this, it may be simpler to just add these as attributes to the CreateFunction node to reduce code bloat and make it easy to work with the visitor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will be a reusable unit in ALTER FUNCTION so it makes sense to have it separated. Visitor won't touch this class since it is not a subclass of Node, or a child of Create Function.

arhimondr
arhimondr approved these changes Oct 4, 2019
View reviewed changes
Copy link
Member

@arhimondr arhimondr left a comment

Choose a reason for hiding this comment

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

LGTM % questions / nits

presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4 Outdated
@@ -472,21 +507,21 @@ nonReserved
// IMPORTANT: this rule must only contain tokens. Nested rules are not supported. See SqlParser.exitNonReserved
: ADD | ADMIN | ALL | ANALYZE | ANY | ARRAY | ASC | AT
| BERNOULLI
| CALL | CASCADE | CATALOGS | COLUMN | COLUMNS | COMMENT | COMMIT | COMMITTED | CURRENT | CURRENT_ROLE
| DATA | DATE | DAY | DESC | DISTRIBUTED
| CALL | CALLED | CASCADE | CATALOGS | COLUMN | COLUMNS | COMMENT | COMMIT | COMMITTED | CURRENT | CURRENT_ROLE
Copy link
Member

Choose a reason for hiding this comment

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

question: none of the newly introduced keywords are reserved? Did we check the spec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed: They are in fact all reserved words.
Also updated reserved.rst to pass tests.

presto-parser/src/main/java/com/facebook/presto/sql/SqlFormatter.java Outdated Show resolved Hide resolved
presto-parser/src/main/java/com/facebook/presto/sql/parser/AstBuilder.java Outdated Show resolved Hide resolved
presto-parser/src/main/java/com/facebook/presto/sql/tree/RoutineCharacteristics.java Show resolved Hide resolved
@caithagoras caithagoras force-pushed the s5 branch 3 times, most recently from 637b878 to ad4fd4b Compare October 7, 2019 18:53
@caithagoras caithagoras force-pushed the s5 branch 2 times, most recently from ed0f080 to 2099686 Compare October 7, 2019 20:49
@rongrong
Copy link
Contributor

rongrong commented Oct 7, 2019

@caithagoras There's still a test failure (error message change)

@caithagoras caithagoras merged commit 0ec6621 into prestodb:master Oct 8, 2019
@caithagoras caithagoras deleted the s5 branch October 8, 2019 04:53
@tooptoop4 tooptoop4 mentioned this pull request Sep 5, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rongrong rongrong rongrong approved these changes

@arhimondr arhimondr arhimondr approved these changes

@kaikalur kaikalur kaikalur approved these changes

Assignees

@rongrong rongrong

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@caithagoras @rongrong @kaikalur @arhimondr @facebook-github-bot @caithagoras0