Jvm field ir

[RCHowell]

Description

This PR improves the efficiency and simplicity of generated IR data structures. As we've been debugging larger and larger trees, we have noticed some hidden bloat that comes with using Kotlin over Java — ie each Kotlin field has a private java field along with public getter/setter. The solution is to mark each field as @JvmField, but this is not quite as simple as it sounds in our case.

For us, the IR is defined by an interface with internal implementations. A Kotlin interface can have fields (because they're really Java methods) which an implementation overrides/implements. These are non-final fields because they're actually method implementations; but the syntax is deceiving. You cannot mark non-final fields as @JvmField which means we can't actually use interfaces here.

But these brings light to Vladimir's suggestion; why bother with all the machinery? It's indeed simpler and more efficient to use data classes with @JvmField on all parameters. As others have suggested, we could have avoid the shenanigans with Java and Lombok 😉

There are three changes here that are minimal in practice but have nice access efficiency implications.

1. Each field becomes a final @JvmField (+1 access efficiency)
2. Using data class to get Kotlin's official hashcode/equals (+1 maintainability); we also get destructuring
3. Remove factory/abstract factory in favor of top-level constructor functions (+1 simplicity/ergonomics)

While the factory is gone, we keep the Java style builders and Kotlin type-safe DSL.

// Before

object Ast : AstFactoryBase() {}

abstract class Foo : Node {
	abstract val x: String
	abstract val y: Bool
}

open class FooImpl internal constructor(
	override val x: String,
	override val y: Bool,
) : Foo() {

   // our own copy/hashcode/equals/toString
}

Ast.foo(...)            // 1. construct with factory
Ast.create { foo(...) } // 2. factory in scope


// After, reduction

data class Foo(
	@JvmField val x: String,
	@JvmField val y: Bool
) : Node()
// inherit copy/hashcode/equals/toString/componentN

// Ast.kt
Foo("x", true)   // 1. construct directly, no indirection
foo("y", false)  // 2. construct with "factory" method

This second way of doing things reduces the complexity of the Java API while maintaining the fluent builders.

Breaking Change (Kotlin only)

This is a very minimal breaking change. It's as simple as removing the factory in node construction and importing the top-level function. The interfaces are the exact same; only node construction has changed.

// before

import org.partiql.ast.Ast

val a = Ast.foo("x", false)

// after

import org.partiql.ast.foo

val a = foo("x", false)

The symbols from the Java API don't change, but instead of factory methods called on a singleton object, they are static methods.

Other Information

• Updated Unreleased Section in CHANGELOG: [YES/NO]
  Yes

• Any backward-incompatible changes? [YES/NO]
  Yes, the factory class in no longer necessary.

• Any new external dependencies? [YES/NO]
  No

• Do your changes comply with the Contributing Guidelines
  and Code Style Guidelines? [YES/NO]
  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]

Conformance comparison report

	Base (a3199cf)	0839d3c	+/-
% Passing	92.33%	92.33%	0.00%
✅ Passing	5372	5372	0
❌ Failing	446	446	0
🔶 Ignored	0	0	0
Total Tests	5818	5818	0

Number passing in both: 5372

Number failing in both: 446

Number passing in Base (a3199cf) but now fail: 0

Number failing in Base (a3199cf) but now pass: 0

[johnedquinn]

Very clear PR. Great work. There were some straggling comments and a file potentially left behind. I can approve once cleaned.

[johnedquinn]

Is this different from the ParserBenchmark? Was this meant to be added?

[RCHowell]

Yes it includes the org.partiql.parser implementation.

[codecov-commenter]

Codecov Report

Attention: 33 lines in your changes are missing coverage. Please review.

Comparison is base (a3199cf) 71.48% compared to head (ffdc968) 71.48%.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #1242   +/-   ##
=========================================
  Coverage     71.48%   71.48%           
  Complexity     2444     2444           
=========================================
  Files           229      229           
  Lines         17595    17595           
  Branches       3244     3244           
=========================================
  Hits          12577    12577           
  Misses         4010     4010           
  Partials       1008     1008           

Flag	Coverage Δ
CLI	13.70% <ø> (ø)
EXAMPLES	80.28% <ø> (ø)
LANG	78.62% <65.26%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...n/org/partiql/lang/planner/transforms/AstToPlan.kt	88.00% <100.00%> (ø)
.../partiql/lang/planner/transforms/plan/PlanTyper.kt	60.95% <61.53%> (ø)
...rtiql/lang/planner/transforms/plan/RelConverter.kt	70.04% <70.37%> (ø)
...rtiql/lang/planner/transforms/plan/RexConverter.kt	63.15% <62.96%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.