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

feat: add support for type name #106

Merged
merged 3 commits into from
Jan 8, 2024

Conversation

cvng
Copy link
Contributor

@cvng cvng commented Jan 2, 2024

What kind of change does this PR introduce?

add support for "some" typename

What is the current behavior?

parser panics

What is the new behavior?

parser returns

Additional context

as I understands it, the case that causes panics are:

  • multi-word type name, eg: timestamp with time zone
  • and interval options, eg: hour to second(3)

single-word type name, looks ok but I will have to double-check. cc @psteinroe

@cvng cvng marked this pull request as draft January 2, 2024 18:47
Copy link
Collaborator

@psteinroe psteinroe left a comment

Choose a reason for hiding this comment

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

For the type names, you can extend the alias definition. I think it's defined directly in the parser code.

@cvng
Copy link
Contributor Author

cvng commented Jan 2, 2024

For the type names, you can extend the alias definition. I think it's defined directly in the parser code.

OK, after going back in the git history, it looks like the original parser panics was fixed, so not sure if this if required anymore?

the original error and the failing case:

could not find node for token Token { kind: To, text: "to", span: 98..100, token_type: ReservedKeyword } at depth 4
CREATE TABLE IF NOT EXISTS distributors (name varchar(40) DEFAULT 'Luso Films', len interval hour to second(3), name varchar(40) DEFAULT 'Luso Films', did int DEFAULT nextval('distributors_serial'), stamp timestamp DEFAULT now() NOT NULL, stamptz timestamp with time zone, "time" time NOT NULL, timetz time with time zone, CONSTRAINT name_len PRIMARY KEY (name, len));
                                                                                                  ^

I was about to implement this https://github.com/pganalyze/libpg_query/blob/16-latest/src/postgres_deparse.c#L3879

EDIT: I had the wrong base reference, the parser still panics, I will try Alias as suggested

@cvng
Copy link
Contributor Author

cvng commented Jan 2, 2024

base case added in 3aba5fb

@cvng cvng marked this pull request as ready for review January 2, 2024 22:34
@psteinroe
Copy link
Collaborator

lgtm, thanks!

@psteinroe psteinroe merged commit 92a8f2b into supabase-community:main Jan 8, 2024
1 check passed
.and_then(|node| if let protobuf::a_const::Val::Ival(n) = node { Some(n.ival) } else { None });

if let Some(fields) = fields {
match fields.clone() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: add other fields from intervaltypmodout

@cvng cvng deleted the feat/type-name branch January 8, 2024 16:34
@cvng cvng mentioned this pull request Jan 8, 2024
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.

2 participants