> 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
>
>
