Ports tests from `partiql-lang-kotlin`'s PTS #38

alancai98 · 2022-10-05T00:22:59Z

Closes #7.

Ports tests from partiql-lang-kotlin PTS (https://github.com/partiql/partiql-lang-kotlin/tree/main/test/partiql-testscript/pts).

Associated Kotlin test runner PR: partiql/partiql-lang-kotlin#829 with an updated skip list.

Tests were ported using a script similar to what was used in porting the EvaluatorTestSuite.kt tests from partiql-lang-kotlin (#28). This was slightly trickier than the previous PR due to the number of tests being ported and because many of those tests did not have a PERMISSIVE/COERCE evaluation mode behavior defined.

TODO:

Self review of the PR and additional description since it's so large
ISL validation step of build fails (gives stack overflow error) on typed null values. Still sorting out if this is an error in the .isl validation file or in ISL
Clear up more of the typing behavior tests in the undecided directory

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

…is failures

… to separate dir

…ests

alancai98 · 2022-10-05T00:25:03Z

Build failing due to ISL validation check over typed nulls, which for some reason causes a stack overflow error. Still investigating before marking PR as ready for review.

alancai98 · 2022-10-07T20:56:24Z

Previous validation failure is due to amazon-ion/ion-schema-kotlin#169. Updated the required, nullable field to $any in the .isl to fix the stack overflow issue.

alancai98

Most of the diff is from these ported PTS files:

select-mysql.ion
- environment specified in lang-kotlin
- tests defined in lang-kotlin
select-postgresql.ion
- environment defined in lang-kotlin
- tests defined in lang-kotlin

alancai98 · 2022-10-07T22:50:47Z

partiql-tests-data-extended/README.md

@@ -0,0 +1,3 @@
+Directory contains tests that are not formally part of the PartiQL specification or SQL-92. These tests may contain


Decided to create this directory to hold tests that aren't part of the PartiQL or SQL-92 spec. Currently contains

Functions that are part of partiql-lang-kotlin but not part of the spec. Decided on what's in 'extended' from this doc: https://github.com/partiql/partiql-docs/blob/main/docs/builtins-sql.adoc

Numeric semantics that are operation and implementation-specific. Deferred on the modeling until Decide on modeling of different numeric semantics in test suite #39 is resolved

In the future, this could be a good directory to also put tests with ambiguous behavior that will be sorted out in the future.

alancai98 · 2022-10-07T22:52:00Z

partiql-tests-data/partiql-tests-schema.isl

@@ -122,7 +122,7 @@ type::{
    type: struct,
    fields: {
        result: { type: symbol, valid_values: [EvaluationSuccess], occurs: required },
-        output: nullable::{ occurs: required },
+        output: { type: $any, occurs: required },


Change was due to a stack overflow bug (existing issue: amazon-ion/ion-schema-kotlin#169) when validating typed nulls.

So you mean if we didn't have the bug we would go with nullable?

Yes, if the ion-schema-kotlin bug mentioned in amazon-ion/ion-schema-kotlin#169 didn't exist, then we could go with the previous approach (i.e. nullable::{ occurs: required }), which imo was more understandable. The change here is an equivalent way to specify a nullable, required field.

In that case can we add a TODO with reference to the bug? last time I checked we didn't have issue with adding comments in .ion files but not sure about .isl.

Added a TODO linking to the bug in 7065d48.

Regarding comments, it was mostly a concern related to standardizing the .ion test file formatting: #31. There shouldn't be an issue with adding comments to .isl for now.

alancai98 · 2022-10-07T22:57:31Z

partiql-tests-data/fail/syntax/query/select/select.ion

@@ -61,3 +61,79 @@
        result: SyntaxFail
    },
 }
+
+'select-star'::[


PTS didn't distinguish between different error categories, so ported some tests from PTS that were syntax errors. From my read of the SQL-92 BNF, select star with other select list items is not permitted. PostgreSQL does allow some of the following cases, while others like MySQL gives a syntax error. From a discussion with Almann, we couldn't think of a use case where a user would want select star with other projection items, so it may be something we want to disallow in a future spec RFC.

Does it makes sense to put these tests under partiql namespace to re-iterate the fact that they are not necessarily SQL conformed.

Yeah, it would probably make some sense to indicate that these tests are not necessarily SQL-conformant. I'll wrap these tests in a partiql namespace and add a comment in the next commit.

alancai98 · 2022-10-07T23:03:21Z

partiql-tests-data/fail/static-analysis/primitives/functions/char_length.ion

+  ],
+]
+
+invalid_function_args::[


Function calls with no schema present in STRICT/ERROR typing mode give errors with mistyped function arguments and return MISSING in PERMISSIVE/COERCE mode. But since a literal is provided in these tests, a static analysis pass could determine that the query has a mistyped argument and give an error.

From discussion with Almann, we determined that these cases (i.e. mistyped function args with statically-knowable types) should "fail-fast" and not wait till evaluation time. Similar reasoning applies to the other static-analysis tests ported in this PR. This type of static-time error will probably necessitate some RFC.

Are you saying the StaticAnalysisFail symbol may be a richer element, and the RFC will define this?

Not sure what you meant by a "richer element". Could you clarify?

The tests in the fail/static-analysis directory define tests that will fail at some stage of static analysis or type-checking. These particular tests with invalid number of arguments and invalid function arguments (known at static-time since they involve literals) are classified as static analysis fail tests in this PR. What I meant by needing an RFC was that this particular behavior isn't part of the current PartiQL spec and we should add an RFC to define the static analysis fail behavior.

I can cut a partiql-spec issue for static-analysis failures if the above makes sense.

alancai98 · 2022-10-07T23:07:16Z

partiql-tests-data/eval/primitives/path.ion

+  }
+]
+
+'repeated-field-on-struct'::[


The COERCE/permissive mode behavior is implementation-dependent as per section 4 of spec. Hence why only the ERROR/strict mode behavior tests are defined.

alancai98 · 2022-10-07T23:12:58Z

partiql-tests-data/eval/primitives/operators/like.ion

+  },
+  {
+      name:"More than one character given for ESCAPE",
+      statement:"SELECT * FROM `[true]` WHERE 'a' LIKE 'a' ESCAPE 'aa'",


This was the somewhat ambiguous case for an invalid function argument that's not due to typing error -- hence why the behavior is the same for both eval modes. PostgreSQL don't do a "smarter" static check for the escape pattern of a LIKE operator but gives an evaluator error.

partiql-tests-data-extended/README.md

RCHowell · 2022-10-10T21:20:46Z

partiql-tests-data/fail/static-analysis/primitives/functions/char_length.ion

+  ],
+]
+
+invalid_function_args::[


Are you saying the StaticAnalysisFail symbol may be a richer element, and the RFC will define this?

partiql-tests-data/eval/query/select/joins.ion

partiql-tests-data/eval/query/select/group-by.ion

Co-authored-by: R. C. Howell <[email protected]>

am357

I'm not sure how we can review this PR effectively; I think this is one of those PRs that we need to skip the thorough review and lean on asserting its correctness by application i.e. relying on test execution in the Kotlin runner—considering this, I did a cursory review and left couple comments.

am357 · 2022-10-14T22:22:12Z

partiql-tests-data/fail/syntax/query/select/select.ion

@@ -61,3 +61,79 @@
        result: SyntaxFail
    },
 }
+
+'select-star'::[


Does it makes sense to put these tests under partiql namespace to re-iterate the fact that they are not necessarily SQL conformed.

am357 · 2022-10-14T22:24:50Z

partiql-tests-data/partiql-tests-schema.isl

@@ -122,7 +122,7 @@ type::{
    type: struct,
    fields: {
        result: { type: symbol, valid_values: [EvaluationSuccess], occurs: required },
-        output: nullable::{ occurs: required },
+        output: { type: $any, occurs: required },


So you mean if we didn't have the bug we would go with nullable?

…omment

alancai98

I'm not sure how we can review this PR effectively; I think this is one of those PRs that we need to skip the thorough review and lean on asserting its correctness by application i.e. relying on test execution in the Kotlin runner—considering this, I did a cursory review and left couple comments.

Yeah, I agree given the number of tests being ported. Hopefully running the ported tests through the Kotlin test runner can give more confidence that they were ported correctly and had the spec-conformant behavior properly defined to begin with. Working on the PartiQL Rust implementation should also help us identify potential bugs in the tests.

alancai98 · 2022-10-14T22:41:04Z

partiql-tests-data/partiql-tests-schema.isl

@@ -122,7 +122,7 @@ type::{
    type: struct,
    fields: {
        result: { type: symbol, valid_values: [EvaluationSuccess], occurs: required },
-        output: nullable::{ occurs: required },
+        output: { type: $any, occurs: required },


Yes, if the ion-schema-kotlin bug mentioned in amazon-ion/ion-schema-kotlin#169 didn't exist, then we could go with the previous approach (i.e. nullable::{ occurs: required }), which imo was more understandable. The change here is an equivalent way to specify a nullable, required field.

alancai98 · 2022-10-14T22:43:42Z

partiql-tests-data/fail/syntax/query/select/select.ion

@@ -61,3 +61,79 @@
        result: SyntaxFail
    },
 }
+
+'select-star'::[


Yeah, it would probably make some sense to indicate that these tests are not necessarily SQL-conformant. I'll wrap these tests in a partiql namespace and add a comment in the next commit.

am357

Thank you for putting together this change; I understand it hasn't been easy.

alancai98 added 12 commits September 29, 2022 16:30

Initial port of pts tests

fd7d813

Remove replacement character from test name

a712ac3

Fix missing propagation of function args in pts tests

1ce48af

Move wrong number of argument function tests to static-analysis

203403e

Fix concat with missing operand eval tests

f660ad7

Change wrong num function argument tests assertions to static-analays…

8035f7b

…is failures

Fix some null and whitespace porting errors; move undecided int tests…

22e4f51

… to separate dir

Fix failing eval tests; move undecided, syntax, and static-analysis t…

2543f18

…ests

Add move wrong and invalid function arg tests to separate files

7430daf

Move pts tests to different files

5d34309

Delete main eval/pts.ion file

541e991

Change syntax test assertions

a06efc7

alancai98 self-assigned this Oct 5, 2022

alancai98 mentioned this pull request Oct 5, 2022

Update conformance test runner skip list with ported pts tests partiql/partiql-lang-kotlin#829

Merged

alancai98 added 3 commits October 6, 2022 16:39

Add extended set of tests; recategorized 'undecided' tests

24826ee

Change type to prevent stackoverflow error during validation

71524ef

Fix some coerce mode behaviors

652ce9e

Slight directory restructuring, whitespace fixes

bcebd4e

alancai98 commented Oct 7, 2022

View reviewed changes

alancai98 requested review from am357 and jpschorr October 7, 2022 23:20

alancai98 marked this pull request as ready for review October 7, 2022 23:20

RCHowell reviewed Oct 10, 2022

View reviewed changes

alancai98 and others added 2 commits October 10, 2022 15:20

Update partiql-tests-data-extended/README.md

67edc4d

Co-authored-by: R. C. Howell <[email protected]>

Restructure eval dirs

11b9ceb

alancai98 requested review from RCHowell October 10, 2022 22:40

am357 reviewed Oct 14, 2022

View reviewed changes

Wrap select-star syntax fail tests with 'partiql' namespace and add c…

8cba8f5

…omment

alancai98 commented Oct 14, 2022

View reviewed changes

alancai98 requested a review from am357 October 14, 2022 22:55

Add todo comment related to nullable in test schema

7065d48

am357 approved these changes Oct 14, 2022

View reviewed changes

RCHowell approved these changes Oct 17, 2022

View reviewed changes

alancai98 merged commit 29c49b9 into main Oct 17, 2022

alancai98 deleted the add-eval-tests branch October 17, 2022 20:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ports tests from `partiql-lang-kotlin`'s PTS #38

Ports tests from `partiql-lang-kotlin`'s PTS #38

alancai98 commented Oct 5, 2022 •

edited

Loading

alancai98 commented Oct 5, 2022

alancai98 commented Oct 7, 2022

alancai98 left a comment

alancai98 Oct 7, 2022

alancai98 Oct 7, 2022

alancai98 Oct 7, 2022

am357 Oct 14, 2022

alancai98 Oct 14, 2022

am357 Oct 14, 2022

alancai98 Oct 14, 2022

alancai98 Oct 7, 2022

am357 Oct 14, 2022

alancai98 Oct 14, 2022

alancai98 Oct 7, 2022

RCHowell Oct 10, 2022

alancai98 Oct 10, 2022

alancai98 Oct 10, 2022

alancai98 Oct 7, 2022

alancai98 Oct 7, 2022

RCHowell Oct 10, 2022

am357 left a comment

am357 Oct 14, 2022

am357 Oct 14, 2022

alancai98 left a comment

alancai98 Oct 14, 2022

alancai98 Oct 14, 2022

am357 left a comment

		@@ -0,0 +1,3 @@
		Directory contains tests that are not formally part of the PartiQL specification or SQL-92. These tests may contain

Ports tests from partiql-lang-kotlin's PTS #38

Ports tests from partiql-lang-kotlin's PTS #38

Conversation

alancai98 commented Oct 5, 2022 • edited Loading

alancai98 commented Oct 5, 2022

alancai98 commented Oct 7, 2022

alancai98 left a 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

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

am357 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alancai98 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

am357 left a comment

Choose a reason for hiding this comment

Ports tests from `partiql-lang-kotlin`'s PTS #38

Ports tests from `partiql-lang-kotlin`'s PTS #38

alancai98 commented Oct 5, 2022 •

edited

Loading