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 antlr grammar and visitor for postgresql #416

Merged
merged 26 commits into from
Sep 6, 2023
Merged

Conversation

NQPhuc
Copy link
Contributor

@NQPhuc NQPhuc commented Aug 4, 2023

Summary

  • Add postgresql parser using antlr grammar (based on: https://github.com/antlr/grammars-v4/tree/master/sql/postgresql)

  • The old postgres parser can still be used through: sql2dbml --postgres-legacy

  • Add AST classes to unified parsers output. These class can be used to build the model_structure.

  • Implement PostgresASTGen visitor to generate the AST from the parse tree. Note that not all nodes will be visited in our visitor, only the nodes related to model_structure and thus DBML is.

  • Fix an error in dbml parser where index pk setting couldn't be used with other settings such as name

  • Update tests.

  • Note: Files in ANTLR/parsers/postgresql are auto-generated and can be safely ignored.

Issue

Lasting Changes (Technical)

Checklist

Please check directly on the box once each of these are done

  • Documentation (if necessary)
  • Tests (integration test/unit test)
  • Integration Tests Passed
  • Code Review

@NQPhuc NQPhuc marked this pull request as ready for review August 7, 2023 11:08
@huyphung1602
Copy link
Collaborator

We should add some tests here. For example, put a PostgreSQL to the parser and the compare the output with the expected output.

Comment on lines 463 to 465
for (let i = 1; i < ctx.getChildCount(); i++) {
r += ` ${ctx.getChild(i).getText()}`;
};
Copy link
Collaborator

@huyphung1602 huyphung1602 Aug 10, 2023

Choose a reason for hiding this comment

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

Can we use lodash functions here?

Copy link
Contributor Author

@NQPhuc NQPhuc Aug 24, 2023

Choose a reason for hiding this comment

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

Updated in 15d016f. Although I did not use lodash, I replaced the for loop and made it functional by generating an array range of indices and then reduce it into a string.

@@ -0,0 +1,941 @@
/* eslint-disable class-methods-use-this */
import _ from 'lodash';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please import only the lodash functions that you are using. For example:

import { map } from 'lodash';

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 in 15d016f.

@huyphung1602
Copy link
Collaborator

  • Note: Except for the 2 based class, files in ANTLR/parsers/postgresql are auto-generated and can be safely ignored.

It is not useful for the reviewer em. You should list out the files that are auto-generated and can be skip:

  • ignore_path/*
  • file_name_1
  • file_name_2

TBH, I do not understand what is '2 based class'.

@TeaNguyen TeaNguyen mentioned this pull request Aug 16, 2023
Comment on lines 106 to 149
-- issue 324
CREATE TABLE "public"."accounts" (
"id" integer GENERATED ALWAYS AS IDENTITY NOT NULL,
"domain" character varying(100),
"name" character varying(100),
"slug" character varying(10),
CONSTRAINT "accounts_pkey" PRIMARY KEY ("id")
) WITH (oids = false);

-- issue 281
CREATE TABLE public.tests ( id bigint NOT NULL, type character varying DEFAULT 'testing'::character varying );

-- issue 427
CREATE TABLE public.users2 (
username character varying NOT NULL,
hashed_password character varying NOT NULL,
full_name character varying NOT NULL,
email character varying NOT NULL,
password_changed_at timestamp with time zone DEFAULT '0001-01-01 00:00:00+00'::timestamp with time zone NOT NULL,
created_at timestamp with time zone DEFAULT now() NOT NULL
);

-- issue 248
create table table1 (
field text not null,
field2 text not null,
primary key(field, field2)
);

create table if not exists table2 (
id int generated always as identity primary key,
field text,
field2 text,
foreign key (field, field2) references table1(field, field2)
);

-- issue 222
CREATE TABLE test_table (
"name" varchar NOT NULL,
id varchar NULL,
linked_name varchar NOT NULL,
CONSTRAINT test_table_pk PRIMARY KEY ("name", id),
CONSTRAINT test_table_fk FOREIGN KEY (linked_name,id) REFERENCES test_table("name",id)
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add the issue links inside the code nha em

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in 5acb21a

}

// helper functions required by the generated grammar
// To avoid error, we declare their signature and import them in the generated PostgreSQLLexer.js
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should mention that these methods were defaulted in C# but they were not defaulted in Javascript. Do these methods affect the result of the parser?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the comment in 5acb21a.

Do these methods affect the result of the parser?

According to my testing, no.

Copy link
Collaborator

@huyphung1602 huyphung1602 left a comment

Choose a reason for hiding this comment

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

LGTM. However, because it supports more PostgreSQL syntaxes, performance may suffer. Please assist in benchmarking the parser.

@nguyenalter
Copy link
Contributor

Please update the documentation page to include your changes related to the CLI and JS Module

@NQPhuc NQPhuc merged commit 16edf62 into master Sep 6, 2023
4 checks passed
@NQPhuc NQPhuc added PR: Bug Fix 🐛 A type of pull request used for changelog categories PR: Breaking Change 💥 A type of pull request used for changelog categories PR: New Feature 🚀 A type of pull request used for changelog categories labels Sep 6, 2023
@coderabbitai coderabbitai bot mentioned this pull request Apr 25, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Breaking Change 💥 A type of pull request used for changelog categories PR: Bug Fix 🐛 A type of pull request used for changelog categories PR: New Feature 🚀 A type of pull request used for changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants