[v1] Remove CAN_CAST and CAN_LOSSLESS_CAST from partiql-ast
#1544
Conversation
CAN_CAST and CAN_LOSSLESS_CAST from partiql-ast
1f263b3 to
895a3a8
Compare
| // PartiQL special form `CAN_CAST` | ||
| can_cast::{ | ||
| value: expr, | ||
| as_type: '.type', | ||
| }, | ||
|
|
||
| // PartiQL special form `CAN_LOSSLESS_CAST` | ||
| can_lossless_cast::{ | ||
| value: expr, | ||
| as_type: '.type', | ||
| }, | ||
|
|
There was a problem hiding this comment.
(self-review): I'm not sure of a way we can mark these partiql-ast nodes as deprecated and then remove them in a subsequent release. In the PR's current state, it will remove the nodes and corresponding partiql-ast code within the v1 branch.
There was a problem hiding this comment.
from offline discussion, PartiQL team will remove these partiql-ast nodes from the v1 branch.
| @@ -131,6 +131,7 @@ abstract class CastTestBase : EvaluatorTestBase() { | |||
| } | |||
| override fun toString(): String = "$expression -> ${expected ?: expectedErrorCode}" | |||
|
|
|||
| // TODO: delete these tests ahead of `v1` release | |||
There was a problem hiding this comment.
(self-review): Within the legacy code of partiql-lang, I didn't remove anything just yet. Currently just marks parts of the code in partiql-lang that we will remove in the v1 release.
There was a problem hiding this comment.
from offline discussion, we'll leave existing tests as-is. They will be deleted in the v1 release.
| @@ -623,8 +623,8 @@ exprPrimary | |||
| | substring # ExprPrimaryBase | |||
| | position # ExprPrimaryBase | |||
| | overlay # ExprPrimaryBase | |||
| | canCast # ExprPrimaryBase | |||
| | canLosslessCast # ExprPrimaryBase | |||
| | canCast # ExprPrimaryBase // TODO remove ahead of `v1` release | |||
There was a problem hiding this comment.
(self-review) Was not sure if we wanted to remove the partiql-parser logic just yet or wait until after we've deprecated everything in partiql-lang before removing.
There was a problem hiding this comment.
from offline discussion, we'll keep the parsing rules in the grammar in place for now. Eventually, the rules in the parser and lexer will be removed in the v1 branch. We will have the following behavior for the ANTLR visitors:
- partiql-parser ANTLR visitor (parsing to partiql-ast) -- will now error when visiting the
canCastandcanLosslessCastnodes - partiql-lang ANTLR visitor (parsing to PIG-generated AST) -- leave as-is
CAN_CAST and CAN_LOSSLESS_CAST from partiql-astCAN_CAST and CAN_LOSSLESS_CAST from partiql-ast
CAN_CAST and CAN_LOSSLESS_CAST from partiql-astCAN_CAST and CAN_LOSSLESS_CAST from partiql-ast
CROSS-ENGINE Conformance Report ❌
Testing DetailsResult Details
Now Failing Tests ❌The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact. Now Passing Tests492 test(s) were previously failing in BASE (LEGACY-8A68AE4) but now pass in TARGET (EVAL-8A68AE4). 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-LEGACY Conformance Report ✅
Testing DetailsResult Details
CROSS-COMMIT-EVAL Conformance Report ✅
Testing DetailsResult Details
|
895a3a8 to
706bd54
Compare
CAN_CAST and CAN_LOSSLESS_CAST from partiql-astCAN_CAST and CAN_LOSSLESS_CAST from partiql-ast
| val type = visitAs<Type>(ctx.type()) | ||
| exprCanCast(expr, type) | ||
| throw error(ctx, "CAN_CAST is no longer supported in the default PartiQLParser") | ||
| } | ||
|
|
||
| override fun visitCanLosslessCast(ctx: GeneratedParser.CanLosslessCastContext) = translate(ctx) { | ||
| val expr = visitExpr(ctx.expr()) | ||
| val type = visitAs<Type>(ctx.type()) | ||
| exprCanLosslessCast(expr, type) | ||
| throw error(ctx, "CAN_LOSSLESS_CAST is no longer supported in the default PartiQLParser") |
There was a problem hiding this comment.
(self-review): erroring on canCast and canLosslessCast in the partiql-parser ANTLR visitor does not result in any parsing test failures. As we alter the parsing behavior for other nodes we want to delete from partiql-ast, we may encounter some parsing test failures. We will need some skip-list or other mechanism to
- not run the PartiQLParserDefault on those tests
- continue to support parsing w/ the PartiQLPigVisitor
706bd54 to
4dfbb98
Compare
4dfbb98 to
5bf5aeb
Compare
Relevant Issues
Description
partiql-ast-- removesCAN_CASTandCAN_LOSSLESS_CASTnodespartiql-parsergrammar andpartiql-langtests for parts we want to remove ahead of thev1releaseOther Information
Updated Unreleased Section in CHANGELOG: [NO]
v1branch.Any backward-incompatible changes? [YES]
CAN_CASTandCAN_LOSSLESS_CASTnodes from thepartiql-astAny 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.