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

Closed

Conversation

johnedquinn
Copy link
Member

@johnedquinn johnedquinn commented Apr 8, 2024

Relevant Issues

  • None

Description

  • Adds a CompilationErrorHandling flag to evaluator for the sake of handling all of the "handle always missing" errors. All of the conformance tests do not handle the "handle always missing" errors. Therefore, to be 1:1 with the EvaluatingCompiler, I've added a flag that a user can override to specify the compilation behavior for RexOpErr.
  • From the Conformance Report:

Conformance comparison report-Cross Commit-EVAL

Base (41c00fd) 6a9f18c +/-
% Passing 85.70% 89.05% 3.35%
✅ Passing 4986 5181 195
❌ Failing 832 637 -195
🔶 Ignored 0 0 0
Total Tests 5818 5818 0

Conformance comparison report-Cross Engine

Base (legacy) eval +/-
% Passing 92.51% 89.05% -3.45%
✅ Passing 5382 5181 -201
❌ Failing 436 637 201
🔶 Ignored 0 0 0
Total Tests 5818 5818 0

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.

@johnedquinn johnedquinn marked this pull request as ready for review April 8, 2024 22:00
@johnedquinn johnedquinn marked this pull request as draft April 9, 2024 00:05
@johnedquinn johnedquinn force-pushed the v1-conformance-compilation-error branch from b4e5dfb to 88b9e26 Compare April 11, 2024 21:55
@johnedquinn johnedquinn marked this pull request as ready for review April 11, 2024 22:03
@johnedquinn johnedquinn force-pushed the v1-conformance-compilation-error branch from 88b9e26 to 4304118 Compare April 16, 2024 21:36
@johnedquinn johnedquinn marked this pull request as draft April 16, 2024 21:36
@johnedquinn johnedquinn changed the title Adds a CompilationErrorHandling flag to evaluator Adds ExprError to new evaluator Apr 16, 2024
@johnedquinn johnedquinn marked this pull request as ready for review April 16, 2024 21:52
@johnedquinn johnedquinn requested a review from RCHowell April 16, 2024 22:44
Comment on lines +1 to +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)
}
}
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

@johnedquinn johnedquinn force-pushed the v1-conformance-compilation-error branch from 4304118 to 5b2ab76 Compare April 18, 2024 18:29
@johnedquinn johnedquinn requested a review from RCHowell April 18, 2024 18:35
Copy link
Member

@RCHowell RCHowell left a comment

Choose a reason for hiding this comment

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

Compiler should blow up on Rex.Op.Err

@RCHowell
Copy link
Member

RCHowell commented Apr 22, 2024

Closed in favor of #1437

@RCHowell RCHowell closed this Apr 22, 2024
@johnedquinn johnedquinn deleted the v1-conformance-compilation-error branch April 22, 2024 16:28
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