You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@calcite.apache.org by Gonzalo Ortiz Jaureguizar <go...@gmail.com> on 2024/02/22 14:59:26 UTC
Remove a specific type coersion rule
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
Re: Remove a specific type coersion rule
Posted by Julian Hyde <jh...@gmail.com>.
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 <mb...@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 <go...@gmail.com>
> Sent: Thursday, February 22, 2024 11:53 PM
> To: dev@calcite.apache.org <de...@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 (<mb...@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 <jh...@gmail.com>
>> Sent: Thursday, February 22, 2024 4:16 PM
>> To: dev@calcite.apache.org <de...@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 <
>> golthiryus@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
>>
>>
Re: Remove a specific type coersion rule
Posted by Mihai Budiu <mb...@gmail.com>.
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 <go...@gmail.com>
Sent: Thursday, February 22, 2024 11:53 PM
To: dev@calcite.apache.org <de...@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 (<mb...@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 <jh...@gmail.com>
> Sent: Thursday, February 22, 2024 4:16 PM
> To: dev@calcite.apache.org <de...@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 <
> golthiryus@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
>
>
Re: Remove a specific type coersion rule
Posted by Gonzalo Ortiz Jaureguizar <go...@gmail.com>.
> 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 (<mb...@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 <jh...@gmail.com>
> Sent: Thursday, February 22, 2024 4:16 PM
> To: dev@calcite.apache.org <de...@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 <
> golthiryus@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
>
>
Re: Remove a specific type coersion rule
Posted by Mihai Budiu <mb...@gmail.com>.
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 <jh...@gmail.com>
Sent: Thursday, February 22, 2024 4:16 PM
To: dev@calcite.apache.org <de...@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 <go...@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
Re: Remove a specific type coersion rule
Posted by Julian Hyde <jh...@gmail.com>.
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 <go...@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