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 postgres CREATE FUNCTION #722

Merged
merged 7 commits into from Dec 1, 2022

Conversation

wangrunji0408
Copy link
Contributor

This PR adds some support for postgres CREATE FUNCTION based on:
https://www.postgresql.org/docs/15/sql-createfunction.html

To live with the hive syntax introduced by #496, we move its field class_name and using into the new CreateFunctionBody structure.

Signed-off-by: Runji Wang <wangrunji0408@163.com>
Signed-off-by: Runji Wang <wangrunji0408@163.com>
Signed-off-by: Runji Wang <wangrunji0408@163.com>
Signed-off-by: Runji Wang <wangrunji0408@163.com>
Signed-off-by: Runji Wang <wangrunji0408@163.com>
@coveralls
Copy link

coveralls commented Nov 21, 2022

Pull Request Test Coverage Report for Build 3590787964

  • 150 of 169 (88.76%) changed or added relevant lines in 4 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.01%) to 86.361%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/ast/mod.rs 40 45 88.89%
src/parser.rs 67 81 82.72%
Files with Coverage Reduction New Missed Lines %
src/ast/mod.rs 1 77.7%
src/parser.rs 1 83.48%
Totals Coverage Status
Change from base Build 3586103034: 0.01%
Covered Lines: 12309
Relevant Lines: 14253

💛 - Coveralls

Copy link
Collaborator

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you very much for the contribution @wangrunji0408

I had a question about the use of Vec<CreateFunctionBody> but otherwise this PR is looking good. Thank you!

cc @AugustoFKL

cc @mobuchowski as you contributed the original CREATE FUNCTION #496

src/ast/mod.rs Outdated
using: Option<CreateFunctionUsing>,
args: Option<Vec<CreateFunctionArg>>,
return_type: Option<DataType>,
bodies: Vec<CreateFunctionBody>,
Copy link
Collaborator

@alamb alamb Nov 30, 2022

Choose a reason for hiding this comment

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

I find the Vec<CreateFunctionBody> somewhat confusing as it implies that some of them can be repeated. For example, can there be two CreateFunctionBody::Returning in this list? I don't think it is possible

Did you consider creating optional fields to encode this in the type system itself?

So instead of

    CreateFunction {
        or_replace: bool,
        temporary: bool,
        name: ObjectName,
        args: Option<Vec<CreateFunctionArg>>,
        return_type: Option<DataType>,
        bodies: Vec<CreateFunctionBody>,

Something like

    CreateFunction {
        or_replace: bool,
        temporary: bool,
        name: ObjectName,
        args: Option<Vec<CreateFunctionArg>>,
        return_type: Option<DataType>,
        as: Option<String>.
        /// LANGUAGE lang_name
        language: Option<Ident>,
...
}

?

Copy link
Contributor

Choose a reason for hiding this comment

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

@alamb @wangrunji0408 I have some time, but will not be able to review the whole PR RN, sorry.

  • The return type does not match the PostgreSQL syntax. @alamb, do you think it's reasonable to add something like a SUPPORTED SYNTAX field to the header of the structure? Here's an example that I'm implementing for myself.

  • @wangrunji0408, for the body structure, it's a problem that we can repeat. You're creating a syntax that can be built wrong for a single dialect. Please let us know if you have a good reason (something like it being repeatable on Snowflake or something like that). However, @alamb, I think having all of them, as you showed, makes building it complex and moving it as well. Sometimes the upstream only wants to handle one or two scenarios, not the whole object. My suggestion is to have a structure that encapsulates them all:

CREATE FUNCTION {
...,
or_replace: bool, 
temporary: bool, 
name: ObjectName, 
args: Option<Vec<CreateFunctionArg>>, 
return_type: Option<DataType>, 
opt_body: Option<FunctionBody>,
}

pub enum FunctionBody {
Language...
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @AugustoFKL and @alamb regarding repeatable structures.

From my side, I can still express Hive's CREATE FUNCTION syntax so it looks good for me - for me it's okay to change my code when bumping sqlparser-rs dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this catch. I didn't realize the problem with duplicate items. It has been changed to the way @AugustoFKL mentioned above.

tests/sqlparser_hive.rs Show resolved Hide resolved
src/ast/mod.rs Outdated Show resolved Hide resolved
src/ast/mod.rs Outdated
using: Option<CreateFunctionUsing>,
args: Option<Vec<CreateFunctionArg>>,
return_type: Option<DataType>,
bodies: Vec<CreateFunctionBody>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @AugustoFKL and @alamb regarding repeatable structures.

From my side, I can still express Hive's CREATE FUNCTION syntax so it looks good for me - for me it's okay to change my code when bumping sqlparser-rs dependency.

tests/sqlparser_hive.rs Show resolved Hide resolved
Signed-off-by: Runji Wang <wangrunji0408@163.com>
Signed-off-by: Runji Wang <wangrunji0408@163.com>
Copy link
Collaborator

@alamb alamb 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 to me -- thank you @wangrunji0408

pub behavior: Option<FunctionBehavior>,
/// AS 'definition'
///
/// Note that Hive's `AS class_name` is also parsed here.
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@alamb alamb merged commit 5b53df9 into sqlparser-rs:main Dec 1, 2022
@wangrunji0408 wangrunji0408 deleted the create-function-as-expr branch December 2, 2022 02:45
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

5 participants