IGNITE-23008 SQL Calcite: Fix binary type dynamic parameter type coercion #11606
Conversation
| @@ -315,7 +316,7 @@ public RelDataType createCustomType(Type type) { | |||
| public RelDataType createCustomType(Type type, boolean nullable) { | |||
| if (UUID.class == type) | |||
| return canonize(new UuidType(nullable)); | |||
| else if (Object.class == type) | |||
| else if (Object.class == type || (type instanceof Class && BinaryObject.class.isAssignableFrom((Class<?>)type))) | |||
There was a problem hiding this comment.
type instanceof Class looks useless
There was a problem hiding this comment.
Why? In case of not Class type we can get an exception on cast to Class, so it's safer to check type.
There was a problem hiding this comment.
I think we can't get any problem here. Calcite expects a class in createType(Type type):
if (!(type instanceof Class)) { throw new UnsupportedOperationException("TODO: implement " + type); } final Class clazz = (Class) type;
An assert looks enough to me. Up to you,
There was a problem hiding this comment.
In createType we call super.createType if custom type is null. So type other than Class can be processed by superclass.
| @@ -315,7 +316,7 @@ public RelDataType createCustomType(Type type) { | |||
| public RelDataType createCustomType(Type type, boolean nullable) { | |||
| if (UUID.class == type) | |||
| return canonize(new UuidType(nullable)); | |||
| else if (Object.class == type) | |||
| else if (Object.class == type || (type instanceof Class && BinaryObject.class.isAssignableFrom((Class<?>)type))) | |||
| return canonize(new OtherType(nullable)); | |||
There was a problem hiding this comment.
Actually, canonize() is not working for IgniteCustomType. Does nothing
There was a problem hiding this comment.
It canonized by DATATYPE_CACHE.intern and always return the same instance for OtherType. Why do you think it does nothing?
There was a problem hiding this comment.
This is the reason. It always return the same value.
|
|
||
| setValidatedNodeType(node, (param == null) ? typeFactory().createSqlType(SqlTypeName.NULL) : | ||
| typeFactory().toSql(typeFactory().createType(param.getClass()))); | ||
| setValidatedNodeType(node, (param == null) ? typeFactory().createSqlType(SqlTypeName.NULL) : |
There was a problem hiding this comment.
typeFactory().createSqlType(SqlTypeName.NULL) might be cached. The brackets (param == null) can be removed too.
There was a problem hiding this comment.
typeFactory().createSqlType(SqlTypeName.NULL) used only once. Where it might be cached?
There was a problem hiding this comment.
In IgniteSqlValidator for example. No need to create type every time for every null param.
There was a problem hiding this comment.
Planning is a resource consuming process, types caching during planning does not give any performance boost. I don't know why, but types are not cached in Calcite anywhere. On the other hand we cache it during execution (see ExpressionFactoryImpl#nullType), since it's more critical here.
alex-plekhanov
force-pushed
the
ignite-23008
branch
from
e37fada
to
19de95e
Compare
|
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
The Contribution Checklist
The description explains WHAT and WHY was made instead of HOW.
The following pattern must be used:
IGNITE-XXXX Change summarywhereXXXX- number of JIRA issue.(see the Maintainers list)
the
green visaattached to the JIRA ticket (see TC.Bot: Check PR)Notes
If you need any help, please email [email protected] or ask anу advice on http://asf.slack.com #ignite channel.