Adds performance optimizations for materialization #1379

johnedquinn · 2024-02-26T22:58:27Z

Relevant Issues

None

Description

Adds performance optimizations for materialization on scan. See RecordValueIterator and RelScan.
Adds performance optimizations for checking if a query returns data (using hasNext). We no longer need to materialize data pre-maturely.
Adds the ability to define globals as Ion in the memory catalog.
Updates Operator.Relation to be an iterable
Makes materialization of SELECTs lazy.
- In order to handle lazy materialization, I needed to update how we handle out-of-scope values. In this PR, we model them in the evaluator similar to how closures operate. We pass and maintain an Environment to maintain a linked list of scopes/records.

Other 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.

johnedquinn · 2024-02-26T23:06:21Z

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

@@ -15,17 +16,17 @@ internal class RelScanPermissive(
    override fun open() {
        val r = expr.eval(Record.empty)
        records = when (r) {
-            is CollectionValue<*> -> r.map { Record.of(it) }.iterator()
+            is CollectionValue<*> -> RecordValueIterator(r)


This line had previously been proven to be expensive for CPU utilization.

github-actions · 2024-02-26T23:11:09Z

Conformance comparison report-Cross Engine

	Base (eval)	legacy	+/-
% Passing	77.09%	92.45%	15.37%
✅ Passing	4485	5379	894
❌ Failing	1333	439	-894
🔶 Ignored	0	0	0
Total Tests	5818	5818	0
Number passing in both: 4317

Number failing in both: 271

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

Number failing in eval engine but pass in legacy engine: 1062
⁉️ CONFORMANCE REPORT REGRESSION DETECTED ⁉️
The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact.
1062 test(s) were failing in eval but now pass in legacy. 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.

Conformance comparison report-Cross Commit-EVAL

	Base (`a8cda41`)	`4a278df`	+/-
% Passing	77.52%	77.09%	-0.43%
✅ Passing	4510	4485	-25
❌ Failing	1308	1333	25
🔶 Ignored	0	0	0
Total Tests	5818	5818	0
Number passing in both: 4483

Number failing in both: 1306

Number passing in Base (a8cda41) but now fail: 27

Number failing in Base (a8cda41) but now pass: 2
⁉️ 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) were previously failing but now pass. Before merging, confirm they are intended to pass:

Click here to see

unpivot with pivot to analyze attribute names, compileOption: PERMISSIVE
unpivot with pivot to analyze attribute names, compileOption: LEGACY

Conformance comparison report-Cross Commit-LEGACY

	Base (`a8cda41`)	`4a278df`	+/-
% Passing	92.47%	92.45%	-0.02%
✅ Passing	5380	5379	-1
❌ Failing	438	439	1
🔶 Ignored	0	0	0
Total Tests	5818	5818	0
Number passing in both: 5379

Number failing in both: 438

Number passing in Base (a8cda41) but now fail: 1

Number failing in Base (a8cda41) but now pass: 0
⁉️ CONFORMANCE REPORT REGRESSION DETECTED ⁉️. The following test(s) were previously passing but now fail:

Click here to see

MYSQL_SELECT_21, compileOption: PERMISSIVE

Adds support for globals Updates Operator.Relation to be an iterable

codecov-commenter · 2024-02-27T22:29:09Z

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

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

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

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-02-28T21:25:04Z

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

+    private val current: Record,
+    private val parent: Environment? = null


Suggested change

private val current: Record,

private val parent: Environment? = null

private val record: Record,

private val parent: Environment? = null

Record could be called Bindings which would be more accurate to our spec.

Sounds good. I'm going to update the local variable name here, but I'm going to leave the name Record until another PR. The diff would touch everything.

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

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

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

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

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

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

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

rchowell · 2024-02-29T17:52:58Z

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

-        while (inputRecord != null) {
+    override fun open(env: Environment) {
+        input.open(env)
+        for (inputRecord in input) {


I think this is a good example of why materialization in the first call to hasNext/next makes sense. Here we would be materializing many of the relations under this aggregation — which is coming from open rather than iterating the iterator

rchowell · 2024-02-29T18:14:52Z

partiql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rex/ExprSelect.kt

+    @OptIn(PartiQLValueExperimental::class)
+    class Elements(
+        private val input: Operator.Relation,
+        private val constructor: Operator.Expr,
+        private val env: Environment,
+    ) : Iterable<PartiQLValue> {
+
+        override fun iterator(): Iterator<PartiQLValue> {
+            return object : Iterator<PartiQLValue> {
+                private var _init = false
+
+                override fun hasNext(): Boolean {
+                    if (!_init) {
+                        input.open(env)
+                        _init = true
+                    }
+                    val hasNext = input.hasNext()
+                    if (!hasNext) {
+                        input.close()
+                    }
+                    return hasNext
+                }
+
+                override fun next(): PartiQLValue {
+                    val r = input.next()
+                    val result = constructor.eval(env.push(r))
+                    return result
+                }
+            }
+        }
+    }


Looks good.

johnedquinn commented Feb 26, 2024

View reviewed changes

johnedquinn added 3 commits February 26, 2024 19:14

Adds optimizations for materialization

2457153

Adds support for globals Updates Operator.Relation to be an iterable

Fixes the lazy materialization

6f6f9c5

Migrates variable access to use the environment

a736113

johnedquinn force-pushed the partiql-eval-perf branch from caa80a7 to a736113 Compare February 27, 2024 22:20

johnedquinn added 3 commits February 27, 2024 14:37

Makes minor modifications for clean-up

9b309ff

Fixes unpivot

49e718c

Fixes bug in RelMaterialized

e92d3ba

johnedquinn changed the title ~~[DRAFT] Adds performance optimizations for materialization~~ Adds performance optimizations for materialization Feb 28, 2024

johnedquinn marked this pull request as ready for review February 28, 2024 16:53

johnedquinn requested a review from rchowell February 28, 2024 16:53

rchowell suggested changes Feb 28, 2024

View reviewed changes

Addresses PR comments

5a92d2b

johnedquinn requested a review from rchowell February 29, 2024 01:13

rchowell approved these changes Feb 29, 2024

View reviewed changes

johnedquinn merged commit e5a1e24 into partiql-eval Mar 4, 2024
10 checks passed

johnedquinn deleted the partiql-eval-perf branch March 4, 2024 16:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds performance optimizations for materialization #1379

Adds performance optimizations for materialization #1379

johnedquinn commented Feb 26, 2024 •

edited

Loading

johnedquinn Feb 26, 2024

github-actions bot commented Feb 26, 2024 •

edited

Loading

codecov-commenter commented Feb 27, 2024 •

edited

Loading

rchowell Feb 28, 2024

johnedquinn Feb 28, 2024

rchowell Feb 29, 2024

rchowell Feb 29, 2024

		private val current: Record,
		private val parent: Environment? = null

Adds performance optimizations for materialization #1379

Adds performance optimizations for materialization #1379

Conversation

johnedquinn commented Feb 26, 2024 • edited Loading

Relevant Issues

Description

Other Information

License Information

johnedquinn Feb 26, 2024

Choose a reason for hiding this comment

github-actions bot commented Feb 26, 2024 • edited Loading

Conformance comparison report-Cross Engine

Conformance comparison report-Cross Commit-EVAL

Conformance comparison report-Cross Commit-LEGACY

codecov-commenter commented Feb 27, 2024 • edited Loading

Codecov Report

rchowell Feb 28, 2024

Choose a reason for hiding this comment

johnedquinn Feb 28, 2024

Choose a reason for hiding this comment

rchowell Feb 29, 2024

Choose a reason for hiding this comment

rchowell Feb 29, 2024

Choose a reason for hiding this comment

johnedquinn commented Feb 26, 2024 •

edited

Loading

github-actions bot commented Feb 26, 2024 •

edited

Loading

codecov-commenter commented Feb 27, 2024 •

edited

Loading