Initializes Datum and adds functionality to evaluator #1451

johnedquinn · 2024-04-27T16:57:43Z

Relevant Issues

None

Description

Initializes Datum, the engine's representation of a value
Also and adds functionality to evaluator. Currently, Datum is all over the evaluator, however, we convert to PartiQLValue when calling UDFs and doing comparisons.
While working on the JOIN PR (Simplifies joins and fixes bugs #1438) and PartiQL Cursor PR (Initializes PartiQLData and PartiQLValueLoader #1443), it became apparent to me that our modelling of PartiQLValue is tightly coupled with Java's type semantics. In many places, we rely on Java's types (example: x is StructValue<*>) rather than relying on our plumbing: (x.type == PartiQLValueType.STRUCT). On top of that, as part of the JOIN PR, I found it incredibly difficult (and sub-optimal performance-wise) to coerce nulls of a particular type to a null of another type. This PR takes these learnings and completely overhauls PartiQLValue to a representation that we are all familiar with: Ion's AnyElement. In a way, this modelling is as close as we can get to C's Union type, and by modelling it this way, we gain control over our type system.
Notice that the API proposed is called PQLValue. I intend for its core to be copied and pasted directly into PartiQLValue. I have also written this in Java, as I've noticed our inability to control our public API by writing in Kotlin. On top of that, PQLValue is written in Java to get us closer to using primitives directly. That being said, PQLValue will NOT exist during the V1 release. Its contents will replace the contents of PartiQLValue -- and PartiQLValue will be written in Java.

Internal Notes

Notice that I attempt to keep the null-safety that Kotlin provides by marking all applicable API's with @NotNull.
Similarly, I attempt to use primitives as much as possible -- limiting our memory footprint and maintaining the null-safety.
I've left remaining action items to fully replace PartiQLValue in the KDocs of PQLValue. These include adding the comparator, equals, hashCode, etc. These should be able to be added fairly quickly.

Other Information

Updated Unreleased Section in CHANGELOG: NO
Any backward-incompatible changes? NO -- not really, however, PQLValue intends to replace PartiQLValue
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.

partiql-eval/src/main/java/org/partiql/eval/BagValue.java

rchowell · 2024-05-06T22:31:22Z

partiql-eval/src/main/java/org/partiql/eval/CharValue.java

+class CharValue implements PQLValue {
+
+    @NotNull
+    final String _value;


Suggested change

final String _value;

final char _value;

char primitive is 16-bit unicode

The character data type in SQL doesn't represent the char in Java. It's more like a char array

Oh the fixed-length character. In my head I had

char <-> char(1) -- fixed-length character with length 1

partiql-eval/src/main/java/org/partiql/eval/DateValue.java

partiql-eval/src/main/java/org/partiql/eval/DecimalArbitraryValue.java

rchowell · 2024-05-06T22:54:06Z

partiql-eval/src/main/java/org/partiql/eval/PQLValue.java

+    @NotNull
+    static PQLValue nullValue(@NotNull PartiQLValueType type) {
+        return new NullValue(type);
+    }


Hmm I'm not sure about typed nulls if we are going ahead and changing the value system.

In order to pass existing tests, this is necessary. I can mark it as deprecated, but for this PR I'd like to avoid removing PartiQLValueType.NULL as it touches many things

See PR #1463. This will have a pretty significant impact on this comment.

rchowell · 2024-05-06T22:57:55Z

partiql-eval/src/main/java/org/partiql/eval/StructField.java

+/**
+ * Represents the key-value pairs that are embedded within values of type {@link org.partiql.value.PartiQLValueType#STRUCT}.
+ */
+public interface StructField {


Why an interface?

Struct fields should have just two values,

- name -> field name original casing - value -> field value

We should also consider private, but non-final field members

partiql-eval/src/main/java/org/partiql/eval/StructValue.java

partiql-eval/src/main/kotlin/org/partiql/eval/internal/Compiler.kt

rchowell · 2024-05-06T23:27:50Z

partiql-eval/src/main/kotlin/org/partiql/eval/internal/helpers/ValueUtility.kt

+    @OptIn(PartiQLValueExperimental::class)
+    fun PQLValue.getInt32Coerced(): Int {
+        return when (this.type) {
+            PartiQLValueType.INT8 -> this.int8Value.toInt()
+            PartiQLValueType.INT16 -> this.int16Value.toInt()
+            PartiQLValueType.INT32 -> this.int32Value
+            PartiQLValueType.INT64 -> this.int64Value.toInt()
+            PartiQLValueType.INT -> this.intValue.toInt()
+            else -> throw TypeCheckException()
+        }
+    }


I suggest adding all of these coerces to the PQLValue implementations.

class Int32Value ... { private Int value; @Override short toShort() { // throw DataException on overflow etc. return value as short; } @Override int getInt() { return value }; @Override long getLong() { return value }; }

See 7f34544#diff-7273fc22dadb098a56a8f9e14afb134d7fdce01af8709eff841f91576bee9d3e

codecov-commenter · 2024-06-03T17:14:10Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (v1@607c4c0). Learn more about missing BASE report.

Additional details and impacted files

@@          Coverage Diff          @@
##             v1    #1451   +/-   ##
=====================================
  Coverage      ?   72.30%           
  Complexity    ?     2539           
=====================================
  Files         ?      283           
  Lines         ?    20282           
  Branches      ?     3700           
=====================================
  Hits          ?    14664           
  Misses        ?     4596           
  Partials      ?     1022

Flag	Coverage Δ
CLI	`13.71% <ø> (?)`
EXAMPLES	`80.07% <ø> (?)`
LANG	`77.69% <ø> (?)`

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.

rchowell · 2024-06-04T00:45:45Z

partiql-eval/src/main/java/org/partiql/eval/value/DecimalArbitraryValue.java

+/**
+ * This shall always be package-private (internal).
+ */
+class DecimalArbitraryValue implements PQLValue {


I don't think we need both Decimal and DecimalArbitrary for runtime values.

rchowell · 2024-06-04T00:51:37Z

partiql-eval/src/main/java/org/partiql/eval/value/Float32Value.java

+class Float32Value implements PQLValue {
+
+    private final float _value;
+
+    Float32Value(float value) {
+        _value = value;
+    }
+
+    @Override
+    public boolean isNull() {
+        return false;
+    }
+
+    @Override
+    public float getFloat() {
+        return _value;
+    }
+


This is starting to look exactly like PartiQLValue just in Java and with the getFloat() .. getLong() etc. I have a couple of questions.

What's the difference now? (besides flat class hierarchy)

Why don't we have the coercions in the PQLValue? (in this case getDouble())

Perhaps its been too long since I looked, but I don't think I understand the value prop here.

Yes, this PR aims to replace PartiQLValue incrementally. I'd suggest:

Add PQLValue without releasing.

Update functions to use PQLValue. (subsequent PR)

Move the existing PartiQLValue to the plan package (maybe called PlanLiteral) (since it's use is more for the DOM)

Rename PQLValue to PartiQLValue

The difference is that we are moving away from using Java's type semantics in favor of our own type system. With this, we can/should be using our own type information to determine how to extract the underlying data. The idea is that this logic should be exposed by partiql-eval instead of partiql-types.

There shouldn't be a need for coercions that are not represented in the plan. What I'd like to see is that when the executor receives a plan, without any modification, the executor shall execute the plan. If a JVM double is expected, then the planner should have inserted the appropriate coercion.

Consider Rex.Op.Path.Index. The planner currently allows all integer types as the index, and then the executor is actually under the hood coercing any integer value into a JVM int. In my opinion, the "function signature" of the path index should have required that the index be a PartiQL int_32. With this mindset, the expressions representing the index (say its typed to be an INT16) would be wrapped with a coercion by the planner. We're currently making up for our lack of planning by adding all of these additional coercions in the execution.

Discussed other points offline to create more distance from DOM.

There shouldn't be a need for coercions that are not represented in the plan. What I'd like to see is that when the executor receives a plan, without any modification, the executor shall execute the plan. If a JVM double is expected, then the planner should have inserted the appropriate coercion.

Agreed, makes sense now. Thank you.

rchowell · 2024-06-04T00:53:19Z

partiql-eval/src/main/java/org/partiql/eval/value/PQLValue.java

+ * - The comparator for ordering and aggregations
+ * - Adding support for annotations
+ */
+public interface PQLValue extends Iterable<PQLValue> {


I don't think it should be public if it's not a replacement. I also thought we discussed them solving different problems?

We shouldn't introduce another duplication of something existing when PartiQLValue is still under experimental. We either replace it completely or change it however we need to.

rchowell · 2024-06-04T00:54:45Z

partiql-eval/src/main/java/org/partiql/eval/value/PQLValue.java

+     * @return the underlying value applicable to the types:
+     * {@link PartiQLValueType#INT32}
+     * @throws UnsupportedOperationException if the operation is not applicable to the type returned from


I'm confused why coercions wouldn't fit like Int16Value

rchowell · 2024-06-04T00:56:53Z

partiql-eval/src/main/java/org/partiql/eval/value/BagValue.java

+/**
+ * This shall always be package-private (internal).
+ */
+class BagValue implements PQLValue {


I don't think we need Bag/List/Sexp classes if we have a type enum. Because this is always internal, we can add additional classes later, but it doesn't provide additional value. Same for Decimal/DecimalArbitrary.

rchowell · 2024-06-04T00:58:54Z

partiql-eval/src/main/java/org/partiql/eval/value/CharValue.java

+/**
+ * This shall always be package-private (internal).
+ */
+class CharValue implements PQLValue {


Similar comment, but seems we don't need Char and String considering their identical implementations.

rchowell · 2024-06-04T00:59:12Z

partiql-eval/src/main/java/org/partiql/eval/value/ClobValue.java

+/**
+ * This shall always be package-private (internal).
+ */
+class ClobValue implements PQLValue {


Clob/Blob -> ByteArray or Lob

Updates all implementing classes of Datum

rchowell

Only change is to fix the test/partiql-tests submodule otherwise ship it.

rchowell · 2024-06-07T17:43:49Z

partiql-eval/src/main/kotlin/org/partiql/eval/internal/helpers/RecordValueIterator.kt

 internal class RecordValueIterator(
-    collectionValue: CollectionValue<*>
+    collectionValue: Iterator<Datum>
 ) : Iterator<Record> {

    private val collectionIter = collectionValue.iterator()


Nit, but now we can have private val iterator: Iterator<Dataum>

johnedquinn · 2024-06-11T20:43:05Z

Only change is to fix the test/partiql-tests submodule otherwise ship it.

I believe there is a bug in GitHub's rendering of the submodule diff. For some reason, the commit is the same on v1 as on johnedquinn:v1-eval-value-rebased. Checked the files being "changed" and they're all already in v1. I pushed twice to move to some older submodule commit and then back to what is currently on v1, but it's still not rendering appropriately.

johnedquinn force-pushed the v1-eval-value-rebased branch from 4ab73d6 to eee755f Compare April 27, 2024 17:01

johnedquinn mentioned this pull request Apr 27, 2024

Simplifies joins and fixes bugs #1438

Merged

johnedquinn force-pushed the v1-eval-value-rebased branch 2 times, most recently from 5d9957f to d25de2d Compare April 30, 2024 22:52

Initializes PQLValue and adds functionality to evaluator

d201667

johnedquinn force-pushed the v1-eval-value-rebased branch from d25de2d to d201667 Compare April 30, 2024 23:19

johnedquinn marked this pull request as ready for review May 1, 2024 21:40

johnedquinn mentioned this pull request May 2, 2024

[TRACKER] Up-to-date Conformance Tracker #1433

Closed

johnedquinn requested a review from rchowell May 6, 2024 15:42

rchowell suggested changes May 6, 2024

View reviewed changes

johnedquinn added 5 commits May 17, 2024 12:03

Makes all value's properties private

3e7325c

Temporary: Extends Iterable

6db2563

Simplifies iteration and structs

7f34544

Updates methods to use JVM names

bab2574

Moves value classes to org.partiql.eval.value

6f28be3

rchowell suggested changes Jun 4, 2024

View reviewed changes

Updates name to Datum

33a3019

Updates all implementing classes of Datum

johnedquinn changed the title ~~Initializes PQLValue and adds functionality to evaluator~~ Initializes Datum and adds functionality to evaluator Jun 5, 2024

rchowell suggested changes Jun 7, 2024

View reviewed changes

johnedquinn added 2 commits June 11, 2024 13:29

Revert submodule change for GH display bug

3b732fc

Specifies commit currently in use by v1 for conformance tests

127c0f7

rchowell approved these changes Jun 11, 2024

View reviewed changes

rchowell merged commit ed882f7 into partiql:v1 Jun 11, 2024
7 checks passed

johnedquinn deleted the v1-eval-value-rebased branch June 11, 2024 22:06

rchowell mentioned this pull request Sep 16, 2024

Redesign ExprValue to support alternative backends #1188

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initializes Datum and adds functionality to evaluator #1451

Initializes Datum and adds functionality to evaluator #1451

johnedquinn commented Apr 27, 2024 •

edited

Loading

rchowell May 6, 2024

johnedquinn May 7, 2024

rchowell Jun 4, 2024

rchowell May 6, 2024

johnedquinn May 7, 2024

johnedquinn May 17, 2024

rchowell May 6, 2024

rchowell May 6, 2024

johnedquinn May 30, 2024

codecov-commenter commented Jun 3, 2024

rchowell Jun 4, 2024

rchowell Jun 4, 2024

johnedquinn Jun 4, 2024

rchowell Jun 5, 2024

rchowell Jun 4, 2024

rchowell Jun 4, 2024

rchowell Jun 4, 2024

rchowell Jun 4, 2024

rchowell Jun 4, 2024

rchowell left a comment

rchowell Jun 7, 2024

johnedquinn commented Jun 11, 2024

Initializes Datum and adds functionality to evaluator #1451

Initializes Datum and adds functionality to evaluator #1451

Conversation

johnedquinn commented Apr 27, 2024 • edited Loading

Relevant Issues

Description

Internal Notes

Other Information

License Information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 3, 2024

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchowell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnedquinn commented Jun 11, 2024

johnedquinn commented Apr 27, 2024 •

edited

Loading