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.

Sign up for GitHub

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 ExprError to new evaluator #1416

Closed
wants to merge 4 commits into from
Closed

Adds ExprError to new evaluator #1416

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4fba7e6
Adds a CompilationErrorHandling flag to evaluator
johnedquinn Apr 8, 2024
2cd476c
Adds support for evaluation of RexOpMissing
johnedquinn Apr 11, 2024
3c80d0e
Removes unnecessary evaluation session properties
johnedquinn Apr 16, 2024
5b2ab76
Reverts changes for RexOpErr
johnedquinn Apr 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions partiql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rex/ExprError.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package org.partiql.eval.internal.operator.rex

import org.partiql.errors.TypeCheckException
import org.partiql.eval.internal.Environment
import org.partiql.eval.internal.operator.Operator
import org.partiql.value.PartiQLValue
import org.partiql.value.PartiQLValueExperimental

internal class ExprError(
private val message: String,
) : Operator.Expr {

@OptIn(PartiQLValueExperimental::class)
override fun eval(env: Environment): PartiQLValue {
throw TypeCheckException(message)
}
}
Comment on lines +1 to +17
Copy link
Member

Choose a reason for hiding this comment

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

I don't believe we should be catching all errors. The planner inserts a missing rex on errors that are type check exceptions at compile time. The other errors are truly unrecoverable things such as unresolved functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the compiler accordingly

Copy link
Member Author

Choose a reason for hiding this comment

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

For some context, I initially added ExprError for both RexOpErr and RexOpMissing due to how it fixed failing conformance tests. But, now looking at the tests, I actually notice that those conformance tests are wrong. Specifically, in undefined-variable-behavior.ion, all of these tests failed when I didn't compile RexOpErr into an ExprError. That being said, PartiQL Spec Section 10 actually highlights why these tests are wrong. We are good to go ahead with this change, and I'll update those tests.

Copy link
Member

Choose a reason for hiding this comment

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

What I am saying is that the Compiler.kt fails to compile any query that contains a Rex.Op.Err.

If an error is a "mis-typing" error, then the planner inserts Rex.Op.Missing which the compiler may (or may not) recover from given the typing mode.

There should be no ExprError.kt

Loading