Clarify that missing attributes should not result in errors

[Cali0707]

Fixes #1230

Proposed Changes

• Clarify that missing attributes should not result in an error

Note: this is actually something that the SDKs currently implement incorrectly, so if this PR is merged I will go and fix them.

Release Note

CESQL expressions should not return an error when there is a missing attribute

[duglin]

@lionelvillard any comments?

[duglin]

@slinkydeveloper any comment? Howdy! :-)

[duglin]

Not deep into the cesql stuff, but it seems like there are a few options for missingAttr="" (aside from erroring):
1 - returns true because we treat missingAttr the same as "". Which means missingAttr<>"" returns false
2 - returns false because we treat any SINGLE expression with an unknown attribute as false (so missingAttr<>"" would also return false, but NOT(missingAttr<>"") returns true)
3 - returns false because we treat an ENTIRE complex expression with an unknown attribute as false, so missingAttr="" or true returns false

What does missingAttr = 0 yield? I think the spec says to convert missingAttr to an int...but converting "" to an int generates an error, no? I can't seem to find it in the spec, but does an error get mapped to false? If so, is it for just that single expression or for the entire complex expression? (2 vs 3 in the list above)

[Cali0707]

Hmm you bring up a good point @duglin - one other option would be to have the missing attribute cast to the "default" value for the SINGLE expression it is found within.

For example, if we had missingAttr = "" it would default to a string "", and if we had missingAttr = 0, it would default to 0. I think this approach would allow people to still make comparisons ( and generally avoid having a special case when evalutating the expression as we already have type casting logic ).

However, I also like the idea of having the SINGLE expression with the unknown attribute evaluate to false, because then expressions like (type = "my.example.type") OR (missintattr = 50 would evaluate to the result I expect (the first expression already returned true).

WDYT?

[duglin]

I didn't offer a suggested answer in my previous comment because I just don't know yet :-) Way too many options and they all seem to have pros/cons.

Let me walk thru some samples and see what I think might be the right answer for each (xxx=missingAttribute):

expression	expected answer?
xxx=true	false
xxx=false	false. At first this makes sense, but down below "xxx" by itself is false, so is it odd that this isn't true?
xxx=5	false
xxx=0	false
(xxx=5 or true)	true due to the "or true"
xxx="abc"	false
xxx=""	false but could feel odd
not(xxx=5)	true due to xxx=5 returning false
not(xxx="")	true due to xxx="" returning false
xxx	false? is this a valid expression?
not(xxx)	true due to xxx returning false, if previous one is correct
xxx or false	false since both are false

If the above makes sense, then I guess I'm leaning towards the current single operation (if there is one) returning false

Note that people can always use the exists function if they don't like whatever semantics we come up with.
e.g. xxx="" might need to be: not(exists(xxx)) or xxx="" if they want "missing" to be the same as "".

[Cali0707]

@duglin I think I like the idea of the current single operation returning false.

e.g. xxx="" might need to be: not(exists(xxx)) or xxx="" if they want "missing" to be the same as "".

Maybe here we could follow normal SQL and provide a COALESCE function, which returns the value of the first espression that is non null (in our case, the first expression which is not a missing attribute).

So, someone could write COALESCE(xxx, "") to get the desired default behaviour

[duglin]

Seems that could work. I think if the Knative folks (/cc @lionelvillard ) are happy with whatever direction is chosen then that is probably a good choice. Maybe you could discuss this issue on today's call to see what other's think. I"m pretty sure some of them will have an opinion.

[duglin]

It might be clearer to say "the subexpression referencing the missing attribute MUST evaluate to 'false'". Use of the word "entire" could be easily misread as the "entire thing".... like I did at first 🤣

[duglin]

Approved (with edit) on the 2/22 call.

@Cali0707 let me know when to merge

[Cali0707]

@duglin should be good now! I just updated the wording as you suggested. Thanks for the suggestion, I think it's much clearer now!

[duglin]

@Cali0707 thanks