Add `RelSort` and comparator between `PartiQLValue` #1343

alancai98 · 2024-01-20T01:34:26Z

Description

Implements RelSort (ORDER BY) for new evaluator
Adds internal Comparator between PartiQLValues to partiql-eval
Fixes Rex.Op.Select to output a list when input relation has the ordered property
Fixes Rex.Op.Collection to output the appropriate collection rather than always outputting a bag
Modifies ClobValue to implement ScalarValue rather than TextValue

Other Information

Updated Unreleased Section in CHANGELOG: [NO]
Any backward-incompatible changes? [YES]
- Yes, modifies ClobType to implement ScalarValue rather than TextValue
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.

alancai98 · 2024-01-20T01:36:55Z

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

        return visitRex(node.root, ctx)
    }

    // REX

-    override fun visitRex(node: Rex, ctx: Unit): Operator.Expr {
-        return super.visitRexOp(node.op, ctx) as Operator.Expr
+    override fun visitRex(node: Rex, ctx: StaticType?): Operator.Expr {


In the current plan, the rex node's type is only accessible on the Rex and not the inner classes (e.g. Rex.Op.Collection). Passing it along in the context allows the visitor for the inner classes to access the type, which is needed by some nodes like Rex.Op.Collection.

I would really like for our internal IRs to keep the types in the node by defining a type field as part of the PlanNode base class. This would require more codegen work which probably isn't worth the time at the moment. Please let me know if you have an additional ideas here

Hmm I'd need to play around with the generated code a bit to see if there's a better way. Only concern I'd have is that the type field for Rex is a different type (i.e. StaticType) than the type field for Rel (i.e. Rel.Type which has schema and props). Probably that representation in the PlanNode base class would be an enum that could be either of those type definitions?

Anyways, perhaps we should tackle this in another issue/PR.

If you check out PlanTyper, you'll see we use two different visitors since the type is parameterized. There are many ways around this, but really I wish we didn't have the union types. What would be better is for the code generator to support abstract fields.

The best situation however would be handwritten nodes with annotation based generation like Lombok. This would give us the most control.

If you check out PlanTyper, you'll see we use two different visitors since the type is parameterized.

Oh I see. The separate visitors for Rel and Rex doesn't seem too cumbersome.

The best situation however would be handwritten nodes with annotation based generation like Lombok. This would give us the most control.

Agree w/ a mix of code-generated nodes and handwritten nodes would give us the most flexibility when it comes to these interfaces.

alancai98 · 2024-01-20T01:42:22Z

partiql-eval/src/test/kotlin/org/partiql/eval/internal/util/PartiQLValueComparatorTest.kt

@@ -0,0 +1,429 @@
+package org.partiql.eval.internal.util


Tests are taken from NaturalExprValueComparatorsTest.kt but creates and compares the PartiQLValues explictly rather than passing text strings and running the evaluator.

alancai98 · 2024-01-20T02:01:01Z

CI failures seem related to the main merge to partiql-eval commit da03142. They should be fixed by 50f1007.

codecov-commenter · 2024-01-22T19:23:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (partiql-eval@27081a7). Click here to learn what that means.

Additional details and impacted files

@@               Coverage Diff               @@
##             partiql-eval    #1343   +/-   ##
===============================================
  Coverage                ?   50.32%           
  Complexity              ?     1045           
===============================================
  Files                   ?      165           
  Lines                   ?    13129           
  Branches                ?     2452           
===============================================
  Hits                    ?     6607           
  Misses                  ?     5862           
  Partials                ?      660

Flag	Coverage Δ
CLI	`13.77% <0.00%> (?)`
EXAMPLES	`80.28% <0.00%> (?)`
LANG	`54.71% <0.00%> (?)`

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.

test/partiql-tests-runner/src/test/kotlin/org/partiql/runner/executor/EvalExecutor.kt

partiql-planner/src/test/kotlin/org/partiql/planner/util/PlanNodeEquivalentVisitor.kt

partiql-eval/src/main/kotlin/org/partiql/eval/internal/util/PartiQLValueComparator.kt

rchowell · 2024-02-01T17:52:26Z

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

        return visitRex(node.root, ctx)
    }

    // REX

-    override fun visitRex(node: Rex, ctx: Unit): Operator.Expr {
-        return super.visitRexOp(node.op, ctx) as Operator.Expr
+    override fun visitRex(node: Rex, ctx: StaticType?): Operator.Expr {


I would really like for our internal IRs to keep the types in the node by defining a type field as part of the PlanNode base class. This would require more codegen work which probably isn't worth the time at the moment. Please let me know if you have an additional ideas here

partiql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rel/RelSort.kt

partiql-types/src/main/kotlin/org/partiql/value/PartiQLValueComparator.kt

partiql-types/src/main/kotlin/org/partiql/value/PartiQLValue.kt

alancai98 · 2024-02-06T01:21:12Z

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

-) : PlanBaseVisitor<Operator, Symbols>() {
+    private val session: PartiQLEngine.Session,
+    private val symbols: Symbols
+) : PlanBaseVisitor<Operator, StaticType?>() {


(Following rebase of partiql-eval) Change ctx to have type StaticType?. Initialize symbols as part of Compiler constructor since it is not scope sensitive.

alancai98 · 2024-02-06T01:23:05Z

partiql-eval/src/test/kotlin/org/partiql/eval/internal/PartiQLEngineDefaultTest.kt

@@ -203,6 +203,20 @@ class PartiQLEngineDefaultTest {
                    )
                )
            ),
+            SuccessTestCase(


(Following rebase of partiql-eval) This test and the EXCLUDE tests from L250-315 were inadvertently deleted in #1362.

github-actions · 2024-02-06T01:29:59Z

Conformance comparison report-Cross Engine

	Base (legacy)	eval	+/-
% Passing	92.47%	25.95%	-66.52%
✅ Passing	5380	1510	-3870
❌ Failing	438	4308	3870
🔶 Ignored	0	0	0
Total Tests	5818	5818	0
Number passing in both: 1507

Number failing in both: 435

Number passing in legacy engine but fail in eval engine: 3873

Number failing in legacy engine but pass in eval engine: 3
⁉️ CONFORMANCE REPORT REGRESSION DETECTED ⁉️
The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact.
The following test(s) are failing in legacy but pass in eval. Before merging, confirm they are intended to pass:

Click here to see

nullif valid cases{first:"missing",second:"missing",result:missing}, compileOption: PERMISSIVE
nullif valid cases{first:"missing",second:"missing",result:missing}, compileOption: LEGACY
missing and true, compileOption: PERMISSIVE

Conformance comparison report-Cross Commit-LEGACY

	Base (`27081a7`)	`be96085`	+/-
% Passing	92.47%	92.47%	0.00%
✅ Passing	5380	5380	0
❌ Failing	438	438	0
🔶 Ignored	0	0	0
Total Tests	5818	5818	0
Number passing in both: 5380

Number failing in both: 438

Number passing in Base (27081a7) but now fail: 0

Number failing in Base (27081a7) but now pass: 0

Conformance comparison report-Cross Commit-EVAL

	Base (`27081a7`)	`be96085`	+/-
% Passing	25.23%	25.95%	0.72%
✅ Passing	1468	1510	42
❌ Failing	4350	4308	-42
🔶 Ignored	0	0	0
Total Tests	5818	5818	0
Number passing in both: 1468

Number failing in both: 4308

Number passing in Base (27081a7) but now fail: 0

Number failing in Base (27081a7) but now pass: 42
42 test(s) were previously failing but now pass. 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.

rchowell

Nice work

alancai98 self-assigned this Jan 20, 2024

Add RelSort and comparator between PartiQLValue

16931ad

alancai98 force-pushed the partiql-eval-relsort branch from 331e319 to 16931ad Compare January 20, 2024 01:39

alancai98 commented Jan 20, 2024

View reviewed changes

Apply commits from partiql-plugin-impl branch to fix build

50f1007

alancai98 force-pushed the partiql-eval-relsort branch from 7fdfab7 to 50f1007 Compare January 22, 2024 19:16

alancai98 commented Jan 22, 2024

View reviewed changes

test/partiql-tests-runner/src/test/kotlin/org/partiql/runner/executor/EvalExecutor.kt Outdated Show resolved Hide resolved

partiql-planner/src/test/kotlin/org/partiql/planner/util/PlanNodeEquivalentVisitor.kt Outdated Show resolved Hide resolved

Some test corrections; additional comments

48455b7

alancai98 commented Jan 23, 2024

View reviewed changes

partiql-eval/src/main/kotlin/org/partiql/eval/internal/util/PartiQLValueComparator.kt Outdated Show resolved Hide resolved

Move PartiQLValueComparator to partiql-types and make a public fn

cbbdc33

rchowell suggested changes Feb 1, 2024

View reviewed changes

Change RelSort to use Iterator; move comparator to PartiQLValue

792dabe

alancai98 requested a review from rchowell February 5, 2024 20:23

rchowell suggested changes Feb 5, 2024

View reviewed changes

partiql-types/src/main/kotlin/org/partiql/value/PartiQLValue.kt Outdated Show resolved Hide resolved

Merge branch 'partiql-eval' into partiql-eval-relsort

6cbb87b

alancai98 commented Feb 6, 2024

View reviewed changes

alancai98 requested a review from rchowell February 6, 2024 01:27

rchowell approved these changes Feb 6, 2024

View reviewed changes

alancai98 merged commit 97f9926 into partiql-eval Feb 6, 2024
10 checks passed

alancai98 deleted the partiql-eval-relsort branch February 6, 2024 19:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `RelSort` and comparator between `PartiQLValue` #1343

Add `RelSort` and comparator between `PartiQLValue` #1343

alancai98 commented Jan 20, 2024 •

edited

Loading

alancai98 Jan 20, 2024

rchowell Feb 1, 2024

alancai98 Feb 5, 2024

rchowell Feb 6, 2024

alancai98 Feb 6, 2024 •

edited

Loading

alancai98 Jan 20, 2024

alancai98 commented Jan 20, 2024 •

edited

Loading

codecov-commenter commented Jan 22, 2024 •

edited

Loading

rchowell Feb 1, 2024

alancai98 Feb 6, 2024

alancai98 Feb 6, 2024 •

edited

Loading

github-actions bot commented Feb 6, 2024

rchowell left a comment

Add RelSort and comparator between PartiQLValue #1343

Add RelSort and comparator between PartiQLValue #1343

Conversation

alancai98 commented Jan 20, 2024 • edited Loading

Description

Other Information

License Information

alancai98 Jan 20, 2024

Choose a reason for hiding this comment

rchowell Feb 1, 2024

Choose a reason for hiding this comment

alancai98 Feb 5, 2024

Choose a reason for hiding this comment

rchowell Feb 6, 2024

Choose a reason for hiding this comment

alancai98 Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

alancai98 Jan 20, 2024

Choose a reason for hiding this comment

alancai98 commented Jan 20, 2024 • edited Loading

codecov-commenter commented Jan 22, 2024 • edited Loading

Codecov Report

rchowell Feb 1, 2024

Choose a reason for hiding this comment

alancai98 Feb 6, 2024

Choose a reason for hiding this comment

alancai98 Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Feb 6, 2024

Conformance comparison report-Cross Engine

Conformance comparison report-Cross Commit-LEGACY

Conformance comparison report-Cross Commit-EVAL

rchowell left a comment

Choose a reason for hiding this comment

Add `RelSort` and comparator between `PartiQLValue` #1343

Add `RelSort` and comparator between `PartiQLValue` #1343

alancai98 commented Jan 20, 2024 •

edited

Loading

alancai98 Feb 6, 2024 •

edited

Loading

alancai98 commented Jan 20, 2024 •

edited

Loading

codecov-commenter commented Jan 22, 2024 •

edited

Loading

alancai98 Feb 6, 2024 •

edited

Loading