-
Notifications
You must be signed in to change notification settings - Fork 61
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
[v1] Migrate parser to new AST #1626
base: v1-migrate-ast
Are you sure you want to change the base?
Conversation
CROSS-ENGINE-REPORT ❌
Testing Details
Result Details
Now FAILING Tests ❌The following 3 test(s) were previously PASSING in BASE but are now FAILING in TARGET: Click here to see
Now IGNORED Tests ❌The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact. Now Passing Tests180 test(s) were previously failing in BASE (LEGACY-V0.14.8) but now pass in TARGET (EVAL-B964B09). Before merging, confirm they are intended to pass. The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact. CROSS-COMMIT-REPORT ✅
Testing DetailsResult Details
|
6759a9c
to
7a68858
Compare
import org.partiql.value.PartiQLValueExperimental | ||
import org.partiql.value.int32Value | ||
import org.partiql.value.stringValue | ||
import kotlin.test.assertEquals | ||
|
||
class PartiQLParserBagOpTests { | ||
|
||
private val parser = PartiQLParserDefault() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the partiql-parser tests were modified to use the V1PartiQLParserDefault
. Existing tests other than the DDL tests (since DDL will be added in a later v1 release candidate), still passes with the new parser.
@@ -358,7 +358,7 @@ excludeExprSteps | |||
; | |||
|
|||
fromClause | |||
: FROM tableReference; | |||
: FROM ( tableReference ( COMMA tableReference)* ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made some ANTLR grammar changes to account for the new modeling of FROM
clauses to be better aligned with the SQL1999 BNF definition. See #1579 (comment) for some further context.
public final FromTableRef lhs; | ||
|
||
@NotNull | ||
public final From rhs; | ||
public final FromTableRef rhs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was an error in the original PR adding the hand-written ASTs. The lhs
and rhs
should have been FromTableRef
s.
import org.partiql.ast.v1.Statement | ||
import org.partiql.parser.internal.V1PartiQLParserDefault | ||
|
||
public interface V1PartiQLParser { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicated all of the existing PartiQLParser
APIs for now to keep the other tests passing (e.g. planner, eval). After those other packages are migrated to use the parser outputting the new v1 AST, then we can delete the existing sprout AST PartiQLParser
APIs.
8eae47f
to
43584f9
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## v1-migrate-ast-enum-methods #1626 +/- ##
==============================================================
Coverage ? 80.03%
Complexity ? 47
==============================================================
Files ? 19
Lines ? 506
Branches ? 23
==============================================================
Hits ? 405
Misses ? 88
Partials ? 13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
override fun visitTableCrossJoin(ctx: PartiQLParser.TableCrossJoinContext): FromTableRef = translate(ctx) { | ||
val lhs = visitAs<FromTableRef>(ctx.lhs) | ||
val rhs = visitAs<FromTableRef>(ctx.rhs) | ||
// TODO support other CROSS JOIN variants (e.g. LEFT CROSS JOIN) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous ANTLR parser visitor would parse l LEFT CROSS JOIN r
to the AST equivalent of l LEFT JOIN r ON TRUE
. This was mentioned in the PartiQL spec in section 5.4 (https://partiql.org/dql/joins.html#left-join), but I couldn't find it mentioned in the SQL1999 spec. It's not yet clear to me what other JOIN types (e.g. RIGHT
, LEFT OUTER
, INNER
, FULL
, etc.) should support this behavior so I've left it out of the new ANTLR parser visitor for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From offline discussion, will change the ANTLR parsing rules slightly:
- Remove the optional
joinType
before theCROSS JOIN
since that's what is defined in SQL99 spec - Add an explicit rule for
LEFT CROSS JOIN
since that's what is defined in the PartiQL spec - Add TODO linking Vladimir's issue for the other cross join variant investigation -- Explore SQL-consistent syntax for joins #1013
43584f9
to
31d6fd0
Compare
bb30367
to
5db9f62
Compare
Marking as a draft since I need to rebase and incorporate the latest error reporting changes (#1615). |
31d6fd0
to
8751c18
Compare
Relevant Issues
Description
Other Information
Updated Unreleased Section in CHANGELOG: [NO]
Any backward-incompatible changes? [NO]
Any new external dependencies? [NO]
Do your changes comply with the Contributing Guidelines
and Code Style Guidelines? [YES]
License Information
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.