[v1] Migrate parser to new AST #1626

alancai98 · 2024-10-22T00:17:23Z

Relevant Issues

[v1] Replace sprout-generated AST with hand-written AST #1610

Description

Migrates the partiql-parser to use the new AST

Other Information

Updated Unreleased Section in CHANGELOG: [NO]
- On v1 branch
Any backward-incompatible changes? [NO]
- Some changes but on v1 and hasn't been release yet
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.

github-actions · 2024-10-22T00:24:53Z

CROSS-ENGINE-REPORT ❌

	BASE (LEGACY-V0.14.8)	TARGET (EVAL-5E66224)	+/-
% Passing	89.67%	94.39%	4.72% ✅
Passing	5287	5565	278 ✅
Failing	609	50	-559 ✅
Ignored	0	281	281 🔶
Total Tests	5896	5896	0 ✅

Testing Details

Base Commit: v0.14.8
Base Engine: LEGACY
Target Commit: 5e66224
Target Engine: EVAL

Result Details

❌ REGRESSION DETECTED. See Now Failing/Ignored Tests. ❌
Passing in both: 2643
Failing in both: 17
Ignored in both: 0
PASSING in BASE but now FAILING in TARGET: 3
PASSING in BASE but now IGNORED in TARGET: 108
FAILING in BASE but now PASSING in TARGET: 180
IGNORED in BASE but now PASSING in TARGET: 0

Now FAILING Tests ❌

The following 3 test(s) were previously PASSING in BASE but are now FAILING in TARGET:

Click here to see

undefinedUnqualifiedVariableWithUndefinedVariableBehaviorMissing, compileOption: PERMISSIVE
undefinedUnqualifiedVariableIsNullExprWithUndefinedVariableBehaviorMissing, compileOption: PERMISSIVE
undefinedUnqualifiedVariableIsMissingExprWithUndefinedVariableBehaviorMissing, compileOption: PERMISSIVE

Now IGNORED Tests ❌

The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact.

Now Passing Tests

180 test(s) were previously failing in BASE (LEGACY-V0.14.8) but now pass in TARGET (EVAL-5E66224). 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 ✅

	BASE (EVAL-246B63C)	TARGET (EVAL-5E66224)	+/-
% Passing	94.39%	94.39%	0.00% ✅
Passing	5565	5565	0 ✅
Failing	50	50	0 ✅
Ignored	281	281	0 ✅
Total Tests	5896	5896	0 ✅

Testing Details

Base Commit: 246b63c
Base Engine: EVAL
Target Commit: 5e66224
Target Engine: EVAL

Result Details

Passing in both: 5565
Failing in both: 50
Ignored in both: 281
PASSING in BASE but now FAILING in TARGET: 0
PASSING in BASE but now IGNORED in TARGET: 0
FAILING in BASE but now PASSING in TARGET: 0
IGNORED in BASE but now PASSING in TARGET: 0

alancai98 · 2024-10-23T21:58:51Z

partiql-parser/src/test/kotlin/org/partiql/parser/internal/PartiQLParserBagOpTests.kt

 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()


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.

alancai98 · 2024-10-23T22:00:44Z

partiql-parser/src/main/antlr/PartiQLParser.g4

@@ -358,7 +358,7 @@ excludeExprSteps
    ;

 fromClause
-    : FROM tableReference;
+    : FROM ( tableReference ( COMMA tableReference)* );


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.

alancai98 · 2024-10-23T22:01:32Z

partiql-ast/src/main/java/org/partiql/ast/v1/FromJoin.java

+    public final FromTableRef lhs;

    @NotNull
-    public final From rhs;
+    public final FromTableRef rhs;


This was an error in the original PR adding the hand-written ASTs. The lhs and rhs should have been FromTableRefs.

partiql-parser/src/main/kotlin/org/partiql/parser/V1PartiQLParser.kt

codecov-commenter · 2024-10-24T00:54:44Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (v1-migrate-ast-enum-methods@bb30367). Learn more about missing BASE report.

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

Flag	Coverage Δ
EXAMPLES	`80.03% <ø> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

alancai98 · 2024-10-24T17:33:13Z

partiql-parser/src/main/kotlin/org/partiql/parser/internal/V1PartiQLParserDefault.kt

+        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)


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.

From offline discussion, will change the ANTLR parsing rules slightly:

Remove the optional joinType before the CROSS 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

alancai98 · 2024-10-25T16:46:47Z

Marking as a draft since I need to rebase and incorporate the latest error reporting changes (#1615).

RCHowell

Small comments, otherwise let's get it merged.

partiql-parser/src/main/kotlin/org/partiql/parser/V1PartiQLParser.kt

partiql-parser/src/main/kotlin/org/partiql/parser/V1PartiQLParserBuilder.kt

RCHowell · 2024-10-28T17:50:09Z

partiql-parser/src/main/kotlin/org/partiql/parser/internal/V1PartiQLParserDefault.kt

+        }
+    }
+
+    companion object {


nit no reason for a companion object here unless maybe static?

These were just copied from the existing PartiQLParserDefault for context. Since the static methods within the companion object are all private, I will just move them out of the companion object.

partiql-parser/src/main/kotlin/org/partiql/parser/internal/V1PartiQLParserDefault.kt

RCHowell · 2024-10-28T18:03:37Z

partiql-parser/src/main/kotlin/org/partiql/parser/internal/V1PartiQLParserDefault.kt

+    @OptIn(PartiQLValueExperimental::class)
+    private class Visitor(
+        private val tokens: CommonTokenStream,
+        private val locations: SourceLocations.Mutable,


not for this PR, but I would like to put source locations class under the microscope..

RCHowell · 2024-10-28T18:10:11Z

partiql-parser/src/main/kotlin/org/partiql/parser/internal/V1PartiQLParserDefault.kt

+                "trail" -> GraphRestrictor.TRAIL()
+                "acyclic" -> GraphRestrictor.ACYCLIC()
+                "simple" -> GraphRestrictor.SIMPLE()
+                else -> throw error(ctx, "Unrecognized pattern restrictor")


Curious if the message ever propagates beyond here

I tested locally with a misspelt graph restrictor (i.e. TRAILL rather than TRAIL). The message will be part of a syntax PError with a message captured in the "CAUSE" field -- "org.partiql.parser.PartiQLParserException: Unrecognized pattern restrictor".

ErrorException{error=PError{code=1, severity=ERROR, kind=SYNTAX, location=null, properties={CAUSE=org.partiql.parser.PartiQLParserException: Unrecognized pattern restrictor}}}

I haven't been following the latest PError developments. Is this the expected behavior?

Yes it's ok, we explicitly state that properties are not semvered

RCHowell · 2024-10-28T22:33:59Z

partiql-parser/src/main/kotlin/org/partiql/parser/internal/V1PartiQLParserDefault.kt

+                "trail" -> GraphRestrictor.TRAIL()
+                "acyclic" -> GraphRestrictor.ACYCLIC()
+                "simple" -> GraphRestrictor.SIMPLE()
+                else -> throw error(ctx, "Unrecognized pattern restrictor")


Yes it's ok, we explicitly state that properties are not semvered

alancai98 self-assigned this Oct 22, 2024

alancai98 force-pushed the v1-migrate-ast-parser branch from 6759a9c to 7a68858 Compare October 23, 2024 21:51

alancai98 changed the base branch from v1 to v1-migrate-ast-enum-methods October 23, 2024 21:52

alancai98 changed the title ~~[v1][WIP] Migrate parser to new AST~~ [v1] Migrate parser to new AST Oct 23, 2024

alancai98 commented Oct 23, 2024

View reviewed changes

alancai98 marked this pull request as ready for review October 23, 2024 22:08

alancai98 requested a review from RCHowell October 23, 2024 22:15

alancai98 mentioned this pull request Oct 23, 2024

[v1] Replace sprout-generated AST with hand-written AST #1610

Closed

23 tasks

alancai98 force-pushed the v1-migrate-ast-parser branch 2 times, most recently from 8eae47f to 43584f9 Compare October 24, 2024 00:47

alancai98 commented Oct 24, 2024

View reviewed changes

alancai98 force-pushed the v1-migrate-ast-parser branch from 43584f9 to 31d6fd0 Compare October 24, 2024 18:49

alancai98 force-pushed the v1-migrate-ast-enum-methods branch from bb30367 to 5db9f62 Compare October 25, 2024 01:11

Base automatically changed from v1-migrate-ast-enum-methods to v1 October 25, 2024 16:45

alancai98 marked this pull request as draft October 25, 2024 16:46

alancai98 force-pushed the v1-migrate-ast-parser branch from 31d6fd0 to 8751c18 Compare October 25, 2024 22:16

alancai98 changed the base branch from v1 to v1-migrate-ast October 25, 2024 22:16

alancai98 marked this pull request as ready for review October 25, 2024 22:22

Base automatically changed from v1-migrate-ast to v1 October 28, 2024 17:52

alancai98 added 2 commits October 28, 2024 10:55

[v1] Replicate PartiQLParserDefault with v1 AST

6ae24a7

Rebase off error-reporting changes

da868e1

alancai98 force-pushed the v1-migrate-ast-parser branch from 8751c18 to da868e1 Compare October 28, 2024 17:55

RCHowell requested changes Oct 28, 2024

View reviewed changes

Apply Robert's feedback

68139a6

alancai98 requested a review from RCHowell October 28, 2024 18:51

RCHowell approved these changes Oct 28, 2024

View reviewed changes

alancai98 merged commit f0569bd into v1 Oct 28, 2024
14 checks passed

alancai98 deleted the v1-migrate-ast-parser branch October 28, 2024 23:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v1] Migrate parser to new AST #1626

[v1] Migrate parser to new AST #1626

alancai98 commented Oct 22, 2024 •

edited

Loading

github-actions bot commented Oct 22, 2024 •

edited

Loading

alancai98 Oct 23, 2024

alancai98 Oct 23, 2024

alancai98 Oct 23, 2024

codecov-commenter commented Oct 24, 2024 •

edited

Loading

alancai98 Oct 24, 2024

alancai98 Oct 24, 2024

alancai98 commented Oct 25, 2024

RCHowell left a comment

RCHowell Oct 28, 2024

alancai98 Oct 28, 2024

RCHowell Oct 28, 2024

RCHowell Oct 28, 2024

alancai98 Oct 28, 2024

alancai98 Oct 28, 2024

RCHowell Oct 28, 2024

RCHowell Oct 28, 2024

[v1] Migrate parser to new AST #1626

[v1] Migrate parser to new AST #1626

Conversation

alancai98 commented Oct 22, 2024 • edited Loading

Relevant Issues

Description

Other Information

License Information

github-actions bot commented Oct 22, 2024 • edited Loading

CROSS-ENGINE-REPORT ❌

Testing Details

Result Details

Now FAILING Tests ❌

Now IGNORED Tests ❌

Now Passing Tests

CROSS-COMMIT-REPORT ✅

Testing Details

Result Details

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 24, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alancai98 commented Oct 25, 2024

RCHowell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alancai98 commented Oct 22, 2024 •

edited

Loading

github-actions bot commented Oct 22, 2024 •

edited

Loading

codecov-commenter commented Oct 24, 2024 •

edited

Loading