-
Notifications
You must be signed in to change notification settings - Fork 62
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] Add name method to AstEnum #1630
Conversation
default: return "UNKNOWN"; | ||
} | ||
} | ||
|
||
@NotNull | ||
private static final int[] codes = { |
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.
(self-review): I re-arranged the values in this PR to align with the order of the data type constants defined above https://github.com/partiql/partiql-lang-kotlin/blob/v1-migrate-ast/partiql-ast/src/main/java/org/partiql/ast/v1/DataType.java#L12-L71.
private static final int UNKNOWN = 0; | ||
private static final int UNION = 1; | ||
private static final int INTERSECT = 2; | ||
private static final int EXCEPT = 3; |
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.
(self-review) these were inadvertently private
public static final int LAG = 0; | ||
public static final int LEAD = 0; |
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.
(self-review) these enum variants were inadvertently all 0
. Adding the name
method to AstEnum
allowed us to catch this bug.
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-7CF93FD). 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
|
Relevant Issues
Description
.name
method to everyAstEnum
. This is part of normal enum classes but was missing from ourAstEnum
abstract class definition.AstEnum
implementation fixesOther 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.