feat: add VARBINARY type support #812
Conversation
jcamachor
requested review from
jacques-n,
cpcloud,
westonpace,
EpsilonPrime and
vbarua
as code owners
|
I'm supportive of this. Is their clear behavior when doing binary operations around length? I'm assuming a collection of functions will be coming after this? Have any examples? |
|
Thanks, @jacques-n . I was looking into this in more detail and I'm not sure truncation behavior will be consistent across engines. For example, in SQL Server, when converting to |
There was a problem hiding this comment.
The introduction of this type make sense to me. I'm happy to have this merged in by itself and then add functions that use them later.
One thing that we should do @jcamachor is define the https://substrait.io/extensions/#function-signature-compound-names for it.
| @@ -35,6 +35,7 @@ Compound type classes are type classes that need to be configured by means of a | |||
| | FIXEDCHAR<L> | A fixed-length unicode string of L characters. L must be within [1..2,147,483,647]. | L-character `string` | |||
| | VARCHAR<L> | A unicode string of at most L characters.L must be within [1..2,147,483,647]. | `string` with at most L characters | |||
| | FIXEDBINARY<L> | A binary string of L bytes. When casting, values shorter than L are padded with zeros, and values longer than L are right-trimmed. | L-byte `bytes` | |||
| | VARBINARY<L> | A binary string of L bytes. L must be within [1..2,147,483,647]. When casting, values longer than L are right-trimmed. | byte `bytes` with at most L bytes | |||
There was a problem hiding this comment.
Good idea clarifying the CAST behaviour. As you point out, this can be system dependent.
Substrait currently supports FIXEDBINARY and BINARY types. However, it does not support VARBINARY (BINARY VARYING), which is part of the ANSI SQL and supported by many other systems (SQL Server family, MySQL, Oracle, etc.)
This PR adds the
VARBINARYtype and corresponding literal to the spec.