partiql / partiql-lang-kotlin

PartiQL libraries and tools in Kotlin.
https://partiql.org/
Apache License 2.0
538 stars 60 forks source link

Eradicates premature failures when visiting error nodes #1453

Closed johnedquinn closed 2 months ago

johnedquinn commented 5 months ago

Relevant Issues

Description

According to the PartiQL Specification:

For example, in the presence of schema validation, an PartiQL query processor can throw a compile-time error when given the path expression {a:1, b:2}.c. In a more important and common case, an PartiQL implementation can utilize the input data schema to prove that a path expression always returns MISSING and thus throw a compile-time error.

So, this translates to deferring failure until evaluation. While it is sub-optimal, it complies with the specification. We can always change the testing mechanism in the future.

Beyond that, I found that we had some bugs due to throwing errors when we saw unresolved nodes in the PlanTransform. The root cause is that we were appending unresolved nodes to the "causes" list of Rex.Op.Err. Therefore, with the introduction of Rex.Op.Err, yes -- we would temporarily handle errors, however, when we'd visit those errors, we would prematurely fail compilation.

This PR fixes both of the above situations.

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.

RCHowell commented 4 months ago

So, this translates to deferring failure until evaluation.

The specification says most commonly you want to error, but can defer. This is what Yingtao's PR for planner modes handles. I urge that signal mode is the default.

johnedquinn commented 4 months ago

So, this translates to deferring failure until evaluation.

The specification says most commonly you want to error, but can defer. This is what Yingtao's PR for planner modes handles. I urge that signal mode is the default.

The focus of this PR is to bridge the conformance gap (this PR passes an additional 20 tests). The existing conformance tests defer on erroring as does the existing evaluator. To accommodate your suggestion, there are two things I could do:

  1. Update the tests to allow for erroring during compilation.
  2. Or, add a flag to the engine (not default) to defer on erroring. Then, in the conformance tests runs, I can use the non-default flag to match the functionality of the existing evaluator.
RCHowell commented 4 months ago

I still don't quite understand the purpose here other than conformance tests being wrong. I believe Yingtao's planner work and the compiler implementation are correct.

HOWEVER

I think perhaps we should be making changes in the planner not the compiler.

Here are the Options afaik

  1. If we can confirm that the tests are correct, then the fix should go into the planner.
  2. If the tests are incorrect, then we can close this and open an issue in partiql-tests