Enable Eval Test Suites #1340

yliuuuu · 2024-01-17T18:16:13Z

Relevant Issues

[Closes/Related To] N/A

Description

Set up the conformance base line for Eval Engine.
The modeling are not finalized, many pieces are now hardcoded for the purpose of running the conformance test. Apis are subject to change.

Other Information

Updated Unreleased Section in CHANGELOG: [YES/NO]
- No.
Any backward-incompatible changes? [YES/NO]
- N/A Not released.
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 · 2024-01-17T19:15:30Z

Conformance comparison report-Cross Engine

	Base (legacy)	eval	+/-
% Passing	92.54%	21.59%	-70.95%
✅ Passing	5384	1256	-4128
❌ Failing	434	4562	4128
🔶 Ignored	0	0	0
Total Tests	5818	5818	0
Number passing in both: 1249

Number failing in both: 427

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

Number failing in legacy engine but pass in eval engine: 7
⁉️ 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

null comparison{sql:"MISSING = NULL",result:missing::null}, compileOption: LEGACY
null comparison{sql:"NULL = MISSING",result:missing::null}, compileOption: LEGACY
null comparison{sql:"null.null = MISSING",result:missing::null}, compileOption: LEGACY
nullif valid cases{first:"missing",second:"missing",result:missing}, compileOption: PERMISSIVE
nullif valid cases{first:"missing",second:"missing",result:missing}, compileOption: LEGACY
equality of scalar missing, compileOption: LEGACY
missing and true, compileOption: PERMISSIVE

Conformance comparison report-Cross Commit-LEGACY

	Base (`57496a9`)	`f48fd6c`	+/-
% Passing	92.54%	92.54%	0.00%
✅ Passing	5384	5384	0
❌ Failing	434	434	0
🔶 Ignored	0	0	0
Total Tests	5818	5818	0
Number passing in both: 5384

Number failing in both: 434

Number passing in Base (57496a9) but now fail: 0

Number failing in Base (57496a9) but now pass: 0

Conformance comparison report-Cross Commit-EVAL

	Base (`57496a9`)	`f48fd6c`	+/-
% Passing	3.01%	21.59%	18.58%
✅ Passing	175	1256	1081
❌ Failing	5644	4562	-1082
🔶 Ignored	0	0	0
Total Tests	5819	5818	-1
Number passing in both: 169

Number failing in both: 4556

Number passing in Base (57496a9) but now fail: 6

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

Click here to see

equiv attribute value pair unpivot non-missing, compileOption: LEGACY
offset -1, compileOption: PERMISSIVE
offset -1, compileOption: LEGACY
offset 1-2, compileOption: LEGACY
offset , compileOption: LEGACY
offset >, compileOption: LEGACY

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

codecov-commenter · 2024-01-17T19:49:36Z

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (partiql-plugin-impl@57496a9). Click here to learn what that means.

Additional details and impacted files

@@                  Coverage Diff                   @@
##             partiql-plugin-impl    #1340   +/-   ##
======================================================
  Coverage                       ?   49.27%           
  Complexity                     ?     1046           
======================================================
  Files                          ?      166           
  Lines                          ?    13395           
  Branches                       ?     2504           
======================================================
  Hits                           ?     6600           
  Misses                         ?     6138           
  Partials                       ?      657

Flag	Coverage Δ
CLI	`11.86% <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/ConformanceTestBase.kt

yliuuuu · 2024-01-17T19:52:25Z

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

+            if (rootType is AnyType) {
+                val match = ResolvedVar.Local(StaticType.ANY, ordinal, rootType, listOf(pathPrefix) + path.steps)
+                matches.add(match)


To temporarily unblock the path access on any type. This behavior is probably incorrect.

Interesting. I'm not sure we can resolve a variable if the type is ANY/UNKNOWN. I feel this should just be a path expression on the root

We need a refactor on PartiQLValueType before doing this.

Consider:

tbl : <<{'a' : 1, 'b': 2}>>

We now infer the tbl having PartiQLValueType of Bag, which is then converted to StaticType BAG(ANY).

So if we do SELECT tbl.a FROM tbl. Without this, the inferencer would fail.

I think this would break the defeasible reasoning paper (PartiQL Variable Resolution) written by @RCHowell .

If R is NOT known and S is KNOWN, then we must choose S. In this scenario, we wouldn't choose either due to ambiguity. I believe the recently merged PR #1344 solves this.

Pick the #1344. Removed the temporary work around.

yliuuuu · 2024-01-17T19:54:47Z

partiql-eval/src/main/kotlin/org/partiql/eval/PartiQLEngine.kt

@@ -19,7 +22,7 @@ import org.partiql.plan.PartiQLPlan
 */
 public interface PartiQLEngine {

-    public fun prepare(plan: PartiQLPlan): PartiQLStatement<*>
+    public fun prepare(plan: PartiQLPlan, session: Session): PartiQLStatement<*>


making this more akin to the Planner APIs.

RCHowell · 2024-01-18T19:11:15Z

partiql-eval/src/main/kotlin/org/partiql/eval/PartiQLEngine.kt

+    public class Session @OptIn(PartiQLFunctionExperimental::class) constructor(
+        var bindings: MutableMap<String, ConnectorBindings> = mutableMapOf(),
+        val functions: MutableMap<String, List<PartiQLFunction>> = mutableMapOf()
+    )


At some point we'll need session variables passed during statement execution.

Added a to-do note. Maybe we can do this after the work on Sql plugin is completed.

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

RCHowell · 2024-01-18T19:22:02Z

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

+            if (rootType is AnyType) {
+                val match = ResolvedVar.Local(StaticType.ANY, ordinal, rootType, listOf(pathPrefix) + path.steps)
+                matches.add(match)


Interesting. I'm not sure we can resolve a variable if the type is ANY/UNKNOWN. I feel this should just be a path expression on the root

test/partiql-tests-runner/src/test/kotlin/org/partiql/runner/ConformanceTestBase.kt

partiql-eval/src/main/kotlin/org/partiql/eval/PartiQLEngine.kt

partiql-eval/src/main/kotlin/org/partiql/eval/PartiQLEngineBuilder.kt

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

test/partiql-tests-runner/src/test/kotlin/org/partiql/runner/test/TestRunner.kt

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

yliuuuu added 4 commits January 17, 2024 10:11

initial

f138ad1

clean up

b3d5f84

lint

cc3c03e

clean up

1b66130

yliuuuu added 2 commits January 17, 2024 11:42

clean up

0fc0792

increase timeout constraint

20c4f78

yliuuuu commented Jan 17, 2024

View reviewed changes

yliuuuu marked this pull request as ready for review January 17, 2024 19:59

yliuuuu requested a review from RCHowell January 17, 2024 19:59

RCHowell requested changes Jan 18, 2024

View reviewed changes

address comments

dfe25e3

yliuuuu requested a review from RCHowell January 24, 2024 01:00

johnedquinn requested changes Jan 24, 2024

View reviewed changes

yliuuuu added 2 commits January 24, 2024 15:49

resolve

7ef78e3

address comments

c50fde3

johnedquinn reviewed Jan 25, 2024

View reviewed changes

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

value comparison with annotation

a774199

johnedquinn approved these changes Jan 25, 2024

View reviewed changes

yliuuuu merged commit 23961f3 into partiql-plugin-impl Jan 25, 2024
10 checks passed

yliuuuu deleted the partiql-plugin-eval branch January 25, 2024 00:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Eval Test Suites #1340

Enable Eval Test Suites #1340

yliuuuu commented Jan 17, 2024

github-actions bot commented Jan 17, 2024 •

edited

Loading

codecov-commenter commented Jan 17, 2024 •

edited

Loading

yliuuuu Jan 17, 2024

RCHowell Jan 18, 2024

yliuuuu Jan 18, 2024

johnedquinn Jan 24, 2024

yliuuuu Jan 24, 2024

yliuuuu Jan 17, 2024

RCHowell Jan 18, 2024

yliuuuu Jan 23, 2024

RCHowell Jan 18, 2024

Enable Eval Test Suites #1340

Enable Eval Test Suites #1340

Conversation

yliuuuu commented Jan 17, 2024

Relevant Issues

Description

Other Information

License Information

github-actions bot commented Jan 17, 2024 • edited Loading

Conformance comparison report-Cross Engine

Conformance comparison report-Cross Commit-LEGACY

Conformance comparison report-Cross Commit-EVAL

codecov-commenter commented Jan 17, 2024 • edited Loading

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

github-actions bot commented Jan 17, 2024 •

edited

Loading

codecov-commenter commented Jan 17, 2024 •

edited

Loading