clarify type casting in CESQL spec #1281

Cali0707 · 2024-05-09T20:02:41Z

Follow up on discussion in cloudevents/sdk-go#1046 (comment)

Proposed Changes

Clarify which type casts are required to be supported
Clarify how boolean -> string type casting works
Clarify how type casting is performed in the LIKE operator

Signed-off-by: Calum Murray <cmurray@redhat.com>

Cali0707 · 2024-05-09T20:02:52Z

cc @pierDipi @duglin

duglin · 2024-05-16T15:47:55Z

cesql/spec.md

+
+1. `Integer -> String`
+1. `String -> Integer`
+1. `String -> Boolean`


where are ones like this defined?

Ah I had assumed that they were defined elsewhere and I was just seeking to clarify which ones had to be supported. But, looking more closely now I don't see those defined anywhere...

Let me add those

Signed-off-by: Calum Murray <cmurray@redhat.com>

Cali0707 · 2024-05-20T13:40:14Z

cc @duglin @jskeet

Let me know if this makes sense given your feedback last week!

cesql/spec.md

jskeet · 2024-05-20T13:53:56Z

cesql/spec.md

@@ -353,6 +357,25 @@ left operand of the OR operation evalues to `true`, the right operand MUST NOT b

 #### 3.7. Type casting

+A CESQL engine MUST support the following type casts:


This talks about "returning" an error - elsewhere, the spec talks about "raising" an error but also returning a value. I suspect we need to be consistent here.

Yeah I see your point. Personally, I think "returning" makes more sense because it's not like an exception that is raised can be caught somewhere - it is just a second return value you get on every evaluation (which may be null/nil). WDYT? If you think "returning" makes more sense, I'll open a PR to change that everywhere, otherwise I'll switch these to be "raising" an error.

Sorry, I wasn't clear - I'm fine with the "raising" terminology, but the problem is that these casts don't say what the non-error part of the return is when there's a problem. If I cast "BOGUS" to Boolean, an error will be raised... but what's the value of the expression?

Oh I see your point - let me add some clarification here.

I added some clarification, could you recheck when you have time @jskeet ?

Yup, that definitely works.

cesql/spec.md

Signed-off-by: Calum Murray <cmurray@redhat.com>

duglin · 2024-05-22T14:33:41Z

Error looks real: cesql/spec.md: line 365: 'may' MUST be capitalized ('MAY')

Signed-off-by: Calum Murray <cmurray@redhat.com>

duglin · 2024-05-22T14:46:11Z

cesql/spec.md

@@ -278,6 +278,10 @@ For example, the pattern `_b*` will accept values `ab`, `abc`, `abcd1` but won't
 Both `%` and `_` can be escaped with `\`, in order to be matched literally. For example, the pattern `abc\%` will match
 `abc%` but won't match `abcd`.

+In cases where the left operand is not a `String`, it MUST be cast to a `String` before the comparison is made. 
+The pattern of the `LIKE` operator (that is, the right operand of the operator) MUST be a valid string predicate, 
+otherwise the parse MUST return a parse error.


Just to be clear... subject LIKE TRUE isn't valid because TRUE isn't a valid string, however, we talk about how there's implicit casting all over the place... so someone may wonder why TRUE isn't implicitly converted into "true". Am I correct in this thinking? Should we be explicit and say that casting isn't allowed in this case? Or perhaps should we allow it for consistency? @jskeet thoughts?

It would definitely be good to be clear. Even without the "LIKE" aspect, I can't tell offhand whether "TRUE" = true is casting the LHS to Boolean, or the RHS to String. I suspect it's the former, based on bullet 2 in the list in 3.7.

@duglin I think the main problem is that currently the LIKE expression isn't defined with an expression as the right operand. It is defined as:

expression NOT? LIKE stringLiteral

So, any value on the right operand which is not a string literal MUST be a parse error currently. We can change this, but I'm not sure if it makes sense. The whole point of a LIKE expression is to compare against a pattern, not a specific value. If someone were to compare against another string value wouldn't it just makes sense to use = or something like that?

The type casting is whenever an operator is defined for values, but the LIKE operator is currently only defined for a literal, not a value. I'm not 100% sure why this was the initial decision, but that's how it currently works

I think it's ok to leave it as "must be a string literal w/o casting", but let's be extra clear in the text that implicit type casting isn't allowed in this one spot. I agree defining it as stringLiteral sort of implies it, but saying that things should be implicitly case in all other spaces (even when the spec says the arg is a string) could be unclear to people.

duglin · 2024-05-22T15:05:21Z

cesql/spec.md

+
+| Definition           | Semantics                                                                                                                                                                                                                                        |
+| -------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
+| `Integer -> String`  | Returns the string representation of the integer value in base 10. If the value is less than 0, the '-' character is prepended to the result.                                                                                                    |


To be very anal... w/o leading zeros?

duglin · 2024-05-22T15:08:28Z

cesql/spec.md

+| `Integer -> String`  | Returns the string representation of the integer value in base 10. If the value is less than 0, the '-' character is prepended to the result.                                                                                                    |
+| `String -> Integer`  | Returns the result of interpreting the string as a 32 bit base 10 integer. The string MAY begin with a leading sign '+' or '-'. If the result will overflow or the string is not a valid integer an error is returned along with a value of `0`. |
+| `String -> Boolean`  | Returns `true` or `false` if the lower case representation of the string is exactly "true" or "false, respectively. Otherwise returns an error along with a value of `false`                                                                    |
+| `Boolean -> String`  | Returns `"true"` if the boolean is `true`, and `"false"` if the boolean is `false`.                                                                                                                                                              |


I think we should have a 3x3 table (or something) that shows all combinations in one spot. Then it's easy to see what's missing, like bool->int, int->bool

Signed-off-by: Calum Murray <cmurray@redhat.com>

Cali0707 · 2024-05-27T18:39:20Z

@duglin how does this look now?

duglin · 2024-05-28T17:48:05Z

cesql/spec.md

+| ------- | -------- | ------ | -------- |
+| Integer | N/A      | MUST   | MUST NOT |
+| String  | MUST     | N/A    | MUST     |
+| Boolean | MUST NOT | MUST   | N/A      |


This is a valid choice, but I'm curious why you didn't choose to map 1 with true and 0 with false ?

Tbh, I just based this off the current implementation in the SDKs...

The 1 mapping to true and 0 to false sounds reasonable though, especially because then the zero value for integers would be cast to false.

Which do you think is better?

I'm partial to allowing it, then everything can be implicitly cast to anything else - and I think you can then remove an error type.

@jskeet any concerns?

I'm okay with it - personally I do like being clear about expressions in terms of types, but that's mostly in C# etc. I haven't done significant amounts of SQL in years, so I don't know how it would "feel". If others are happy, that's fine by me.

@duglin if we do this:

map 1 with true and 0 with false

then how would we handle an integer value like 200? Would that be an error? Or to avoid errors would it be better to say that when casting from integer to boolean, any non-zero integer becomes true, and 0 becomes false.

Yup- I like that. Int->bool is zero vs non-zero. But bool->int is 0 vs 1

duglin · 2024-05-28T17:49:37Z

Just one question, but either way that one lands.... LGTM -thanks!

Signed-off-by: Calum Murray <cmurray@redhat.com>

duglin · 2024-05-30T16:26:43Z

Approved on the 5/30 call

clarify type casting in CESQL spec

9193215

Signed-off-by: Calum Murray <cmurray@redhat.com>

duglin reviewed May 16, 2024

View reviewed changes

fix: addressed feedback from meeting

6b5f67e

Signed-off-by: Calum Murray <cmurray@redhat.com>

jskeet reviewed May 20, 2024

View reviewed changes

address review feedback

4729d2e

Signed-off-by: Calum Murray <cmurray@redhat.com>

Cali0707 requested a review from jskeet May 20, 2024 15:15

clarify the default values when an error occurs

8812b57

Signed-off-by: Calum Murray <cmurray@redhat.com>

fix: may -> MAY

14028f4

Signed-off-by: Calum Murray <cmurray@redhat.com>

Cali0707 requested a review from duglin May 22, 2024 14:39

duglin reviewed May 22, 2024

View reviewed changes

Cali0707 added 3 commits May 22, 2024 11:19

clarify second operand in LIKE expression MUST be a string literal

7f90d78

Signed-off-by: Calum Murray <cmurray@redhat.com>

clarify leading zeros

4cbb1af

Signed-off-by: Calum Murray <cmurray@redhat.com>

add table of supported and unsupported type casts

e90f8c6

Signed-off-by: Calum Murray <cmurray@redhat.com>

Cali0707 requested a review from duglin May 27, 2024 18:39

duglin reviewed May 28, 2024

View reviewed changes

duglin approved these changes May 28, 2024

View reviewed changes

add boolean <-> integer casting

c755764

Signed-off-by: Calum Murray <cmurray@redhat.com>

duglin merged commit 66c067b into cloudevents:main May 30, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clarify type casting in CESQL spec #1281

clarify type casting in CESQL spec #1281

Cali0707 commented May 9, 2024

Cali0707 commented May 9, 2024

duglin May 16, 2024

Cali0707 May 16, 2024

Cali0707 commented May 20, 2024

jskeet May 20, 2024

Cali0707 May 20, 2024

jskeet May 20, 2024

Cali0707 May 20, 2024

Cali0707 May 20, 2024

jskeet May 21, 2024

duglin commented May 22, 2024

duglin May 22, 2024

jskeet May 22, 2024

Cali0707 May 22, 2024

Cali0707 May 22, 2024

duglin May 22, 2024 •

edited

duglin May 22, 2024

duglin May 22, 2024 •

edited

Cali0707 commented May 27, 2024

duglin May 28, 2024

Cali0707 May 28, 2024

duglin May 28, 2024

jskeet May 29, 2024

Cali0707 May 29, 2024

duglin May 29, 2024

duglin commented May 28, 2024

duglin commented May 30, 2024

		@@ -353,6 +357,25 @@ left operand of the OR operation evalues to `true`, the right operand MUST NOT b

		#### 3.7. Type casting

		A CESQL engine MUST support the following type casts:

clarify type casting in CESQL spec #1281

clarify type casting in CESQL spec #1281

Conversation

Cali0707 commented May 9, 2024

Proposed Changes

Cali0707 commented May 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cali0707 commented May 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duglin commented May 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duglin May 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duglin May 22, 2024 • edited

Choose a reason for hiding this comment

Cali0707 commented May 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duglin commented May 28, 2024

duglin commented May 30, 2024

duglin May 22, 2024 •

edited

duglin May 22, 2024 •

edited