Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds support for PERMISSIVE vs STRICT #1353

Merged
merged 3 commits into from
Jan 30, 2024

Conversation

johnedquinn
Copy link
Member

@johnedquinn johnedquinn commented Jan 26, 2024

Relevant Issues

  • None

Description

  • Adds support for PERMISSIVE vs STRICT
  • In short, this implementation attempts to reduce as much computation/checking as possible. In STRICT mode, we never attempt to recover from a TypeCheckException. In Permissive mode, given that MISSING represents a type-check exception and is largely propagated through operations, we attempt to catch TypeCheckExceptions as infrequently as possible (only when we need to). We specifically catch the errors (according to the specification) when:
    • the value of an attribute in a struct results in a TypeCheckException.
    • an element in an array results in a TypeCheckException
    • creating the elements of a SELECT VALUE (AKA the constructor results in a TypeCheckException)
    • when the inputs to a function (that takes in ANY/MISSING) results in a TypeCheckException
    • top-level expressions (AKA under StatementQuery)
  • There are also multiple scenarios specified in the PartiQL Specification that handle mistyping cases that are edge-cases. For example: Iteration over a scalar value, iteration over an absent value, iteration over a tuple value, position variable on bags, out-of-bounds indexing, and more. In these scenarios, this implementation occasionally attempts to mitigate as many runtime checks as possible by compiling the nodes to permissive-specific nodes. See RelScanPermissive for example.
  • This PR also adds support for LIKE by copying much of the logic from EvaluatingCompiler. If requested, I can remove these additions and include them in a separate PR.

Testing

  • This PR follows the specification to find guidance on when to use MISSING and when to error. See the naming of tests to see the relevant PartiQL Specification sections.

Other Information

License Information

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

Copy link

github-actions bot commented Jan 26, 2024

Conformance comparison report-Cross Engine

Base (legacy) eval +/-
% Passing 92.54% 26.54% -66.00%
✅ Passing 5384 1544 -3840
❌ Failing 434 4274 3840
🔶 Ignored 0 0 0
Total Tests 5818 5818 0
Number passing in both: 1541

Number failing in both: 431

Number passing in legacy engine but fail in eval engine: 3843

Number failing in legacy engine but pass in eval engine: 3
⁉️ CONFORMANCE REPORT REGRESSION DETECTED ⁉️
The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact.
The following test(s) are failing in legacy but pass in eval. Before merging, confirm they are intended to pass:

Click here to see
  • nullif valid cases{first:"missing",second:"missing",result:missing}, compileOption: PERMISSIVE

  • nullif valid cases{first:"missing",second:"missing",result:missing}, compileOption: LEGACY

  • missing and true, compileOption: PERMISSIVE

Conformance comparison report-Cross Commit-LEGACY

Base (101c19b) 1d0cb4a +/-
% 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 (101c19b) but now fail: 0

Number failing in Base (101c19b) but now pass: 0

Conformance comparison report-Cross Commit-EVAL

Base (101c19b) 1d0cb4a +/-
% Passing 22.10% 26.54% 4.43%
✅ Passing 1286 1544 258
❌ Failing 4532 4274 -258
🔶 Ignored 0 0 0
Total Tests 5818 5818 0
Number passing in both: 1232

Number failing in both: 4220

Number passing in Base (101c19b) but now fail: 54

Number failing in Base (101c19b) but now pass: 312
⁉️ CONFORMANCE REPORT REGRESSION DETECTED ⁉️
The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact.
312 test(s) were previously failing but now pass. Before merging, confirm they are intended to pass
The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact.

@johnedquinn johnedquinn marked this pull request as ready for review January 26, 2024 17:24
Copy link
Contributor

@yliuuuu yliuuuu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR looks good to me. Have some out of scope comments but you don't have to address those in this PR.

private var isIndexable: Boolean = true

override fun open() {
val r = expr.eval(Record.empty)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this PR, but does this work for subquery?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question. I have a subquery PR out, and when I rebase, I'll take a look at that.

return rootEvaluated[keyString] ?: missingValue()
return rootEvaluated[keyString] ?: throw TypeCheckException()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tuple navigation in tuples with duplicate attributes When the tuple t has multiple attributes a, the tuple
path navigation t.a will return the first instance of a. Note that for tuples whose order is defined by schema, this is well-defined, for unordered tuples, it is implementation defined which attribute is returned in permissive mode or an error in type checking mode, which is described in Section 4.1

It seems like right now we don't have the notion of ordered vs unordered struct during runtime. Maybe just keep a todo note here?

@johnedquinn johnedquinn merged commit c16584f into partiql-plugin-impl Jan 30, 2024
10 checks passed
@johnedquinn johnedquinn deleted the partiql-plugin-modes branch January 30, 2024 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants