Add more parse tests from partiql-lang-kotlin; add omitted tests list

[alancai98]

Part of #5.

Ports 151 tests from partiql-lang-kotlin (total is now 314). Tests taken from

• ParserErrorsTest.kt
• SqlParserCorrelatedJoinTests.kt
• SqlParserJoinTest.kt
• Also adds some passing parse tests that weren't part of the Kotlin parse test suite (still parse w/ Kotlin's parser)

This PR also adds a file to keep track of some tests omitted from the Kotlin implementation's test suite: partiql-lang-kotlin-omitted-tests.md. I may need a reviewer to check over the tests I'm unsure about for possible inclusion and check the tests excluded (mostly due to not being part of the PartiQL/SQL spec).

Decided to add the DATE and TIME tests in an upcoming PR since there were a lot of tests involving the constructors.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[alancai98]

Rendered file.

Would be good to get another pair of eyes on this file, in particular the "Unsure if should be included" section for tests I was unsure to include (i.e. tests not part of the PartiQL/SQL spec but are defined as errors by the Kotlin implementation).

The "Tests excluded" (L3-33) may also be relevant to look at. These were tests that were

1. not in spec but likely will be sometime in the future such as DML, DDL, EXEC, LET, etc
2. tests not really parser related (e.g. performance, default parsed AST, custom type)

[alancai98]

Rendered file link updated following 26a5720 commit.

[jpschorr]

Is the space between the @ intended?

[alancai98]

Yes, it's intentional. Test taken from here. Would another test without the space be helpful to add?

[jpschorr]

While this is technically not spec-conformant1, it does not seem ambiguous. And PostGres 9.4-14 seems to support this syntax2.

Which is not to say that we should support it, but I'm also not sure of the utility of the negative test here.

[alancai98]

I'm also unsure of the utility of having this test as a negative test. Tested with MySQL and SQLite, which resulted in errors.

For now, deleted the test from fail/parse and added this test to the "unsure" section of partiql-lang-kotlin-omitted-tests.md.

[jpschorr]

This is just SELECT a FROM data which has had a 'typo' dropping the FROM, right?

[alancai98]

Yes, the FROM was missing from that query. Looking at that example again, it should probably be a parse error. I'll move it to the parse error tests in the next commit.

[jpschorr]

This is implementation-dependent as to whether it's a 'parse' error or a type/semantic error

[alancai98]

Agreed. I ended up moving most of the tests in this file to fail/semantic.

[jpschorr]

Most (though not all) of the fail tests added here seem to be legitimate cases where an error should be thrown. However, depending on implementation, it may not error during parse, but perhaps later in compile (type check for example).

Potentially semantic failures (e.g. not just unbalanced parens and missing clauses) should likely be moved to a set of "semantic" failure tests. Some implementations may fail them in the parse if they strictly implement SQL standard's EBNF, but some may fail them via other means.

[am357]

Is that because you consider them as a semantic error?

[alancai98]

I'm unsure it would be an error to begin with. The Kotlin implementation doesn't allow using * in a select list with other items.

I can't find a section of the PartiQL or SQL92 spec that forbids this. PostgreSQL + SQLite allow those cases. MySQL seems to only allow * as the first item in the select list (can have other columns following the *).

For now, leaving in the "unsure" section, but open to moving if there's something I missed from the spec.

[alancai98]

Thanks for the reviews!

26a5720 makes the following changes based on the feedback:

• Creates a semantic directory under partiql-test-data/fail for tests related to semantic/type-checking errors that aren't strictly parsing errors
• Moves some parse error tests w/ the "call" syntax to semantic directory
• Moves some JOIN tests that were misclassified as fail to pass
• Updates some of the test descriptions in partiql-lang-kotlin-omitted-tests.md
• Updates some JOIN parse error tests to use a more "error-explicit" name

[alancai98]

Yes, the FROM was missing from that query. Looking at that example again, it should probably be a parse error. I'll move it to the parse error tests in the next commit.

[alancai98]

I'm unsure it would be an error to begin with. The Kotlin implementation doesn't allow using * in a select list with other items.

I can't find a section of the PartiQL or SQL92 spec that forbids this. PostgreSQL + SQLite allow those cases. MySQL seems to only allow * as the first item in the select list (can have other columns following the *).

For now, leaving in the "unsure" section, but open to moving if there's something I missed from the spec.

[alancai98]

Yes, it's intentional. Test taken from here. Would another test without the space be helpful to add?

[alancai98]

These tests from L33-157 were mistakenly marked as fail in the first commit. Moved to pass in 26a5720.

[alancai98]

Rendered file link updated following 26a5720 commit.

[am357]

Nit.: Just occurred to me and might be applicable to other files as well—could all these tests be under one semantic namespace?

[alancai98]

Could you clarify "under one semantic namespace? Is the suggestion to wrap all the tests in a top-level namespace called semantic? Such as

semantic::[
    trim::[
        semantic::{
	        name: "TRIM too many arguments",
	        statement: "TRIM(BOTH ' ' FROM 'test' 2)",
	        assert: {
	            result: SemanticError
	        },
	    },
	    ... // other trim tests
	]
	extract::[
		... // extract tests
	]
	... // other tests
]

The most recent commit, 26a5720, added the semantic test annotation (different from parse or eval). I'm not too sure what having a semantic namespace provides.

[am357]

Sorry, perhaps I should've been more clear; I think the ambiguity is because I'm still trying to wrap my head around the test model. The motivation for my comment was to have the tests in such a way that we can avoid repeating the annotation (in this case semantic). This is not a blocker and I know requires more changes in case it makes sense. So feel free to merge and we can discuss further later.

[alancai98]

Ah, I get what you mean. The test model is still subject to change as we continue defining tests, especially as we define the formal schema and verification in #3. Regarding the test category annotation, I'd say the primary benefits are that

• the parsing of the test cases by the test runner doesn't need to keep track of the test category from an outer namespace or file path
  • e.g. partiql-lang-rust parsing of test case: https://github.com/partiql/partiql-lang-rust/blob/5acb46609fc5e9e57494d77b137c394fe7c60622/partiql-conformance-test-generator/src/lib.rs#L111-L141
• could allow for mixing different test categories within a file/namespace

[am357]

Yep I totally get it, the model is to be finalized. On the same note, I don't see the point of tracking the test category via the annotation when we have the Assertion. Basically, if there is a ParseError, I think it already gives enough information about the failure.

[alancai98]

Yeah there seems to be some unnecessary redundancy involving the assertion's result and the category annotation. The annotation only provides the test category, while the assertion result provides the test category and the expected result (e.g. ok or error). Some further discussion will be needed to see if this test category annotation is necessary.