Sorry I didn’t fully read your message, Gonzalo. I’m very busy this week, so I
am focusing on strategy rather than implementation.
The right solution is probably to link the map of type coercions to the
connection’s conformance. That will allow Calcite to receive SQL and pretend to
be (say) Postgres; the validator will allow implicitly or explicitly converting
from type t1 to type t2 if and only if Postgres does.
> On Feb 23, 2024, at 12:07 PM, Mihai Budiu <mbu...@gmail.com> wrote:
>
> It looks to me like whatever semantics Pinot desires, it would still be
> useful to have pluggable semantics for casts. So perhaps a new issue should
> be opened for that; it looks like the existing framework isn't sufficient.
>
> In the meantime we can probably also accept the solution in CALCITE-6210
> (after I apply the suggested fixes from reviews).
>
> Mihai
> ________________________________
> From: Gonzalo Ortiz Jaureguizar <golthir...@gmail.com>
> Sent: Thursday, February 22, 2024 11:53 PM
> To: dev@calcite.apache.org <dev@calcite.apache.org>
> Subject: Re: Remove a specific type coersion rule
>
>> When I read CALCITE-6210 it wasn’t clear that you wanted Calcite to give
> a validation error when someone tries to cast a VARCHAR to a VARBINARY. I
> agree that that is the desirable behavior. (And I believe it is consistent
> with the SQL standard.
>
> In Apache Pinot we are still discussing the correct behavior here. We are
> migrating from a mostly in-house and limited (no join, no window function,
> etc) query engine to one that is in large part based on Apache Calcite and
> our plan is to move more and more logic to the Calcite model instead of
> ours. We are not very original and they are called V1 and V2 respectively.
>
> In V1 we had our own logic to cast from String to Binary and in fact this
> logic is not super clear and dependent on the content. In V2 we are trying
> to be saner and we always try to be closer to other databases (usually
> Postgres). So one of the ideas is to just embrace CALCITE-6210, but that
> would mean that users that migrate from V1 to V2 would have to change their
> queries. Otherwise they will receive incorrect values (because the
> semantics will change). Therefore there is a safer second option which is
> to forbid that cast and force users to either use binary literals
> (`x'abcd') or our own function (`hexToBytes('abcd')`). The third option
> would be to be able to modify calcite behavior to implement our own casting
> logic, but I didn't have the feeling that CALCITE-6210 was going to open
> that possibility.
>
>> Can you amend the Jira case so that it clearly the goal?
>
> As said, our goal is not clear yet. IMHO CALCITE-6210 is fine as it is
> right now. At the end of the day right now there is a bug during
> simplification. Whether that simplification semantics can be plugable or
> not is not part of the issue, right?
>
>> Reading the code, I see that
> https://issues.apache.org/jira/browse/CALCITE-3550 was trying to provide a
> way for people to change the coercion rules. Does that solution meet your
> needs?
>
> That is what I've tried, right? What I'm trying to report here is that some
> parts of the code are just ignoring SqlTypeAssignmentRule. Specifically
> AbstractTypeCoercion.commonTypeForBinaryComparison
> <https://github.com/apache/calcite/blob/main/core/src/main/java/org/apache/calcite/sql/validate/implicit/AbstractTypeCoercion.java#L493>
> and AbstractTypeCoersion.implicitCast
> <https://github.com/apache/calcite/blob/main/core/src/main/java/org/apache/calcite/sql/validate/implicit/AbstractTypeCoercion.java#L719>
> assume
> some castings are always going to be allowed and in fact completely
> ignore assignment rules. AbstractTypeCoercion.needToCast
> <https://github.com/apache/calcite/blob/c49792f9c72159571f898c5fca1e26cba9870b07/core/src/main/java/org/apache/calcite/sql/validate/implicit/AbstractTypeCoercion.java#L256>
> seems
> to be correct, but that is not the code that is being called. So removed
> type assignment rules may or may not be honored depending on the code path.
>
>> I will amend the Jira case when we decide what the implementation should
> do. I can change the corresponding PR to give an error instead, but that is
> not a solution for the Pinot implementation either
>
> As said above, I understand CALCITE-6210 as "there is a bug in this
> casting, lets fix them". It would be great if we could make casting
> semantics plugable, but that is not the reason why CALCITE-6210 was
> open, right?
>
> El vie, 23 feb 2024 a las 1:34, Mihai Budiu (<mbu...@gmail.com>) escribió:
>
>> When I filed the Jira issue I was planning to give an error, but I
>> discovered that several dialects implement this cast, so my PR actually
>> implements the behavior from Postgres.
>>
>> I will amend the Jira case when we decide what the implementation should
>> do. I can change the corresponding PR to give an error instead, but that is
>> not a solution for the Pinot implementation either. It would be great if
>> [CALCITE-3550] provides a solution for them.
>>
>> Mihai
>>
>> ________________________________
>> From: Julian Hyde <jhyde.apa...@gmail.com>
>> Sent: Thursday, February 22, 2024 4:16 PM
>> To: dev@calcite.apache.org <dev@calcite.apache.org>
>> Subject: Re: Remove a specific type coersion rule
>>
>> When I read CALCITE-6210 it wasn’t clear that you wanted Calcite to give a
>> validation error when someone tries to cast a VARCHAR to a VARBINARY. I
>> agree that that is the desirable behavior. (And I believe it is consistent
>> with the SQL standard.) Can you amend the Jira case so that it clearly the
>> goal?
>>
>> Reading the code, I see that
>> https://issues.apache.org/jira/browse/CALCITE-3550 was trying to provide
>> a way for people to change the coercion rules. Does that solution meet your
>> needs?
>>
>> Julian
>>
>>
>>
>>> On Feb 22, 2024, at 6:59 AM, Gonzalo Ortiz Jaureguizar <
>> golthir...@gmail.com> wrote:
>>>
>>> Hello there,
>>>
>>> In the context of https://issues.apache.org/jira/browse/CALCITE-6210,
>> the
>>> Apache Pinot team is thinking about forbidding casting from VARCHAR to
>>> VARBINARY.
>>>
>>> I've been trying to implement that, but I'm not sure if it is possible or
>>> not. Following the Javadoc of SqlTypeCoercionRule (which, btw, seems a
>> bit
>>> outdated) I've tried to create my own coercion rule as:
>>>
>>> ```
>>> private static SqlTypeCoercionRule createPinotCoercionRule() {
>>> // Initialize a Builder instance with the default mappings.
>>> Map<SqlTypeName, ImmutableSet<SqlTypeName>> pinotTypeMapping = new
>>> HashMap<>(
>>> SqlTypeCoercionRule.instance().getTypeMapping()
>>> );
>>> pinotTypeMapping.put(SqlTypeName.BINARY,
>>> ImmutableSet.of(SqlTypeName.VARBINARY));
>>> pinotTypeMapping.put(SqlTypeName.VARBINARY,
>>> ImmutableSet.of(SqlTypeName.BINARY));
>>>
>>> // Initialize a SqlTypeCoercionRules with the new builder mappings.
>>> return SqlTypeCoercionRule.instance(pinotTypeMapping);
>>> }
>>> ```
>>>
>>> Then I've tried to execute a query like: `select 1 from Table where
>>> varBinaryField = 'some text'` and even when that SqlTypeCoercionRule is
>>> used, the Validator turns that into `select 1 from Table where
>>> varBinaryField = cast('some text'` as VARBINARY)`, which should be
>> illegal.
>>> That expression is then simplified when transformed into a RelRoot and
>> then
>>> the error described in
>> https://issues.apache.org/jira/browse/CALCITE-6210
>>> is thrown. Same cast is added with other queries like `select 1 from
>> Table
>>> where OCTET_LENGTH('80c062bc98021f94f1404e9bda0f6b0202') > 0`.
>>>
>>> It seems that the reason why this CAST is being added is because
>>> AbstractTypeCoersion.commonTypeForBinaryComparison and
>>> AbstractTypeCoersion.implicitCast assume some coercions are always valid.
>>>
>>> The question then is: Is this working as expected? Should we assume that
>>> rules can be added to SqlTypeCoercionRule but they cannot be not removed?
>>> What are the alternatives we have if we want to be more restrictive than
>>> the castings explained in
>>>
>> https://docs.google.com/spreadsheets/d/1GhleX5h5W8-kJKh7NMJ4vtoE78pwfaZRJl88ULX_MgU
>>> ?
>>>
>>> It would be fine to me if I could add extra enforcement at validation
>> time.
>>> Specifically, if that enforcement could be added after Validator modified
>>> the AST so I can be sure it will catch any possible CAST. I can do that
>> in
>>> a RelOptRule, but it would be better to enforce the restriction in
>> SqlNode
>>> in order to be able to include in the error message the position in the
>>> original expression.
>>>
>>> Bests
>>>
>>> Gonzalo
>>
>>
