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 spec-defined SqlParserTest tests; fix ParseError symbol #10

Merged
merged 3 commits into from
Apr 22, 2022

Conversation

alancai98
Copy link
Member

@alancai98 alancai98 commented Apr 20, 2022

Part of #5.

Adds tests from the latest commit of SqlParserTests.kt. SqlParserTests.kt is one of the larger files with parser tests. Omits some tests that are not part of the current spec (e.g. DML, DDL, LET clause) along with some other tests (full details in this issue comment).

Also, fixes an error in the parse error assertion result. ParserError -> ParseError.

I verified the new test files are parseable using the Rust test runner created in partiql/partiql-lang-rust#75.

In total, will now have 163 tests (up from 15 previously).

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@alancai98 alancai98 requested review from jpschorr and am357 April 20, 2022 22:05
@alancai98 alancai98 self-assigned this Apr 20, 2022
Copy link
Contributor

@am357 am357 left a comment

Choose a reason for hiding this comment

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

Came up with some comments. In addition, was wondering if at some point we're planning to have unhappy paths too i.e. ParseError.

]
},
parse::{
name: "call SUBSTRING normal syntax",
Copy link
Contributor

Choose a reason for hiding this comment

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

For this and other lines that it applies: by normal do we mean PartiQL syntax? If so, how about changing it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking more about this, should we consider porting the sql92, etc. to namespaces?

E.g.:

'sql92'::[
    parse::{
        ...
    }
]

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, "normal" referred to the PartiQL function call syntax. I can change to use a more specific term.

Thinking more about this, should we consider porting the sql92, etc. to namespaces?

Good suggestion. I will enclose the sql92-related syntax in its own namespace.

Copy link
Member Author

@alancai98 alancai98 left a comment

Choose a reason for hiding this comment

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

In addition, was wondering if at some point we're planning to have unhappy paths too i.e. ParseError.

Yes, as part of #5, more error tests will be added. This PR just added tests from SqlParserTest.kt which only had passing tests.

]
},
parse::{
name: "call SUBSTRING normal syntax",
Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, "normal" referred to the PartiQL function call syntax. I can change to use a more specific term.

Thinking more about this, should we consider porting the sql92, etc. to namespaces?

Good suggestion. I will enclose the sql92-related syntax in its own namespace.

@alancai98 alancai98 requested a review from am357 April 21, 2022 21:00
]
},
],
'function-argument-list'::[
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for breaking these down, since we'll likely going to have a similar pattern in other tests (E.g. sql92 vs. sql2011 vs. partiql), does it make sense to rename such namespaces (E.g. function-argument-list) to partiql?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, sure I can change it to the partiql namespace.

@alancai98 alancai98 requested a review from am357 April 21, 2022 21:36
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.

3 participants