[v1] Change public fields to Lombok getters; make concrete classes final #1685

alancai98 · 2024-12-18T01:44:23Z

Relevant Issues

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

Description

Makes the public AST fields private and adds Lombok getters
Makes existing concrete classes final

Other Information

Updated Unreleased Section in CHANGELOG: [NO]
- No, v1 release not out yet.
Any backward-incompatible changes? [YES]
- Yes, but v1 release not out yet.
Any new external dependencies? [YES]
- Yes, Kotlin lombok plugin to allow for using Java lombok annotations from Kotlin (required for AstRewriter and SqlDialect)
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-12-18T01:50:49Z

CROSS-ENGINE-REPORT ❌

	BASE (LEGACY-V0.14.8)	TARGET (EVAL-28DB2B2)	+/-
% Passing	89.67%	94.32%	4.65% ✅
Passing	5287	5561	274 ✅
Failing	609	54	-555 ✅
Ignored	0	281	281 🔶
Total Tests	5896	5896	0 ✅

Testing Details

Base Commit: v0.14.8
Base Engine: LEGACY
Target Commit: 28db2b2
Target Engine: EVAL

Result Details

❌ REGRESSION DETECTED. See Now Failing/Ignored Tests. ❌
Passing in both: 2641
Failing in both: 17
Ignored in both: 0
PASSING in BASE but now FAILING in TARGET: 5
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 5 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
inPredicateWithTableConstructor, compileOption: PERMISSIVE
notInPredicateWithTableConstructor, 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-28DB2B2). 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-EA779EE)	TARGET (EVAL-28DB2B2)	+/-
% Passing	94.32%	94.32%	0.00% ✅
Passing	5561	5561	0 ✅
Failing	54	54	0 ✅
Ignored	281	281	0 ✅
Total Tests	5896	5896	0 ✅

Testing Details

Base Commit: ea779ee
Base Engine: EVAL
Target Commit: 28db2b2
Target Engine: EVAL

Result Details

Passing in both: 5561
Failing in both: 54
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-12-18T19:24:05Z

partiql-ast/src/main/java/org/partiql/ast/sql/SqlDialect.kt

+        val asAlias = node.asAlias
+        t = if (asAlias != null) t concat " GROUP AS ${asAlias.sql()}" else t


(self-review): previously, when we used public fields, we could use the smart-casting of these nullable fields to non-null values (e.g. call node.asAlias.sql() directly). Since we switch to use getters, will need to define a variable first that we perform null-checks on.

alancai98 · 2024-12-18T19:24:51Z

partiql-ast/src/main/java/org/partiql/ast/Explain.java

@@ -13,12 +14,14 @@
 */
 @Builder(builderClassName = "Builder")
 @EqualsAndHashCode(callSuper = false)
-public class Explain extends Statement {
+public final class Explain extends Statement {


(self-review): change all of the concrete classes to be final.

alancai98 · 2024-12-18T19:33:45Z

partiql-ast/src/main/java/org/partiql/ast/sql/SqlDialect.kt

@@ -394,19 +396,20 @@ public abstract class SqlDialect : AstVisitor<SqlBlock, SqlBlock>() {
    override fun visitExprLike(node: ExprLike, tail: SqlBlock): SqlBlock {
        var t = tail
        t = visitExprWrapped(node.value, t)
-        t = t concat if (node.not) " NOT LIKE " else " LIKE "
+        t = t concat if (node.isNot) " NOT LIKE " else " LIKE "


(self-review): Lombok's default getter for boolean (not the boxed Boolean; boxed will still use get) will use the is prefix rather than get. Stylistically, is is

more consistent with what we do in the plan already (e.g. RelUnion's isAll)

default for IntelliJ when creating new getters from private fields

if we care about conventions, JavaBeans prescribes using is over get

This change applies to other places we had used a boolean getter (e.g. identifier's delimited).

alancai98 · 2024-12-18T19:37:47Z

partiql-ast/build.gradle.kts

@@ -16,6 +16,9 @@
 plugins {
    id(Plugins.conventions)
    id(Plugins.publish)
+    // Need the Kotlin lombok plugin to allow for Kotlin code in partiql-ast to understand Java Lombok annotations.
+    // https://kotlinlang.org/docs/lombok.html
+    id(Plugins.kotlinLombok) version Versions.kotlinLombok


(self-review): Kotlin lombok plugin required to allow Kotlin code to understand Java lombok annotations -- https://kotlinlang.org/docs/lombok.html. It is experimental but

seems pretty stable so far since its published by the Kotlin team

we could always use a fixed version or hand-write the getters if we encounter issues without breaking the public API

codecov-commenter · 2024-12-26T19:48:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.03%. Comparing base (d059c81) to head (73e25c5).
Report is 444 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1685      +/-   ##
============================================
+ Coverage     73.16%   80.03%   +6.87%     
+ Complexity     2393       47    -2346     
============================================
  Files           247       19     -228     
  Lines         17627      506   -17121     
  Branches       3178       23    -3155     
============================================
- Hits          12896      405   -12491     
+ Misses         3854       88    -3766     
+ Partials        877       13     -864

Flag	Coverage Δ
CLI	`?`
EXAMPLES	`80.03% <ø> (-0.04%)`	⬇️
LANG	`?`

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 self-assigned this Dec 18, 2024

alancai98 mentioned this pull request Dec 18, 2024

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

Closed

23 tasks

alancai98 commented Dec 18, 2024

View reviewed changes

alancai98 changed the title ~~[draft] [v1] Change public fields to Lombok getters; make concrete classes final~~ [v1] Change public fields to Lombok getters; make concrete classes final Dec 18, 2024

alancai98 marked this pull request as ready for review December 18, 2024 19:39

alancai98 added 4 commits December 26, 2024 14:42

[v1] Change public fields to Lombok getters; make concrete classes final

e380259

Make boolean getters use 'is' prefix

b232dc7

Change Kotlin lombok plugin version

12cdf50

Add remaining public fields

73e25c5

alancai98 force-pushed the lombok-getters branch from 4c7f12a to 73e25c5 Compare December 26, 2024 19:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v1] Change public fields to Lombok getters; make concrete classes final #1685

[v1] Change public fields to Lombok getters; make concrete classes final #1685

alancai98 commented Dec 18, 2024 •

edited

Loading

github-actions bot commented Dec 18, 2024 •

edited

Loading

alancai98 Dec 18, 2024

alancai98 Dec 18, 2024

alancai98 Dec 18, 2024

alancai98 Dec 18, 2024

codecov-commenter commented Dec 26, 2024

		val asAlias = node.asAlias
		t = if (asAlias != null) t concat " GROUP AS ${asAlias.sql()}" else t

[v1] Change public fields to Lombok getters; make concrete classes final #1685

Are you sure you want to change the base?

[v1] Change public fields to Lombok getters; make concrete classes final #1685

Conversation

alancai98 commented Dec 18, 2024 • edited Loading

Relevant Issues

Description

Other Information

License Information

github-actions bot commented Dec 18, 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

alancai98 Dec 18, 2024

Choose a reason for hiding this comment

alancai98 Dec 18, 2024

Choose a reason for hiding this comment

alancai98 Dec 18, 2024

Choose a reason for hiding this comment

alancai98 Dec 18, 2024

Choose a reason for hiding this comment

codecov-commenter commented Dec 26, 2024

Codecov Report

alancai98 commented Dec 18, 2024 •

edited

Loading

github-actions bot commented Dec 18, 2024 •

edited

Loading