Expand tck tests #1227
Expand tck tests #1227
Conversation
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
| result: false | ||
|
|
||
| - name: With type coercion from bool (1) | ||
| expression: "TRUE LIKE 'tr%'" |
There was a problem hiding this comment.
where does the spec say "TRUE" is a bool? I only see "true"
There was a problem hiding this comment.
Ah I wasn't referring to the spec here, rather the existing tck tests:
spec/cesql/cesql_tck/casting_functions.yaml
Lines 30 to 35 in 755e458
Do you think I should switch this to true here?
There was a problem hiding this comment.
You might be in a better position than me since my only knowledge of the spec is based on this PR and my current reading of the spec to review your PR ;-) , but is the "expression" string meant to adhere to the syntax defined by the spec? If so, then it seems like it may need to be true or the spec needs to allow for both upper and lower case bools. Actually, the spec should probably say which things are case sensitive or not, in general.
There was a problem hiding this comment.
@duglin from https://github.com/cloudevents/spec/blob/main/cesql/spec.md#2-language-syntax it looks like the grammar is case-insensitive so both true and TRUE should be valid. Maybe we can open a new issue to add clarification to the spec where we mention the boolean literals that this is the case?
There was a problem hiding this comment.
I think that would be good since it may not be clear that "keywords" don't just mean function names, or operators, but also constants. And thanks for the pointer - I missed that.
| source: "http://www.some-website.com" | ||
| - name: Prefix OR Suffix filter (5) | ||
| expression: "id LIKE 'my%' OR myext LIKE '%ext'" | ||
| error: missingAttribute |
There was a problem hiding this comment.
Just curious, does the spec say that the stuff after the OR must be evaluated if the first part evaluates to 'true'? We should add a test for that, unless I missed it. In particular, if the 2nd part would generate an error if evaluated... what should happen if someone chooses to NOT evaluate it ?
There was a problem hiding this comment.
I'm not 100% sure, what I could find was:
Because every operator and function is total, an expression evaluation flow is defined statically and cannot be modified by expected or unexpected errors. Nevertheless CESQL includes the concept of errors: when an expression is evaluated, in case an error arises, the evaluator collects a list of errors, referred in this spec as error list, which is then returned together with the evaluated value of the CESQL expression.
So, I guess the spec does not explicitly specify whether or not there should be an error if the OR has an early return. Do you think we should specify this?
There was a problem hiding this comment.
It sure does seem like the spec should be clear on what true OR error results in... either true or error. Otherwise we'll have an interop issue. I would think most people would expect true but I'm not an expert in what query languages normally do for this. @lionelvillard @embano1 any thoughts?
There was a problem hiding this comment.
@duglin on the go sdk v2, when evaluating (type = 'dev.knative.example') OR (missingattribute = 'test') on an event with type: 'dev.knative.example' but no missingattribute I get: missing attribute 'missingattribute'
So, it seems like the current behaviour (at least for go) is that it reports the error, even if the first part evaluated to true
There was a problem hiding this comment.
Should I open an issue to clarify this behaviour, or do we want to open an issue to change this behaviour and make it behave like you think most people would expect (true instead of error)?
There was a problem hiding this comment.
It seems like from https://github.com/cloudevents/spec/blob/main/cesql/spec.md#41-error-handling that we would always return the error regardless of the result though (either fail fast, or continue and find the result and then return result and error)
There was a problem hiding this comment.
I think we should open an issue to get feedback from the group and then make it clear in the spec... and then align the go-sdk if needed.
| source: "http://www.some-website.com" | ||
|
|
||
| - name: Disjunctive Normal Form (1) | ||
| expresion: "(id = 'myId' AND type LIKE '%.success') OR (id = 'notmyId' AND source LIKE 'http://%' AND type LIKE '%.warning')" |
There was a problem hiding this comment.
This might answer my previous question :-) but where is that in the spec?
There was a problem hiding this comment.
As above, don't think that this is in the spec - I guess we should clarify whether or not this is an error that would occur, or whether it would be implementation specific
| type: "example.event.warning" | ||
| - name: Conjunctive Normal Form (4) | ||
| expression: "(id = 'myId' OR type LIKE '%.success') AND (id = 'notmyId' OR source LIKE 'https://%' OR type LIKE '%.warning')" | ||
| result: true |
There was a problem hiding this comment.
shouldn't this generate an error due to missing 'source' ?
There was a problem hiding this comment.
Yep my bad, I forgot that source was not set by default in these test - I'll fix it!
Signed-off-by: Calum Murray <cmurray@redhat.com>
|
@Cali0707 is this one ready to be merged? I assume so, and we have the follow-on issues to continue to clarifying questions. Right? |
@duglin I think if we want to merge this now, I should remove the tests which may or may not error on a OR, otherwise we might break tests on some of the SDKs depending on their implementation since that isn't specified currently. |
|
ok sounds good. Let me know when done and then we can vote on it during the Thursday call |
Signed-off-by: Calum Murray <cmurray@redhat.com>
|
@duglin I believe I have removed the two tests with issues in them. If you think this is ready, then I think it is ready to be voted on on Thursday. Thanks for all of your help on this! |
Fixes #1225
Proposed Changes