[v1] Add AST factory methods; some fixes to AST classes #1622
Conversation
CROSS-ENGINE-REPORT ❌
Testing Details
Result Details
Now FAILING Tests ❌The following 1 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 Tests179 test(s) were previously failing in BASE (LEGACY-V0.14.8) but now pass in TARGET (EVAL-7419125). 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
|
| @@ -36,6 +36,8 @@ public <R, C> R accept(@NotNull AstVisitor<R, C> visitor, C ctx) { | |||
| */ | |||
| @Builder(builderClassName = "Builder") | |||
| public static class AnyShortest extends GraphSelector { | |||
| public AnyShortest() {} | |||
There was a problem hiding this comment.
(self-review) some missing constructors in the graph/ directory.
| @@ -149,7 +149,7 @@ public interface AstVisitor<R, C> { | |||
|
|
|||
| R visitSelectItem(SelectItem node, C ctx); | |||
|
|
|||
| R visitSelectItemAll(SelectItem.Star node, C ctx); | |||
There was a problem hiding this comment.
(self-review) I forgot to change naming of this visitor function after the class was renamed.
| @NotNull | ||
| public final Long lower; |
There was a problem hiding this comment.
(self-review) type could just be primitive long.
| @NotNull | ||
| public static ExprNot exprNot(@NotNull Expr value) { | ||
| return new ExprNot(value); | ||
| } |
There was a problem hiding this comment.
(self-review) for PartiQL expression NOT false,
example usage from Kotlin
exprNot(exprLit(boolValue(false)))example usage from Java
exprNot(exprLit(boolValue(false)));
| @NotNull | ||
| public static ExprAnd exprAnd(Expr lhs, Expr rhs) { | ||
| return new ExprAnd(lhs, rhs); | ||
| } |
There was a problem hiding this comment.
One potentially annoying thing I've noticed about these factory methods being written in Java is that they cannot be called from Kotlin with named parameters.
E.g. below results in an error "Named arguments are not allowed for non-Kotlin functions"
identifier(
symbol = ctx.IDENTIFIER_QUOTED().getStringValue(),
isDelimited = true
)
Wondering if we should actually have these factory methods written in Kotlin instead?
There was a problem hiding this comment.
I'll bring up next week w/ the team if these should be written in Kotlin.
Relevant Issues
Description
visitSelectItemAll->visitSelectItemStar@NotNull Long->longOther Information
Updated Unreleased Section in CHANGELOG: [NO]
Any backward-incompatible changes? [YES]
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.