Adds performance optimizations for ExprCallDynamic #1388

johnedquinn · 2024-03-13T19:34:14Z

Relevant Issues

None

Description

Adds performance optimizations for ExprCallDynamic
- Creates a CandidateIndex.All that preserves the initial order of the candidate functions, however, it internally wraps a list of CandidateIndex.Direct and CandidateIndex.Indirect. CandidateIndex.Direct holds a map between runtime parameter types and the associated candidate to invoke. CandidateIndex.Indirect holds an ordered list of any non-runtime parameter types (aka ANY). With this, CandidateIndex.All can reduce the number of checks by taking advantage of these two subclasses.
- Fixes ordering of the dynamic candidates. Before, we would lose the original ordering of candidates, and for some functions, it would cause extremely expensive computation. For example, when profiling SELECT ... WHERE <ANY> = 2, the lack of ordering would for some reason always place EQ(ANY, ANY) first -- which would invoke PartiQL's comparator -- which is seemingly very slow (taking up 25% of the entire test's CPU usage). By re-ordering the candidates, it significantly impacted our performance for the better (the matched EQ function did not even show up on the profiler, indicating that it took up less than 1% of the CPU usage).
Since CASTs are really function calls in SQL:1999, I've updated the definition of Ref.Cast to contain isNullable -- at least until we truly represent casts with function calls.

<user-defined cast definition> ::=
    CREATE CAST <left paren> <source data type> AS <target data type> <right paren>
    WITH <cast function>
    [ AS ASSIGNMENT ]

I needed to update the tests since the typing of functions like ADD(NULL, INT) were initially returning the type NULL, but it really should have been returning INT?. This is modeled slightly different in SQL:1999 since types are nullable, but until we add this, we'll need to support this.
By examining the profiler, in permissive mode, our runtime exceptions that get converted to MISSINGs are extremely expensive (causing up to 33.2% of our CPU usage for evaluation) due to the fillInStackTrace method. Since we are programming a language, we can/should be architecting our own error messaging system.

Note

The conformance tests that now fail are from NULL = MISSING. See the Summary. I think these tests are wrong. See PartiQL Spec: "Equality never fails in the type-checking mode and never returns MISSING in the permissive mode."

Other Information

Updated Unreleased Section in CHANGELOG: NO
Any backward-incompatible changes? YES -- the isNullable flag, which is required
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.

github-actions · 2024-03-13T19:41:04Z

Conformance comparison report-Cross Engine

	Base (eval)	legacy	+/-
% Passing	82.43%	92.51%	10.07%
✅ Passing	4796	5382	586
❌ Failing	1022	436	-586
🔶 Ignored	0	0	0
Total Tests	5818	5818	0
Number passing in both: 4624

Number failing in both: 264

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

Number failing in eval engine but pass in legacy engine: 758
⁉️ CONFORMANCE REPORT REGRESSION DETECTED ⁉️
The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact.
758 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 (`609f8b8`)	`7985180`	+/-
% Passing	82.81%	82.43%	-0.38%
✅ Passing	4818	4796	-22
❌ Failing	1000	1022	22
🔶 Ignored	0	0	0
Total Tests	5818	5818	0
Number passing in both: 4792

Number failing in both: 996

Number passing in Base (609f8b8) but now fail: 26

Number failing in Base (609f8b8) but now pass: 4
⁉️ 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

MYSQL_SELECT_BUG_01, compileOption: PERMISSIVE
MYSQL_SELECT_BUG_01, compileOption: LEGACY
cast to NULL valid cases{value:"NULL"}, compileOption: PERMISSIVE
cast to NULL valid cases{value:"NULL"}, compileOption: LEGACY

Conformance comparison report-Cross Commit-LEGACY

	Base (`609f8b8`)	`7985180`	+/-
% Passing	92.52%	92.51%	-0.02%
✅ Passing	5383	5382	-1
❌ Failing	435	436	1
🔶 Ignored	0	0	0
Total Tests	5818	5818	0
Number passing in both: 5382

Number failing in both: 435

Number passing in Base (609f8b8) but now fail: 1

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

Click here to see

MYSQL_SELECT_29, compileOption: LEGACY

codecov-commenter · 2024-03-13T23:27:04Z

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

@@               Coverage Diff               @@
##             partiql-eval    #1388   +/-   ##
===============================================
  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.

Adds casts for nulls, fixes toSet() for candidates, and increases performance Fixes dynamic candidate ordering Fixes NULL/MISSING equality

yliuuuu · 2024-03-14T23:49:03Z

If we know that the function will be always null, in my opinion we should type the function to NULL.

For two reasons:

Consider a unary function, i.e., -null, the typer now returns UnionOf(INT2, NULL), which can be fine, but a little confused.
The information that the function is always going to return NULL can help with optimization, i.e., 1 + NULL IS NULL, we can constant fold that to true.

johnedquinn · 2024-03-15T16:18:46Z

If we know that the function will be always null, in my opinion we should type the function to NULL.

For two reasons:

1. Consider a unary function, i.e., `-null`, the typer now returns `UnionOf(INT2, NULL)`, which can be fine, but a little confused.

2. The information that the function is always going to return `NULL` can help with optimization, i.e., `1 + NULL IS NULL`, we can constant fold that to `true`.

True. Good call out. Seemingly our typing of casts is wrong. From the SQL:1999 Spec Section 6.22:

If the specifies NULL, then TV is the null value.
If SV is the null value, then the result is the null value.

Since we model NULL as a distinct type, we should still insert the casts, but the typer really should fold the CAST (since we know the input is null). If you're aligned, I can fix the typing of rexOpCast.

rchowell · 2024-03-15T16:14:26Z

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

    private val args: Array<Operator.Expr>
 ) : Operator.Expr {

+    private val candidateIndex = CandidateIndex.All(candidates)


I say create the index in the Compiler and pass to the ExprCallDynamic. In this scenario, we're passing an arg into the constructor just to be passed to another constructor.

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

rchowell · 2024-03-15T16:35:51Z

partiql-planner/src/test/kotlin/org/partiql/planner/internal/typer/PlanTyperTestsPorted.kt

@@ -729,7 +729,7 @@ class PlanTyperTestsPorted {
            SuccessTestCase(
                name = "BITWISE_AND_NULL_OPERAND",
                query = "1 & NULL",
-                expected = StaticType.NULL,
+                expected = StaticType.unionOf(StaticType.INT4, StaticType.NULL),


Why not StaticType.NULL?

The value will be NULL, however, what is the type of the null value? I'm using our normal rules for type/function precedence to resolve the output types. See the following PostgreSQL DB Fiddle Example.

partiql-plan/src/main/resources/partiql_plan.ion

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

partiql-spi/src/main/kotlin/org/partiql/spi/connector/sql/builtins/FnNot.kt

partiql-planner/src/main/kotlin/org/partiql/planner/internal/FnMatch.kt

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

alancai98 · 2024-03-15T23:11:19Z

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

-                    null -> {
-                        if (!(inputType == parameterType || inputType == PartiQLValueType.NULL || parameterType == PartiQLValueType.ANY)) {
-                            return false
+            init {


Curious if we see any notable CPU utilization jump from this init block since there is more work when an ExprCallDynamic is initialized. Wondering if there are any situations in which this construction of the CandidateIndex results in worse evaluation performance/utilization (e.g. if candidates is just a list of two elements)? I assume this change is better in the general case in which there are more than just a few candidate elements though.

Good call out -- in the future, I'd say it'd be a good idea for the Compiler to dictate what version of ExprCallDynamic to create (the indexing O(1) vs iteration O(n)) depending on the number of candidates (and our own testing). As for init by itself, it is only invoked at compilation.

For now, we know dynamic is quite expensive for our customers.

johnedquinn marked this pull request as ready for review March 13, 2024 23:28

Adds performance optimizations for ExprCallDynamic

7d3aadd

Adds casts for nulls, fixes toSet() for candidates, and increases performance Fixes dynamic candidate ordering Fixes NULL/MISSING equality

johnedquinn force-pushed the partiql-eval-perf-dynamic branch from f397a84 to 7d3aadd Compare March 14, 2024 03:52

Fixes NULL/MISSING EQ tests

324926d

johnedquinn requested a review from rchowell March 14, 2024 18:22

johnedquinn changed the base branch from partiql-eval to v1 March 14, 2024 21:22

rchowell reviewed Mar 15, 2024

View reviewed changes

alancai98 reviewed Mar 15, 2024

View reviewed changes

johnedquinn added 2 commits April 1, 2024 16:57

Addresses PR feedback

43ebfbe

Runs ktlint

25d56dd

rchowell approved these changes Apr 2, 2024

View reviewed changes

johnedquinn merged commit 810035c into v1 Apr 2, 2024
10 checks passed

johnedquinn deleted the partiql-eval-perf-dynamic branch April 2, 2024 20:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds performance optimizations for ExprCallDynamic #1388

Adds performance optimizations for ExprCallDynamic #1388

johnedquinn commented Mar 13, 2024 •

edited

Loading

github-actions bot commented Mar 13, 2024 •

edited

Loading

codecov-commenter commented Mar 13, 2024 •

edited

Loading

yliuuuu commented Mar 14, 2024 •

edited

Loading

johnedquinn commented Mar 15, 2024

rchowell Mar 15, 2024

rchowell Mar 15, 2024

johnedquinn Apr 1, 2024

alancai98 Mar 15, 2024

johnedquinn Apr 2, 2024

Adds performance optimizations for ExprCallDynamic #1388

Adds performance optimizations for ExprCallDynamic #1388

Conversation

johnedquinn commented Mar 13, 2024 • edited Loading

Relevant Issues

Description

Note

Other Information

License Information

github-actions bot commented Mar 13, 2024 • edited Loading

Conformance comparison report-Cross Engine

Conformance comparison report-Cross Commit-EVAL

Conformance comparison report-Cross Commit-LEGACY

codecov-commenter commented Mar 13, 2024 • edited Loading

Codecov Report

yliuuuu commented Mar 14, 2024 • edited Loading

johnedquinn commented Mar 15, 2024

rchowell Mar 15, 2024

Choose a reason for hiding this comment

rchowell Mar 15, 2024

Choose a reason for hiding this comment

johnedquinn Apr 1, 2024

Choose a reason for hiding this comment

alancai98 Mar 15, 2024

Choose a reason for hiding this comment

johnedquinn Apr 2, 2024

Choose a reason for hiding this comment

johnedquinn commented Mar 13, 2024 •

edited

Loading

github-actions bot commented Mar 13, 2024 •

edited

Loading

codecov-commenter commented Mar 13, 2024 •

edited

Loading

yliuuuu commented Mar 14, 2024 •

edited

Loading