Add `EXCLUDE` to `partiql-eval` #1320

alancai98 · 2023-12-20T01:39:45Z

Relevant Issues

EXCLUDE RFC -- RFC-0051: EXCLUDE Clause partiql-docs#51
EXCLUDE implementation in EvaluatingCompiler -- Implement EXCLUDE in EvaluatingCompiler #1249
Closed PR to add EXCLUDE to PhysicalPlanCompiler -- Add exclude physical plan compiler impl #1280
- Much of this PR's additions are based off that PR

Description

Adds EXCLUDE to the partiql-eval evaluator
Different modeling of EXCLUDE in the plan representation. This change in modeling was done to better align with the redundant path elimination and evaluation approaches
Adds some path subsumption tests that were previously commented out in 4ac1b3d
- More tests were added beyond the previous set of tests

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.

alancai98 · 2023-12-20T01:40:41Z

partiql-eval/src/test/kotlin/org/partiql/eval/internal/exclude/CompiledExcludeItemTest.kt

@@ -0,0 +1,251 @@
+package org.partiql.eval.internal.exclude


These tests are the same as the commented out tests from partiql-eval/src/test/kotlin/org/partiql/lang/eval/EvaluatingCompilerExcludeTests.kt

alancai98 · 2023-12-20T01:41:25Z

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

@@ -264,6 +264,35 @@ class PartiQLEngineDefaultTest {
        assertEquals(expected, output)
    }

+    @OptIn(PartiQLValueExperimental::class)
+    @Test
+    fun testExclude() {


Test succeeds when @Disabled is removed from L29.

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

Line 29 in 672a09d

@Disabled

github-actions · 2023-12-20T01:44:59Z

Conformance comparison report

	Base (`d64784c`)	`a496bac`	+/-
% 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 (d64784c) but now fail: 0

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

codecov-commenter · 2023-12-20T01:54:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

@@               Coverage Diff               @@
##             partiql-eval    #1320   +/-   ##
===============================================
  Coverage                ?   49.26%           
  Complexity              ?     1046           
===============================================
  Files                   ?      166           
  Lines                   ?    13396           
  Branches                ?     2504           
===============================================
  Hits                    ?     6600           
  Misses                  ?     6139           
  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.

RCHowell · 2023-12-21T00:09:36Z

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

+    }
+}
+
+@OptIn(PartiQLValueExperimental::class)


Rather than having a bunch of top-level functions, could you please place these in the appropriate spots? Compile related functions in the Compiler, and evaluation related items wihtin the RelExclude operator?

Changed locations of the top-level functions in 9e9a040

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

RCHowell · 2023-12-21T01:05:21Z

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

+            val collWithRemoved = when (coll) {
+                is BagValue -> coll
+                is ListValue, is SexpValue -> coll.filterIndexed { index, _ ->
+                    !indexesToRemove.contains(index)
+                }
+            }
+            val finalColl = collWithRemoved.mapIndexed { index, element ->


what's the difference between collWithRemoved and finalColl? I think you should be applying the exclusions with a mapIndexedNotNull and return null if the index is skipped otherwise apply the exlcusion. This way you don't traverse the collection more than once.

collWithRemoved would remove the indexes at that level (i.e. the leaf nodes with ExcludeStep.CollIndex). finalColl would remove the values at deeper nested levels (i.e. the branch nodes).

I think you should be applying the exclusions with a mapIndexedNotNull and return null if the index is skipped otherwise apply the exlcusion. This way you don't traverse the collection more than once.

Good point. I will change to use mapIndexedNotNull so only one iteration of coll is necessary.

RCHowell · 2023-12-21T01:06:23Z

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

+                // apply collection wildcard exclusions for lists, bags, and sexps
+                val collectionWildcardKey = ExcludeStep.CollWildcard
+                branches.find {
+                    it.step == collectionWildcardKey
+                }?.let {
+                    expr = excludeOnPartiQLValue(expr, it)
+                }


Do you have an example of what this is for?

I'll edit the comment to clarify. This basically applies all the collection wildcard exclude steps that have deeper exclude steps (e.g. t.a.b.c[*].field_x in

SELECT * EXCLUDE t.a.b.c[*].field_x FROM [{ 'a': { 'b': { 'c': [ { -- c[0]; field_x to be removed 'field_x': 0, 'field_y': 0 }, { -- c[1]; field_x to be removed 'field_x': 1, 'field_y': 1 }, { -- c[2]; field_x to be removed 'field_x': 2, 'field_y': 2 } ] } } }] AS t

When we get to the c list, the collection wildcard step will apply the remainder of the step (i.e. .field_x) on each of the elements of c.

Ported above example to another test:

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

Lines 249 to 296 in 50de032

SuccessTestCase(

input = """

SELECT *

EXCLUDE

t.a.b.c[*].field_x

FROM [{

'a': {

'b': {

'c': [

{ -- c[0]; field_x to be removed

'field_x': 0,

'field_y': 0

},

{ -- c[1]; field_x to be removed

'field_x': 1,

'field_y': 1

},

{ -- c[2]; field_x to be removed

'field_x': 2,

'field_y': 2

}

]

}

}

}] AS t

""".trimIndent(),

expected = bagValue(

structValue(

"a" to structValue(

"b" to structValue(

"c" to bagValue( // TODO: should be ListValue; currently, Rex.ExprCollection doesn't return lists

structValue(

"field_y" to int32Value(0)

),

structValue(

"field_y" to int32Value(1)

),

structValue(

"field_y" to int32Value(2)

)

)

)

)

)

)

)

)

}

Though the inner collection should be a list rather than bag. I believe this is a bug (or not yet implemented) in Rex.ExprCollection which currently always returning a bag.

partiql-eval/src/main/kotlin/org/partiql/eval/internal/exclude/CompiledExcludeItem.kt

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

… phase

RCHowell · 2024-01-11T19:50:49Z

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

+        while (true) {
+            val row = input.next() ?: return null
+            val newRecord = exclusions.fold(row) { curRecord, expr ->
+                excludeOnRecord(curRecord, expr)
+            }
+            return newRecord
+        }


You don't need the loop because you aren't processing multiple input rows.

Suggested change

while (true) {

val row = input.next() ?: return null

val newRecord = exclusions.fold(row) { curRecord, expr ->

excludeOnRecord(curRecord, expr)

}

return newRecord

}

val record = input.next() ?: return null

val result = exclusions.fold(record) { curr, path -> curr.exlcude(path) }

return result

Something I've noticed which is a total nitpicky thing is that in Kotlin (unlike rust) functions don't need unique names so you can overload the name because the parameters are different.

Instead of

excludeOnRecord(record: Record. ...) excludeOnStructValue(structValue: StructValue<*>, ...) excludeOnCollValue(collValue: CollectionValue<*>, ...) excludeOnPartiQLValue(initialPartiQLValue: PartiQLValue, ...)

You can do

exclude(record: Record, ...) exclude(value: PartiQLValue, ...) // usually declare the base before the others, but it doesn't matter exclude(struct: StructValue<*>, ...) exclude(collection: CollectionValue<*>, ...)

This is only a style thing to cut out verbosity, but I can see how the naming is carried over from Rust.

You don't need the loop because you aren't processing multiple input rows.

Ah, true. I will change it do process just one row.

For naming of the exclude, I'll change all of the excludeOn... to exclude. This will be more consistent with how the TypeUtils models the similar functions for exclude on static type.

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

RCHowell · 2024-01-11T20:08:13Z

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

+            val subsumedPaths = newPaths
+                .groupBy(keySelector = { it.root }, valueTransform = { it.steps }) // combine exclude paths with the same resolved root before subsumption
+                .map { (root, allSteps) ->
+                    val nonRedundant = ExcludeRepr.toExcludeRepr(allSteps.flatten()).removeRedundantSteps()
+                    relOpExcludePath(root, nonRedundant.toPlanRepr())
+                }
+            val op = relOpExclude(input, subsumedPaths)


Is this in case the AST representations aren't equivalent, but perhaps they resolve to the same?

SELECT * EXCLUDE t.a.*, "t".a FROM { 'a': { 'x': 1 } ... } AS t

I think the only time the root (which is a var) would differ is based on the case-sensitivity of the identifier.

Correct, the ast representations differed since the root (identifier) of t.a.* was case-insensitive while "t".a was case-sensitive. They would resolve to the same in this example and should be under the same exclude path root in the resolved plan.

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

…; add comments

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

RCHowell

Nice work!

alancai98 self-assigned this Dec 20, 2023

alancai98 requested a review from RCHowell December 20, 2023 01:39

alancai98 commented Dec 20, 2023

View reviewed changes

alancai98 force-pushed the add-exclude-partiql-eval branch from 672a09d to 910dec0 Compare December 20, 2023 01:46

Add EXCLUDE to partiql-eval

5bc681e

alancai98 force-pushed the add-exclude-partiql-eval branch from 910dec0 to 5bc681e Compare December 20, 2023 19:34

RCHowell requested changes Dec 21, 2023

View reviewed changes

Move around top-level fns; make EXCLUDE input private

9e9a040

alancai98 requested a review from RCHowell December 21, 2023 00:59

RCHowell requested changes Dec 21, 2023

View reviewed changes

Reduce repeated looping; additional test for EXCLUDE

50de032

alancai98 requested a review from RCHowell December 21, 2023 02:46

RCHowell requested changes Dec 21, 2023

View reviewed changes

[WIP] refactor EXCLUDE repr in plan; move subsumption to earlier plan…

15b846a

… phase

alancai98 marked this pull request as draft January 11, 2024 02:30

Fix case-sensitive plan typing test

ea5baf5

RCHowell requested changes Jan 11, 2024

View reviewed changes

Address RelExclude comments; slight refactor of subsumption + testing…

3be4a5e

…; add comments

alancai98 marked this pull request as ready for review January 12, 2024 00:24

alancai98 requested a review from RCHowell January 12, 2024 00:30

RCHowell requested changes Jan 12, 2024

View reviewed changes

partiql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rel/RelExclude.kt Outdated Show resolved Hide resolved

Remove unnecessary fold in RelExclude

596a429

alancai98 requested a review from RCHowell January 13, 2024 00:13

RCHowell approved these changes Jan 16, 2024

View reviewed changes

alancai98 merged commit bb0342d into partiql-eval Jan 16, 2024
10 checks passed

alancai98 deleted the add-exclude-partiql-eval branch January 16, 2024 21:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `EXCLUDE` to `partiql-eval` #1320

Add `EXCLUDE` to `partiql-eval` #1320

alancai98 commented Dec 20, 2023 •

edited

Loading

alancai98 Dec 20, 2023

alancai98 Dec 20, 2023

github-actions bot commented Dec 20, 2023 •

edited

Loading

codecov-commenter commented Dec 20, 2023 •

edited

Loading

RCHowell Dec 21, 2023

alancai98 Dec 21, 2023

RCHowell Dec 21, 2023

alancai98 Dec 21, 2023

RCHowell Dec 21, 2023

alancai98 Dec 21, 2023

alancai98 Dec 21, 2023 •

edited

Loading

RCHowell Jan 11, 2024

alancai98 Jan 11, 2024

RCHowell Jan 11, 2024

alancai98 Jan 11, 2024

RCHowell left a comment

	SuccessTestCase(
	input = """
	SELECT *
	EXCLUDE
	t.a.b.c[*].field_x
	FROM [{
	'a': {
	'b': {
	'c': [
	{ -- c[0]; field_x to be removed
	'field_x': 0,
	'field_y': 0
	},
	{ -- c[1]; field_x to be removed
	'field_x': 1,
	'field_y': 1
	},
	{ -- c[2]; field_x to be removed
	'field_x': 2,
	'field_y': 2
	}
	]
	}
	}
	}] AS t
	""".trimIndent(),
	expected = bagValue(
	structValue(
	"a" to structValue(
	"b" to structValue(
	"c" to bagValue( // TODO: should be ListValue; currently, Rex.ExprCollection doesn't return lists
	structValue(
	"field_y" to int32Value(0)
	),
	structValue(
	"field_y" to int32Value(1)
	),
	structValue(
	"field_y" to int32Value(2)
	)
	)
	)
	)
	)
	)
	)
	)
	}

Add EXCLUDE to partiql-eval #1320

Add EXCLUDE to partiql-eval #1320

Conversation

alancai98 commented Dec 20, 2023 • edited Loading

Relevant Issues

Description

Other Information

License Information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 20, 2023 • edited Loading

Conformance comparison report

codecov-commenter commented Dec 20, 2023 • 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

alancai98 Dec 21, 2023 • edited Loading

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

RCHowell left a comment

Choose a reason for hiding this comment

Add `EXCLUDE` to `partiql-eval` #1320

Add `EXCLUDE` to `partiql-eval` #1320

alancai98 commented Dec 20, 2023 •

edited

Loading

github-actions bot commented Dec 20, 2023 •

edited

Loading

codecov-commenter commented Dec 20, 2023 •

edited

Loading

alancai98 Dec 21, 2023 •

edited

Loading