Implement EXCLUDE in `EvaluatingCompiler` #1249

alancai98 · 2023-10-18T23:36:39Z

Relevant Issues

partiql/partiql-lang#27 (still experimental until the RFC is merged).

Description

Implementation of EXCLUDE operator (partiql/partiql-lang#27) in the EvaluatingCompiler. A future PR will implement EXCLUDE in the PhysicalPlanCompiler, targeting the partiql-eval branch.

Other Information

Updated Unreleased Section in CHANGELOG: [Yes]
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.

github-actions · 2023-10-18T23:43:18Z

Conformance comparison report

	Base (`9ad3599`)	`e6ccf41`	+/-
% Passing	92.33%	92.33%	0.00%
✅ Passing	5372	5372	0
❌ Failing	446	446	0
🔶 Ignored	0	0	0
Total Tests	5818	5818	0

Number passing in both: 5372

Number failing in both: 446

Number passing in Base (9ad3599) but now fail: 0

Number failing in Base (9ad3599) but now pass: 0

codecov-commenter · 2023-10-18T23:48:22Z

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (9ad3599) 72.03% compared to head (f27fe71) 72.20%.
Report is 3 commits behind head on main.

Files	Patch %	Lines
...kotlin/org/partiql/lang/eval/EvaluatingCompiler.kt	90.30%	6 Missing and 10 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1249      +/-   ##
============================================
+ Coverage     72.03%   72.20%   +0.16%     
- Complexity     2058     2100      +42     
============================================
  Files           221      222       +1     
  Lines         15869    16019     +150     
  Branches       2856     2891      +35     
============================================
+ Hits          11432    11567     +135     
- Misses         3630     3635       +5     
- Partials        807      817      +10

Flag	Coverage Δ
CLI	`13.43% <ø> (ø)`
EXAMPLES	`80.28% <ø> (ø)`
LANG	`80.57% <90.75%> (+0.10%)`	⬆️

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.

alancai98 · 2023-11-15T01:10:15Z

Based on some upcoming plans to deprecate EvaluatingCompiler, I'll look to port this reference implementation over to PhysicalPlanCompiler.

RCHowell · 2023-11-29T21:28:38Z

partiql-lang/src/main/kotlin/org/partiql/lang/eval/EvaluatingCompiler.kt

+    /**
+     * Represents all the exclusions at the current level and other nested levels.
+     */
+    internal data class RemoveAndOtherSteps(val remove: Set<PartiqlAst.ExcludeStep>, val steps: Map<PartiqlAst.ExcludeStep, RemoveAndOtherSteps>) {


Could you please help me understand how the steps are modeled? Why is there a set of steps and then a map from steps to more? This seems like an odd way to model what looks to be a cluster of trees

Understanding this will help be better see what's going on in compileExcludeClause

To give more context, the EXCLUDE paths are associative (order in which they're specified shouldn't matter) so all of the exclude paths can be evaluated in parallel. Associative exclude paths proved to be less confusing especially when dealing with collection index exclude steps. For example, if we have EXCLUDE t.a[1], t.a[2].field, t.a's 1st index and the field attribute of t.a's 2nd index will be removed in parallel rather than iteratively removing t.a[1] and then t.a[2].field after removing t.a[1].

I used RemoveAndOtherSteps to make it easier to process these exclude paths in parallel. At each level of the exclude paths (index of exclude path step), we remove some tuple attributes and collection indexes corresponding to the exclude paths that end at the current level. This is modeled by the remove set.

There may also be some exclude paths that have additional steps (so their final steps are at a deeper level). This is modeled using a map of exclude steps to the other RemoveAndOtherSteps to group all the exclude paths that have the same exclude step at the current level.

For example, let's say we have exclude paths (ignoring the exclude path root) of

a.b, -- exclude path 1 x.y.z1, -- exclude path 2 x.y.z2 -- exclude path 3 ^ ^ ^ Level 1 2 3

These exclude paths would be converted to the following in RemoveAndOtherSteps.

// For demonstration purposes, the syntax '<string>' corresponds to the exclude tuple attribute step of <string> RemoveAndOtherSteps( // Level 1 (no exclusions at level 1) remove = emptySet(), steps = mapOf( 'a' to RemoveAndOtherSteps( // Level 2 for paths that have `'a'` in Level 1 remove = setOf('b'), // path `a.b` has final step at level 2 steps = emptyMap() ), 'x' to RemoveAndOtherSteps( // Level 2 for paths that have `'x'` in Level 1 remove = emptySet(), steps = mapOf( 'y' to RemoveAndOtherSteps( // Level 3 for paths that have `'y'` in Level 2 and `'x'` in Level 1 remove = setOf('z1', 'z2'), // paths `x.y.z1` and `x.y.z2` have final step at level 3 steps = emptyMap() ) ) ), ) )

Some of the other EXCLUDE path merging/subsumption tests shows the relationship between EXCLUDE paths and this data structure: https://github.com/partiql/partiql-lang-kotlin/blob/2d05d8a13237ed0a0ed1e1d5cec53171ca8d8b28/partiql-lang/src/test/kotlin/org/partiql/lang/eval/EvaluatingCompilerExcludeTests.kt#L841-L1074C14.

I see this is just trees where you've marked leaves. It's fine as is, just that on the surface the naming and factoring is more complicated than it need be so kind of confused me.

Say a "red" node means it gets excluded, this is all you need

Source

digraph G { b[color=red] z1[color=red] z2[color=red] a -> b x -> y y -> z1 y -> z2 }

I agree that the naming/factoring of RemoveAndOtherSteps could be initially confusing. Since this is currently internal, I'll leave it as it is. I plan to take another look at refactoring this as part of porting EXCLUDE to the plan compiler in #1280.

alancai98 · 2023-11-30T20:43:44Z

partiql-lang/src/main/kotlin/org/partiql/lang/eval/EvaluatingCompiler.kt

+    /**
+     * Represents all the exclusions at the current level and other nested levels.
+     */
+    internal data class RemoveAndOtherSteps(val remove: Set<PartiqlAst.ExcludeStep>, val steps: Map<PartiqlAst.ExcludeStep, RemoveAndOtherSteps>) {


To give more context, the EXCLUDE paths are associative (order in which they're specified shouldn't matter) so all of the exclude paths can be evaluated in parallel. Associative exclude paths proved to be less confusing especially when dealing with collection index exclude steps. For example, if we have EXCLUDE t.a[1], t.a[2].field, t.a's 1st index and the field attribute of t.a's 2nd index will be removed in parallel rather than iteratively removing t.a[1] and then t.a[2].field after removing t.a[1].

I used RemoveAndOtherSteps to make it easier to process these exclude paths in parallel. At each level of the exclude paths (index of exclude path step), we remove some tuple attributes and collection indexes corresponding to the exclude paths that end at the current level. This is modeled by the remove set.

There may also be some exclude paths that have additional steps (so their final steps are at a deeper level). This is modeled using a map of exclude steps to the other RemoveAndOtherSteps to group all the exclude paths that have the same exclude step at the current level.

alancai98 · 2023-11-30T22:46:10Z

partiql-lang/src/main/kotlin/org/partiql/lang/eval/EvaluatingCompiler.kt

+    /**
+     * Represents all the exclusions at the current level and other nested levels.
+     */
+    internal data class RemoveAndOtherSteps(val remove: Set<PartiqlAst.ExcludeStep>, val steps: Map<PartiqlAst.ExcludeStep, RemoveAndOtherSteps>) {


For example, let's say we have exclude paths (ignoring the exclude path root) of

a.b, -- exclude path 1 x.y.z1, -- exclude path 2 x.y.z2 -- exclude path 3 ^ ^ ^ Level 1 2 3

These exclude paths would be converted to the following in RemoveAndOtherSteps.

// For demonstration purposes, the syntax '<string>' corresponds to the exclude tuple attribute step of <string> RemoveAndOtherSteps( // Level 1 (no exclusions at level 1) remove = emptySet(), steps = mapOf( 'a' to RemoveAndOtherSteps( // Level 2 for paths that have `'a'` in Level 1 remove = setOf('b'), // path `a.b` has final step at level 2 steps = emptyMap() ), 'x' to RemoveAndOtherSteps( // Level 2 for paths that have `'x'` in Level 1 remove = emptySet(), steps = mapOf( 'y' to RemoveAndOtherSteps( // Level 3 for paths that have `'y'` in Level 2 and `'x'` in Level 1 remove = setOf('z1', 'z2'), // paths `x.y.z1` and `x.y.z2` have final step at level 3 steps = emptyMap() ) ) ), ) )

Some of the other EXCLUDE path merging/subsumption tests shows the relationship between EXCLUDE paths and this data structure: https://github.com/partiql/partiql-lang-kotlin/blob/2d05d8a13237ed0a0ed1e1d5cec53171ca8d8b28/partiql-lang/src/test/kotlin/org/partiql/lang/eval/EvaluatingCompilerExcludeTests.kt#L841-L1074C14.

alancai98 · 2023-11-30T22:52:47Z

partiql-lang/src/main/kotlin/org/partiql/lang/eval/EvaluatingCompiler.kt

+     * Represents all the compiled `EXCLUDE` paths that start with the same [CompiledExcludeExpr.root]. Notably,
+     * redundant paths (i.e. exclude paths that exclude values already excluded by other paths) will be removed.
+     */
+    internal data class CompiledExcludeExpr(val root: PartiqlAst.Identifier, val exclusions: RemoveAndOtherSteps)


From offline discussion, some of this PR's code in EvaluatingCompiler can be reused when porting EXCLUDE to the physical plan compiler. Some logic here will be refactored in an upcoming PR that adds EXCLUDE to the other compiler.

…dOtherSteps'

RCHowell · 2023-12-06T18:43:35Z

partiql-lang/src/main/kotlin/org/partiql/lang/eval/EvaluatingCompiler.kt

+    /**
+     * Represents all the exclusions at the current level and other nested levels.
+     */
+    internal data class RemoveAndOtherSteps(val remove: Set<PartiqlAst.ExcludeStep>, val steps: Map<PartiqlAst.ExcludeStep, RemoveAndOtherSteps>) {


I see this is just trees where you've marked leaves. It's fine as is, just that on the surface the naming and factoring is more complicated than it need be so kind of confused me.

Say a "red" node means it gets excluded, this is all you need

Source

digraph G { b[color=red] z1[color=red] z2[color=red] a -> b x -> y y -> z1 y -> z2 }

alancai98 self-assigned this Oct 18, 2023

alancai98 force-pushed the exclude-in-evaluator-poc branch from f9ab32c to 05a6963 Compare October 20, 2023 23:55

alancai98 force-pushed the exclude-in-evaluator-poc branch from 05a6963 to 8838897 Compare November 16, 2023 00:07

alancai98 changed the title ~~WIP Implement EXCLUDE in EvaluatingCompiler~~ Implement EXCLUDE in EvaluatingCompiler Nov 28, 2023

alancai98 requested a review from RCHowell November 28, 2023 19:32

RCHowell reviewed Nov 29, 2023

View reviewed changes

alancai98 commented Nov 30, 2023

View reviewed changes

alancai98 mentioned this pull request Dec 2, 2023

Add exclude physical plan compiler impl #1280

Closed

alancai98 added 4 commits December 4, 2023 13:11

Implement EXCLUDE in EvaluatingCompiler

7ab4f12

A few more tests; add subsumption tests

00d77b4

Update changelog, add docs

a77a047

Fix some typos in 'compileExcludeClause'; add more docs for 'RemoveAn…

f12858f

…dOtherSteps'

alancai98 force-pushed the exclude-in-evaluator-poc branch from 2d05d8a to f12858f Compare December 4, 2023 21:11

alancai98 requested a review from RCHowell December 4, 2023 21:12

alancai98 added 2 commits December 5, 2023 16:20

Remove Named's value property; refactor evalExclude

85d3ace

Fix NamedExprValue impl

f27fe71

RCHowell approved these changes Dec 6, 2023

View reviewed changes

alancai98 merged commit f8f6b2a into main Dec 6, 2023
10 checks passed

alancai98 deleted the exclude-in-evaluator-poc branch December 6, 2023 19:51

alancai98 mentioned this pull request Dec 20, 2023

Add EXCLUDE to partiql-eval #1320

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement EXCLUDE in `EvaluatingCompiler` #1249

Implement EXCLUDE in `EvaluatingCompiler` #1249

alancai98 commented Oct 18, 2023 •

edited

Loading

github-actions bot commented Oct 18, 2023 •

edited

Loading

codecov-commenter commented Oct 18, 2023 •

edited

Loading

alancai98 commented Nov 15, 2023

RCHowell Nov 29, 2023

RCHowell Nov 29, 2023

alancai98 Nov 30, 2023

alancai98 Nov 30, 2023

RCHowell Dec 6, 2023

alancai98 Dec 6, 2023

alancai98 Nov 30, 2023

alancai98 Nov 30, 2023

alancai98 Nov 30, 2023

RCHowell Dec 6, 2023

Implement EXCLUDE in EvaluatingCompiler #1249

Implement EXCLUDE in EvaluatingCompiler #1249

Conversation

alancai98 commented Oct 18, 2023 • edited Loading

Relevant Issues

Description

Other Information

License Information

github-actions bot commented Oct 18, 2023 • edited Loading

Conformance comparison report

codecov-commenter commented Oct 18, 2023 • edited Loading

Codecov Report

alancai98 commented Nov 15, 2023

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

Choose a reason for hiding this comment

Implement EXCLUDE in `EvaluatingCompiler` #1249

Implement EXCLUDE in `EvaluatingCompiler` #1249

alancai98 commented Oct 18, 2023 •

edited

Loading

github-actions bot commented Oct 18, 2023 •

edited

Loading

codecov-commenter commented Oct 18, 2023 •

edited

Loading