[v1] Adds AstVisitor and AstRewriter; port partiql-ast normalization passes to partiql-planner #1629

alancai98 · 2024-10-23T23:55:50Z

Relevant Issues

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

Description

Makes AstVisitor an abstract class with defaults
Adds an AstRewriter
Port the partiql-ast normalization passes to partiql-planner package and makes them internal
Other partiql-planner AstVisitor impls are to be added in a subsequent PR

Other Information

Updated Unreleased Section in CHANGELOG: [NO]
- No, on v1 branch.
Any backward-incompatible changes? [NO]
Any new external dependencies? [NO]
- < If YES, which ones and why? >
- < In addition, please also mention any other alternatives you've considered and the reason they've been discarded >
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.

alancai98 · 2024-10-23T23:56:46Z

partiql-ast/src/main/kotlin/org/partiql/ast/helpers/ToBinder.kt

@@ -6,6 +6,8 @@ import org.partiql.ast.builder.ast
 import org.partiql.value.PartiQLValueExperimental
 import org.partiql.value.StringValue

+// TODO DELETE FILE


These partiql-ast files marked w/ // TODO DELETE FILE will be deleted once the sprout-generated AST is deleted.

partiql-ast/src/main/java/org/partiql/ast/v1/expr/SessionAttribute.java

github-actions · 2024-10-24T00:03:39Z

CROSS-ENGINE-REPORT ❌

	BASE (LEGACY-V0.14.8)	TARGET (EVAL-2BE7C0F)	+/-
% 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: 2be7c0f
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-2BE7C0F). 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-F0569BD)	TARGET (EVAL-2BE7C0F)	+/-
% 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: f0569bd
Base Engine: EVAL
Target Commit: 2be7c0f
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

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

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.03%. Comparing base (31d6fd0) to head (cb9dd8a).

Additional details and impacted files

@@                   Coverage Diff                    @@
##             v1-migrate-ast-parser    #1629   +/-   ##
========================================================
  Coverage                    80.03%   80.03%           
  Complexity                      47       47           
========================================================
  Files                           19       19           
  Lines                          506      506           
  Branches                        23       23           
========================================================
  Hits                           405      405           
  Misses                          88       88           
  Partials                        13       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-28T23:33:12Z

Going to mark this as a draft till I fix the visitor modeling a bit.

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

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

alancai98 · 2024-10-30T23:20:04Z

partiql-ast/src/main/java/org/partiql/ast/v1/AstRewriter.kt

+import org.partiql.ast.v1.graph.GraphSelector
+import org.partiql.value.PartiQLValueExperimental
+
+public abstract class AstRewriter<C> : AstVisitor<AstNode, C> {


(self-review) AstRewriter written in Kotlin since it was slightly easier to create based on the sprout-generated AstRewriter. I believe it should have similar public API as if it was in Java but if not, I can migrate it to Java.

I can check the decompiled bytecode, but this resides in the java source set btw.

but this resides in the java source set btw.

Yeah, I'll fix this in a later PR (probably the PR that deletes the sprout AST code). There's some other Kotlin sources in the java directory that I'll also move in that PR.

…asses to partiql-planner

johnedquinn · 2024-10-31T20:59:13Z

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

-    R visitGraphLabelConj(GraphLabel.Conj node, C ctx);
-
-    R visitGraphLabelDisj(GraphLabel.Disj node, C ctx);
+public abstract class AstVisitor<R, C> {


For all of these, I'd indicate in the JVM signature and Javadocs that it might throw an IllegalStateException if the variant is not officially supported. Maybe link to some documentation about how one can successfully add a new variant without blowing up their visitors.

Replacing the instanceOf checking with the node.accept(...) calls for the sum type visit methods (from your comment) would get rid of the IllegalStateExceptions.

I'll add docs and a usage guide in a subsequent PR once the APIs are more finalized.

johnedquinn · 2024-10-31T21:02:58Z

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

+        if (node instanceof Query) {
+            return visitQuery((Query) node, ctx);
+        } else if (node instanceof Explain) {
+            return visitExplain((Explain) node, ctx);
+        } else {
+            throw new IllegalStateException("Unexpected value: " + node);
+        }


I'm trying to think about how we can make it easier for users to add their own custom nodes without breaking their visitors completely. Is there something wrong with just:

Suggested change

if (node instanceof Query) {

return visitQuery((Query) node, ctx);

} else if (node instanceof Explain) {

return visitExplain((Explain) node, ctx);

} else {

throw new IllegalStateException("Unexpected value: " + node);

}

node.accept(this, ctx)

Theoretically, each variant of a sum type would look like:

public class Query : AstNode() { @Override public <R, C> R accept(@NotNull AstVisitor<R, C> visitor, C ctx) { return visitor.visitQuery(this, ctx); } }

So, it would definitely work for the built-in variants. For the other variants, I'm not entirely sure how they'd be able to invoke a custom visit (AKA visitor.visitCustomNode(this, ctx)).

You're right about the simplification. Those sum type visit functions can be simplified to node.accept(this, ctx). I will change.

From offline discussion, there's pros/cons to both approaches. There are some drawbacks to the node.accept default for the sum types, primarily a user of this library would need to be careful with overriding the proper methods when adding their own variants. This can hopefully be resolved by including some good usage guides.

I do like your suggested approach a bit more since it will simplify what we (the PartiQL team) need to add when adding a new variant.

partiql-planner/src/main/kotlin/org/partiql/planner/internal/normalize/Normalize.kt

johnedquinn · 2024-10-31T21:10:14Z

partiql-ast/src/main/java/org/partiql/ast/v1/AstRewriter.kt

+import org.partiql.ast.v1.graph.GraphSelector
+import org.partiql.value.PartiQLValueExperimental
+
+public abstract class AstRewriter<C> : AstVisitor<AstNode, C> {


I can check the decompiled bytecode, but this resides in the java source set btw.

…passes to partiql-planner (#1629)

alancai98 requested a review from RCHowell October 23, 2024 23:55

alancai98 self-assigned this Oct 23, 2024

alancai98 force-pushed the v1-migrate-ast-astvisitor branch 2 times, most recently from fd6c81a to 8c863a0 Compare October 24, 2024 00:02

alancai98 commented Oct 24, 2024

View reviewed changes

alancai98 force-pushed the v1-migrate-ast-parser branch from 7a68858 to 8eae47f Compare October 24, 2024 00:37

alancai98 force-pushed the v1-migrate-ast-astvisitor branch from 8c863a0 to eeae1fd Compare October 24, 2024 00:38

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

alancai98 force-pushed the v1-migrate-ast-astvisitor branch from eeae1fd to 1154b30 Compare October 24, 2024 00:47

alancai98 force-pushed the v1-migrate-ast-astvisitor branch 2 times, most recently from 238ef60 to 055db0d Compare October 24, 2024 17:51

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-astvisitor branch from 055db0d to cb9dd8a Compare October 24, 2024 18:50

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

alancai98 force-pushed the v1-migrate-ast-astvisitor branch from cb9dd8a to 3688156 Compare October 25, 2024 22:24

alancai98 mentioned this pull request Oct 26, 2024

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

Closed

23 tasks

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

alancai98 force-pushed the v1-migrate-ast-astvisitor branch from 3688156 to cf6a2fa Compare October 28, 2024 17:58

Base automatically changed from v1-migrate-ast-parser to v1 October 28, 2024 23:14

alancai98 marked this pull request as draft October 28, 2024 23:33

alancai98 force-pushed the v1-migrate-ast-astvisitor branch from cf6a2fa to 23c1a3c Compare October 30, 2024 22:58

alancai98 marked this pull request as ready for review October 30, 2024 23:02

alancai98 commented Oct 30, 2024

View reviewed changes

[v1] Add AstVisitor and AstRewriter; port partiql-ast normalization p…

1bb9b21

…asses to partiql-planner

alancai98 force-pushed the v1-migrate-ast-astvisitor branch from 23c1a3c to 1bb9b21 Compare October 31, 2024 18:38

alancai98 changed the title ~~[v1] Add defaults for AstVisitor; port partiql-ast normalization passes to partiql-planner~~ [v1] Adds AstVisitor and AstRewriter; port partiql-ast normalization passes to partiql-planner Oct 31, 2024

alancai98 requested a review from johnedquinn October 31, 2024 18:42

johnedquinn reviewed Oct 31, 2024

View reviewed changes

Change sum variant visit methods

16f56ba

alancai98 requested a review from johnedquinn October 31, 2024 22:16

johnedquinn approved these changes Oct 31, 2024

View reviewed changes

alancai98 merged commit 47de56f into v1 Oct 31, 2024
14 checks passed

alancai98 deleted the v1-migrate-ast-astvisitor branch October 31, 2024 22:38

RCHowell pushed a commit that referenced this pull request Nov 1, 2024

[v1] Adds AstVisitor and AstRewriter; port partiql-ast normalization …

4016203

…passes to partiql-planner (#1629)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v1] Adds AstVisitor and AstRewriter; port partiql-ast normalization passes to partiql-planner #1629

[v1] Adds AstVisitor and AstRewriter; port partiql-ast normalization passes to partiql-planner #1629

alancai98 commented Oct 23, 2024 •

edited

Loading

alancai98 Oct 23, 2024

github-actions bot commented Oct 24, 2024 •

edited

Loading

codecov-commenter commented Oct 24, 2024 •

edited

Loading

alancai98 commented Oct 28, 2024

alancai98 Oct 30, 2024

johnedquinn Oct 31, 2024

alancai98 Oct 31, 2024

johnedquinn Oct 31, 2024

alancai98 Oct 31, 2024

johnedquinn Oct 31, 2024

johnedquinn Oct 31, 2024

alancai98 Oct 31, 2024 •

edited

Loading

alancai98 Oct 31, 2024

johnedquinn Oct 31, 2024

[v1] Adds AstVisitor and AstRewriter; port partiql-ast normalization passes to partiql-planner #1629

[v1] Adds AstVisitor and AstRewriter; port partiql-ast normalization passes to partiql-planner #1629

Conversation

alancai98 commented Oct 23, 2024 • edited Loading

Relevant Issues

Description

Other Information

License Information

Choose a reason for hiding this comment

github-actions bot commented Oct 24, 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

codecov-commenter commented Oct 24, 2024 • edited Loading

Codecov Report

alancai98 commented Oct 28, 2024

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 Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alancai98 commented Oct 23, 2024 •

edited

Loading

github-actions bot commented Oct 24, 2024 •

edited

Loading

codecov-commenter commented Oct 24, 2024 •

edited

Loading

alancai98 Oct 31, 2024 •

edited

Loading