Exclude planner & plan evaluator from tests for features not yet implemented #586

dlurton · 2022-04-29T21:11:55Z

Originally part of #582 and based on #583 (which must be merged first), this PR primarily sets the target evaluator for all tests related to the following features to be the AST evaluator, thereby excluding the planner and plan evaluator from them. (These features are not within the current scope of work for the planner and new evaluator.) Note that the new planner and evaluator are not in this branch yet and will be introduced in a separate pull request. There are also a few new tests that I added to provide simpler test cases for a few scenarios that were a little difficult to get working with the new planner and evaluator.

ORDER BY
GROUP BY
HAVING
Aggregate functions (SUM, MAX, MIN, etc)
PIVOT
UNPIVOT

Also, some of the exception tests have been set to only execute under the CompilerPipeline because there is a behavior change surrounding a rare edge case: "ambiguous bindings", i.e. when you have a case-insensitive variable reference and more than one matching variable that varies only by letter case. Example: if you have fOo and FoO defined but you refer to variable foo (case-insenstive), CompilerPipeline etc would throw an exception at evaluation time if both are defined in the same lexical scope. The plan evaluator simply selects fOo and moves along merrily. (This behavior is consistent with Postgres, BTW.) This change is fully backward compatible because it allows queries to execute that wouldn't previously.

The PlannerPipeline does not support UndefinedVariableBehavior.MISSING, so tests related that also execute only under CompilerPipeline. This feature was intended for backward compatibility for a legacy customer who is stuck on a very old version of PartiQL for other reasons anyway.

Lastly, the PlannerPipeline does not (yet?) support the StaticTypeInferenceVisitorTransform, so any test that required the inferencer (i.e. EvaluatingCompilerNAryIntOverflowTests) to run has also been set to only run against the CompilerPipeline.

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

…CompilerPipeline and future PlannerPipeline

…nted features.

lang/test/org/partiql/lang/eval/EvaluatingCompilerExceptionsTest.kt

alancai98 · 2022-05-03T21:36:19Z

lang/test/org/partiql/lang/eval/EvaluatingCompilerExceptionsTest.kt

        """SELECT VALUE a FROM `[{v:5}]` AS item, @item.v AS a, @item.v AS a""",
        ErrorCode.EVALUATOR_AMBIGUOUS_BINDING,
        expectedErrorContext = propertyValueMapOf(1, 14, Property.BINDING_NAME to "a", Property.BINDING_NAME_MATCHES to "a, a"),
-        expectedPermissiveModeResult = "<<MISSING>>"
+        expectedPermissiveModeResult = "<<MISSING>>",
+        target = EvaluatorTestTarget.COMPILER_PIPELINE


This PartiQL statement doesn't seem to use any of the excluded features mentioned in the PR description. Should L103 be omitted?

You're correct, sir. Good catch. EDIT: HOWEVER... there is another slight difference in behavior where ambiguous binding names are concerned. Queries compiled with CompilerPipeline throw at evaluation-time when an ambiguous binding is detected. However, PlannerPipeline, etc, arbitrarily pick the first matching binding and disregard any ambiguity. This is not a backward compatibility issue since we are allowing queries to be evaluated that were not able to be evaluated previously.

Updated PR description to include this.

nit: may be worth leaving a comment for this test stating what you mentioned in this comment (or link the PR comment/description)

alancai98 · 2022-05-03T21:40:08Z

lang/test/org/partiql/lang/eval/EvaluatingCompilerExceptionsTest.kt

@@ -140,7 +144,8 @@ class EvaluatingCompilerExceptionsTest : EvaluatorTestBase() {
        // Same query as previous test--but DO NOT throw exception this time because of UndefinedVariableBehavior.MISSING
        runEvaluatorTestCase(
            sqlWithUndefinedVariable, expectedResult = "[null]",
-            compileOptionsBuilderBlock = { undefinedVariable(UndefinedVariableBehavior.MISSING) }
+            compileOptionsBuilderBlock = { undefinedVariable(UndefinedVariableBehavior.MISSING) },
+            target = EvaluatorTestTarget.COMPILER_PIPELINE


sqlWithUndefinedVariable corresponds to the query SELECT VALUE y FROM << 'el1' >> AS x, which doesn't have one of the "not implemented yet features". Is setting target to COMPILER_PIPELINE intentional?

Edited the PR description to include: "the PlannerPipeline does not support UndefinedVariableBehavior.MISSING, so tests related that have also execute only under CompilerPipeline. This feature was intended for backward compatibility for a legacy customer who is stuck on a very old version of PartiQL for other reasons anyway."

Thanks for editing the PR description for this.

nit: could also leave a comment for this test and the one on L169 stating what's mentioned here. Or could use a comment you had in QuotedIdentifierTests.kt

// planner & physical plan have no support for UndefinedVariableBehavior.MISSING (and may never)

lang/test/org/partiql/lang/eval/EvaluatingCompilerExceptionsTest.kt

lang/test/org/partiql/lang/eval/EvaluatingCompilerOffsetTests.kt

lang/test/org/partiql/lang/eval/QuotedIdentifierTests.kt

lang/test/org/partiql/lang/eval/SimpleEvaluatingCompilerTests.kt

lang/test/org/partiql/lang/eval/EvaluatorTestSuite.kt

Co-authored-by: Alan Cai <caialan@amazon.com>

alancai98

Thanks for providing more details in the PR description. Changes look good. Only have a couple minor nits to include some of the PR conversation in a comment.

alancai98 · 2022-05-06T23:00:07Z

lang/test/org/partiql/lang/eval/EvaluatingCompilerExceptionsTest.kt

        """SELECT VALUE a FROM `[{v:5}]` AS item, @item.v AS a, @item.v AS a""",
        ErrorCode.EVALUATOR_AMBIGUOUS_BINDING,
        expectedErrorContext = propertyValueMapOf(1, 14, Property.BINDING_NAME to "a", Property.BINDING_NAME_MATCHES to "a, a"),
-        expectedPermissiveModeResult = "<<MISSING>>"
+        expectedPermissiveModeResult = "<<MISSING>>",
+        target = EvaluatorTestTarget.COMPILER_PIPELINE


nit: may be worth leaving a comment for this test stating what you mentioned in this comment (or link the PR comment/description)

alancai98 · 2022-05-06T23:01:42Z

lang/test/org/partiql/lang/eval/EvaluatingCompilerExceptionsTest.kt

@@ -140,7 +144,8 @@ class EvaluatingCompilerExceptionsTest : EvaluatorTestBase() {
        // Same query as previous test--but DO NOT throw exception this time because of UndefinedVariableBehavior.MISSING
        runEvaluatorTestCase(
            sqlWithUndefinedVariable, expectedResult = "[null]",
-            compileOptionsBuilderBlock = { undefinedVariable(UndefinedVariableBehavior.MISSING) }
+            compileOptionsBuilderBlock = { undefinedVariable(UndefinedVariableBehavior.MISSING) },
+            target = EvaluatorTestTarget.COMPILER_PIPELINE


Thanks for editing the PR description for this.

nit: could also leave a comment for this test and the one on L169 stating what's mentioned here. Or could use a comment you had in QuotedIdentifierTests.kt

// planner & physical plan have no support for UndefinedVariableBehavior.MISSING (and may never)

…exclusions

dlurton added 2 commits April 29, 2022 12:07

Additional pipeline abstractions to share more code when testing the …

be3c31a

…CompilerPipeline and future PlannerPipeline

Exclude planner & plan evaluator from evaluation tests for unimpleme…

ea26eea

…nted features.

This was referenced Apr 29, 2022

PIG domains for planner and plan evaluator #584

Merged

[DRAFT] Experimental planner and physical plan evaluator #582

Closed

alancai98 mentioned this pull request May 2, 2022

Additional pipeline abstraction for test framework #583

Merged

alancai98 reviewed May 3, 2022

View reviewed changes

dlurton added this to the Basic Query Planner and Plan Evaluator milestone May 5, 2022

dlurton and others added 2 commits May 6, 2022 11:53

Apply suggestions from code review

12bcbcc

Co-authored-by: Alan Cai <caialan@amazon.com>

Apply feedback from Alan

624837f

dlurton requested a review from alancai98 May 6, 2022 18:59

alancai98 approved these changes May 6, 2022

View reviewed changes

Base automatically changed from physical-plan-staging-pipeline to physical-plan-staging May 9, 2022 17:35

Merge branch 'physical-plan-staging' into physical-plan-staging-test-…

81ef690

…exclusions

dlurton merged commit 35f659e into physical-plan-staging May 9, 2022

dlurton deleted the physical-plan-staging-test-exclusions branch May 9, 2022 18:19

alancai98 mentioned this pull request May 10, 2022

Make ThunkFactory generic #587

Merged

dlurton mentioned this pull request May 24, 2022

Add Plan Evaluator #592

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exclude planner & plan evaluator from tests for features not yet implemented #586

Exclude planner & plan evaluator from tests for features not yet implemented #586

dlurton commented Apr 29, 2022 •

edited

Loading

alancai98 May 3, 2022

dlurton May 6, 2022 •

edited

Loading

dlurton May 6, 2022

alancai98 May 6, 2022

alancai98 May 3, 2022

dlurton May 6, 2022 •

edited

Loading

alancai98 May 6, 2022

alancai98 left a comment

alancai98 May 6, 2022

alancai98 May 6, 2022

Exclude planner & plan evaluator from tests for features not yet implemented #586

Exclude planner & plan evaluator from tests for features not yet implemented #586

Conversation

dlurton commented Apr 29, 2022 • edited Loading

alancai98 May 3, 2022

Choose a reason for hiding this comment

dlurton May 6, 2022 • edited Loading

Choose a reason for hiding this comment

dlurton May 6, 2022

Choose a reason for hiding this comment

alancai98 May 6, 2022

Choose a reason for hiding this comment

alancai98 May 3, 2022

Choose a reason for hiding this comment

dlurton May 6, 2022 • edited Loading

Choose a reason for hiding this comment

alancai98 May 6, 2022

Choose a reason for hiding this comment

alancai98 left a comment

Choose a reason for hiding this comment

alancai98 May 6, 2022

Choose a reason for hiding this comment

alancai98 May 6, 2022

Choose a reason for hiding this comment

dlurton commented Apr 29, 2022 •

edited

Loading

dlurton May 6, 2022 •

edited

Loading

dlurton May 6, 2022 •

edited

Loading