-
Notifications
You must be signed in to change notification settings - Fork 583
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
Update CESQL tck tests to match spec changes #1291
Update CESQL tck tests to match spec changes #1291
Conversation
Signed-off-by: Calum Murray <[email protected]>
@@ -25,3 +25,4 @@ The `error` values could be any of the following: | |||
* `cast`: Casting error | |||
* `missingFunction`: Addressed a missing function | |||
* `functionEvaluation`: Error while evaluating a function | |||
* `missingAttribute`: Error due to a missing attribute |
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 think you added a generic error message to the spec - should we test for that too? It might end up being more of an impl-specific error case. If it's too hard to put into this generic tck stuff then I'm ok with skipping it. Was just curious.
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 think we could mention it in the README
for completeness, but I can't think of any use cases where we would actually use it
expression: -TRUE | ||
result: -1 |
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.
Correct result, but funny
cesql/cesql_tck/negate_operator.yaml
Outdated
result: 0 | ||
error: cast | ||
error: match |
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.
What does "match' mean here - I don't see it listed as a valid error? I expected "missingAttribute"
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.
Tbh I'm not sure how I ended up with match
here...
You are totally correct, it should be missingAttribute
Just a few minor questions... otherwise LGTM |
* Add generic error to README * Fix match error -> missingAttribute error Signed-off-by: Calum Murray <[email protected]>
Approved on the 6/6 call |
With all of the CESQL spec changes recently, the spec needed some updates to reflect what had changed
Proposed Changes
ABS
function overflow error