-
Notifications
You must be signed in to change notification settings - Fork 62
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
Eradicates premature failures when visiting error nodes #1453
Conversation
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. |
override fun visitRexOpErr(node: Rex.Op.Err, ctx: StaticType?): Operator { | ||
val message = buildString { | ||
this.appendLine(node.message) | ||
PlanPrinter.append(this, plan) | ||
} | ||
throw IllegalStateException(message) | ||
return ExprMissing(node.message) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No these are compile errors no matter what and this should be left as is. The rexOpMissing
is an error node that can be quiet-ed based on evaluation mode.
error("Unresolved cast $node") | ||
@OptIn(PartiQLValueExperimental::class) | ||
override fun visitRexOpCastUnresolved(node: Rex.Op.Cast.Unresolved, ctx: Unit): org.partiql.plan.Rex.Op { | ||
return rexOpErr("Unresolved cast: ${node.arg.type} to ${node.target}.", emptyList()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the public plan should have error nodes btw... I suggest PlanTransform use the antlr style error listeners. It's a bit silly to hide errors in a tree, then make the compiler later throw them.
We have two error nodes at the moment, but should only have one.
We have,
- rexOpError
- rexOpMissing
The rexOpError should NOT exist in the public plan IR, but rexOpMissing should. The rexOpMissing operation represents a typing error that either throws or becomes MISSING in the evaluator.
The rexOpError is an outright error that will never compile.
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:
|
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
|
Relevant Issues
Description
According to the PartiQL Specification:
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
and Code Style Guidelines? YES
License Information
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.