Partiql eval wildcard #1374

yliuuuu · 2024-02-19T20:36:56Z

Relevant Issues

[Closes/Related To] Issue #XXX

Description

To be reviewed after PR#1365 is merged.
Adding support for Path Wildcard.
This PR does so by rewriting the unresolved plan.

Path Expressions with Wildcards PartiQL also provides multi-step path expressions, called path collection expressions.Their semantics is a generalization of the semantics of a path expression with a single [∗] or .∗. Consider the path collection expression:
ew1p1 . . . wnpn
where e is any expression; n > 0; each wildcard step wi is either [∗] or .∗; each series of plain path steps pi
is a sequence of zero or more tuple path navigations or array navigations (potentially mixed).
Then the path collection expression is equivalent to the SFW query
SELECT VALUE vn.pn
FROM
u1 e AS v1,
u2 @v1.p1 AS v2,
. . .,
un @vn−1.pn−1 AS vn,
where each vi is a fresh variable and each ui is UNPIVOT if wi is a .∗ and it is nothing if wi
is a [∗]. Intuitively vi corresponds to the i-th star.

Other Information

Updated Unreleased Section in CHANGELOG: [YES/NO]
- < If NO, why? >
Any backward-incompatible changes? [YES/NO]
- < If YES, why? >
- < For this purpose, we define backward-incompatible changes as changes that—when consumed—can potentially result in
  errors for users that are using our public APIs or the entities that have public visibility in our code-base. >
Any new external dependencies? [YES/NO]
- < If YES, which ones and why? >
- < In addition, please also mention any other alternatives you've considered and the reason they've been discarded >
Do your changes comply with the Contributing Guidelines
and Code Style Guidelines? [YES/NO]

License Information

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov-commenter · 2024-02-19T20:44:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

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

github-actions · 2024-02-19T22:32:04Z

Conformance comparison report-Cross Engine

	Base (eval)	legacy	+/-
% Passing	78.64%	92.51%	13.87%
✅ Passing	4575	5382	807
❌ Failing	1243	436	-807
🔶 Ignored	0	0	0
Total Tests	5818	5818	0
Number passing in both: 4406

Number failing in both: 267

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

Number failing in eval engine but pass in legacy engine: 976
⁉️ CONFORMANCE REPORT REGRESSION DETECTED ⁉️
The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact.
976 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 (`f2369ba`)	`3d2aa2e`	+/-
% Passing	77.84%	78.64%	0.79%
✅ Passing	4529	4575	46
❌ Failing	1289	1243	-46
🔶 Ignored	0	0	0
Total Tests	5818	5818	0
Number passing in both: 4529

Number failing in both: 1243

Number passing in Base (f2369ba) but now fail: 0

Number failing in Base (f2369ba) but now pass: 46
46 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.

Conformance comparison report-Cross Commit-LEGACY

	Base (`f2369ba`)	`3d2aa2e`	+/-
% Passing	92.51%	92.51%	0.00%
✅ Passing	5382	5382	0
❌ Failing	436	436	0
🔶 Ignored	0	0	0
Total Tests	5818	5818	0
Number passing in both: 5382

Number failing in both: 436

Number passing in Base (f2369ba) but now fail: 0

Number failing in Base (f2369ba) but now pass: 0

partiql-planner/src/main/kotlin/org/partiql/planner/internal/transforms/RexConverter.kt

johnedquinn · 2024-02-26T21:05:17Z

partiql-planner/src/main/kotlin/org/partiql/planner/internal/typer/TypeEnv.kt

@@ -29,6 +29,7 @@ internal data class TypeEnv(
    internal fun getScope(depth: Int): TypeEnv {
        return when (depth) {
            0 -> this
+            // TODO: Consider resolve the variable to the correct index


We calculate the correct index in Compiler. There are a couple reasons that we can discuss.

partiql-planner/src/main/kotlin/org/partiql/planner/internal/transforms/RelConverter.kt

johnedquinn · 2024-02-27T23:05:53Z

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

+    @Test
+    fun wildCard2() {
+        val query = """
+            {'a':1, 'b':2}.*


Can you add a query for <someTuple>.*.a?

johnedquinn · 2024-02-27T23:27:55Z

partiql-planner/src/main/kotlin/org/partiql/planner/internal/transforms/RexConverter.kt

+                    is Expr.Path.Step.Unpivot -> {
+                        // Unpivot produces two binding, in this context we want the value,
+                        // which always going to be the second binding
+                        val op = rexOpVarLocal(1, 1)


Does it matter at all what values you put in here? Same goes for the Wildcard below.

If so, does this work when you have more than two path expressions with wildcards/unpivots? Can you add a test to PartiQLEngineDefaultTest for an example like: <someList>[*].books[*].author[*].name[*].middle_initial and maybe something similar for unpivots?

Yes. Specifically:

SELECT VALUE vn.pn |_ this came from pathNavi, which is rexOpVarLocal(1,$last).pn |_ we change the root node to rexOpVarLocal(0, $last_in_from_schema).pn FROM u1 e AS v1, u2 @v1.p1 AS v2, |_ this needs to be rexOpVarLocal(1,$last). . . ., un @vn−1.pn−1 AS vn, |_ this needs to be rexOpVarLocal(1,$last)

Those tests you asking are included in the conformance test:

{ name:"pathDoubleWildCard", statement:"stores[*].books[*].title", assert:{ evalMode:[EvalModeCoerce, EvalModeError], result:EvaluationSuccess, output:$bag::[ "A", "B", "C", "D", "A", "E", "F" ] } }, { name:"pathDoubleUnpivotWildCard", statement:"friends.*.likes.*.type", assert:{ evalMode:[EvalModeCoerce, EvalModeError], result:EvaluationSuccess, output:$bag::[ "dog", "human", "dog", "cat" ] } },

You can check the conformance report details here.

I think the conformance tests should be sufficient. But I can also port the tests to PartiQLEngineDefaultTest

If so, does this work when you have more than two path expressions with wildcards/unpivots?

This doesn't work when you have more than two path expressions. See this commit: e8e31d9.

The example above doesn't compile.

It has to do with the 1, 1 and 1, 0 you've used for rexOpVarLocal for unpivots and wildcards. Those only work for the test cases you've written. Since JOINs really concatenate the LHS schema with the RHS schema, you can't always assume that the item you'll be referencing will always be at some specific index. It needs to be calculated.

Oh I see what you mean. You are right.

johnedquinn

See my other comments.

johnedquinn

LGTM!

yliuuuu force-pushed the partiql-eval-wildcard branch from 397ed08 to 467a954 Compare February 20, 2024 01:37

yliuuuu marked this pull request as ready for review February 20, 2024 03:30

yliuuuu requested review from RCHowell and johnedquinn February 20, 2024 19:22

johnedquinn requested changes Feb 26, 2024

View reviewed changes

yliuuuu force-pushed the partiql-eval-wildcard branch from 30fe790 to a9aca3a Compare February 26, 2024 23:20

yliuuuu requested a review from johnedquinn February 27, 2024 00:03

johnedquinn reviewed Feb 27, 2024

View reviewed changes

yliuuuu requested a review from johnedquinn February 28, 2024 01:51

johnedquinn requested changes Feb 28, 2024

View reviewed changes

yliuuuu added 8 commits February 28, 2024 14:33

wildcard

3524ca0

roll back scan modeling

7fd5966

rebase

8f585c1

rebase clean up

28aec15

address comments

4ec9ff8

rebase

4a6c961

address feedback

c71c11f

add test cases

490c708

yliuuuu force-pushed the partiql-eval-wildcard branch from a9aca3a to 490c708 Compare February 29, 2024 00:40

flatten

5d676bb

johnedquinn approved these changes Mar 7, 2024

View reviewed changes

yliuuuu merged commit 5fb9a1c into partiql-eval Mar 7, 2024
10 checks passed

yliuuuu deleted the partiql-eval-wildcard branch March 7, 2024 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partiql eval wildcard #1374

Partiql eval wildcard #1374

yliuuuu commented Feb 19, 2024

codecov-commenter commented Feb 19, 2024 •

edited

Loading

github-actions bot commented Feb 19, 2024 •

edited

Loading

johnedquinn Feb 26, 2024

johnedquinn Feb 27, 2024

johnedquinn Feb 27, 2024

yliuuuu Feb 28, 2024

yliuuuu Feb 28, 2024

johnedquinn Feb 28, 2024

johnedquinn Feb 28, 2024

yliuuuu Feb 28, 2024

johnedquinn left a comment

johnedquinn left a comment

Partiql eval wildcard #1374

Partiql eval wildcard #1374

Conversation

yliuuuu commented Feb 19, 2024

Relevant Issues

Description

Other Information

License Information

codecov-commenter commented Feb 19, 2024 • edited Loading

Codecov Report

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

Conformance comparison report-Cross Engine

Conformance comparison report-Cross Commit-EVAL

Conformance comparison report-Cross Commit-LEGACY

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

johnedquinn left a comment

Choose a reason for hiding this comment

johnedquinn left a comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 19, 2024 •

edited

Loading

github-actions bot commented Feb 19, 2024 •

edited

Loading