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

Add PostgreSQL specfic "CREATE TYPE t AS ENUM (...)" support. #1460

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

caldwell
Copy link
Contributor

@caldwell caldwell commented Oct 7, 2024

See: https://www.postgresql.org/docs/current/sql-createtype.html

I implemented this as a separate Statement::CreateTypeAsEnum { name, labels } so it wouldn't be as invasive to the existing Statement::CreateType (which I would imagine helps forwards/backwards compatibility for the crate's users).

I used the PostgreSqlDialect's comment as a blueprint but that means that the generic dialect doesn't get this feature (unless I'm missing something). I didn't see parse_statement() implemented in GenericDialect though I suppose it could be (and call out to other dialects' parse_statement()s). I didn't want to set that precedent without asking.

Copy link
Contributor

@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 for this contribution @caldwell

implemented in GenericDialect though I suppose it could be (and call out to other dialects' parse_statement()s). I didn't want to set that precedent without asking.

I agree -- I think unless there is a good reason to start migrating the code to a different pattern, following the existing patterns is probably the best

cc @iffyio

@coveralls
Copy link

coveralls commented Oct 7, 2024

Pull Request Test Coverage Report for Build 11429592113

Details

  • 31 of 32 (96.88%) changed or added relevant lines in 3 files are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.004%) to 89.391%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/dialect/postgresql.rs 16 17 94.12%
Files with Coverage Reduction New Missed Lines %
src/ast/ddl.rs 1 87.46%
tests/sqlparser_postgres.rs 2 88.57%
Totals Coverage Status
Change from base Build 11429400839: 0.004%
Covered Lines: 30494
Relevant Lines: 34113

💛 - Coveralls

src/ast/mod.rs Outdated
Comment on lines 3184 to 3190
/// CREATE TYPE <name> AS ENUM
/// ```
/// Note: this is a PostgreSQL-specific statement. See <https://www.postgresql.org/docs/current/sql-createtype.html>
CreateTypeAsEnum {
name: ObjectName,
labels: Vec<Ident>,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking it could make sense to include it as part of the existing CreateType, especially since its already an enum there wouldn't be as much of disadvantage in terms of backwards compatibility? then going forward we'd be able to extend it in a similar manner with future extensions to the statement. something like:

pub enum UserDefinedTypeRepresentation {
    Composite {
        attributes: Vec<UserDefinedTypeCompositeAttributeDef>,
    },
    // new variant
    Enum(CreateTypeEnum)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! I somehow missed that UserDefinedTypeRepresentation was an enum. I thought it was a struct which would be more invasive to change, hence the new CreateTypeAsEnum. But yeah, as an enum that's a much better place for it.

I ended up with:

pub enum UserDefinedTypeRepresentation {
    Composite {
        attributes: Vec<UserDefinedTypeCompositeAttributeDef>,
    },
    /// Note: this is PostgreSQL-specific. See <https://www.postgresql.org/docs/current/sql-createtype.html>
    Enum { labels: Vec<Ident> },
}

…so that Enum matches Composite. If you think it should be a real struct instead of the anonymous enum struct I can change it.

Comment on lines 142 to 147
match parse_create(parser) {
Some(result) => Some(result),
None => {
parser.prev_token(); // unconsume the CREATE if we didn't end up parsing anything
None
}
Copy link
Contributor

Choose a reason for hiding this comment

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

it might be possible to simplify this if we call prev_token() before calling parse_create? thinking since the latter already returns an Option representing the same result

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that can make it much simpler. I also tried using peek to avoid doing "consume, unconsume, consume" but it looks like there's no nice peek_keyword() type function and so it ended up looking like:

if matches!(parser.peek_token().token, Token::Word(Word { keyword: Keyword::CREATE, .. })) {
    parse_create(parser)
}

That's a bit of a mouthful and I ended up just going with your suggestion:

if parser.parse_keyword(Keyword::CREATE) {
    parser.prev_token(); // unconsume the CREATE in case we don't end up parsing anything
    parse_create(parser)
}

@alamb
Copy link
Contributor

alamb commented Oct 20, 2024

@caldwell have you had a chance to consider @iffyio 's suggestions above?

@caldwell
Copy link
Contributor Author

Apologies, not yet. I'll try to look at it today.

@caldwell
Copy link
Contributor Author

I amended my commit per @iffyio's suggestions. Thanks for taking the time to look at it and apologies again for taking too long to respond.

@alamb
Copy link
Contributor

alamb commented Oct 20, 2024

I amended my commit per @iffyio's suggestions. Thanks for taking the time to look at it and apologies again for taking too long to respond.

no worries -- thank you!

@caldwell
Copy link
Contributor Author

I pushed a fix for the formatting test failure.

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.

4 participants