IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results - #13543
IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results#13543tkalkirill wants to merge 9 commits into
Conversation
|
First of all you need to follow the common process [1] i.e. make a PR from your own ignite mirror, not from origin = https://github.com/apache but from: https://github.com/tkalkirill/ignite |
|
@zstan Okay, subsequent tickets will do as described. |
| if (isA(fromType, Primitive.LONG)) | ||
| return Expressions.call(BuiltInMethod.INTERNAL_TO_TIMESTAMP.method, operand); | ||
| } | ||
| else if (targetType == byte[].class && fromType == ByteString.class) |
There was a problem hiding this comment.
The reverse branch in fromInternal is reachable through RexImpTable.defineReflective. I verified this with an operator backed by binaryLength(byte[]): with the conversion, a binary literal works; without it, generated code fails to compile because it passes ByteString to a method expecting byte[]. I suggest keeping this branch.
There was a problem hiding this comment.
sorry, but still miss it ( do we have a test for it ? suggest it plz ?
There was a problem hiding this comment.
Added a org.apache.ignite.internal.processors.query.calcite.integration.OperatorsExtensionIntegrationTest#testByteArrayFunctions that reproduces the issue if this is removed.
| assertNotSame(row, res); | ||
| assertEquals(1, res.length); | ||
| assertSame(val, res[0]); | ||
| assertSame(val, row[0]); |
| RelDataType type = tf.toSql(delegate.getType(typeFactory)); | ||
|
|
||
| // Prevent the validator from replacing OTHER with a structured type derived from a dynamic parameter value. | ||
| return type.getSqlTypeName() == SqlTypeName.OTHER ? new OtherType(type.isNullable()) : type; |
There was a problem hiding this comment.
optimization: if type.getSqlTypeName() == SqlTypeName.OTHER - above derived RelDataType type will be dropped
There was a problem hiding this comment.
We need to call toSql first: Java types such as LocalDateTime initially report OTHER. Moving the check earlier changes the parameter type to ANY and skips required implicit casts. I verified that passing a DATE literal to a LocalDateTime parameter then fails with ClassCastException in both UDF and UDTF. I suggest keeping the current implementation.
| if (isA(fromType, Primitive.LONG)) | ||
| return Expressions.call(BuiltInMethod.INTERNAL_TO_TIMESTAMP.method, operand); | ||
| } | ||
| else if (targetType == byte[].class && fromType == ByteString.class) |
There was a problem hiding this comment.
sorry, but still miss it ( do we have a test for it ? suggest it plz ?
| } | ||
|
|
||
| /** */ | ||
| static List<Expression> fromInternal(RexToLixTranslator translator, |
There was a problem hiding this comment.
Look now you have two identical code, differs only with "RexToLixTranslator translator" param
i mean:
ConverterUtils#fromInternal(java.lang.Class[], List)
and
ConverterUtils#fromInternal(RexToLixTranslator, java.lang.Class[], java.util.List)
seems you can just rewrite your code like :
private static Expression fromInternal(@Nullable Expression root, Expression operand, Type targetType) {
if (Types.isAssignableFrom(targetType, operand.getType())
|| Types.isAssignableFrom(targetType, Primitive.box(operand.getType())))
return operand;
if (!TypeUtils.isConvertableType(targetType))
return targetType == BigDecimal.class ? fromInternal(operand, operand.getType(), targetType) :
convert(operand, operand.getType(), targetType);
Primitive primitive = Primitive.of(targetType);
if (Primitive.is(operand.getType()))
operand = Expressions.box(operand);
Expression converted = Expressions.call(
TypeUtils.class,
"fromInternal",
root,//translator.getRoot(),
operand,
Expressions.constant(targetType)
);
return primitive == null
? Expressions.convert_(converted, targetType)
: Expressions.unbox(Expressions.convert_(converted, primitive.boxClass), primitive);
}
probably some assertions check are helpful, it`s just a prototype
I run this approach through all calcite tests and it`s ok
wdyt ? props: more readable code, less code base
There was a problem hiding this comment.
Thanks, I did it a bit differently, and the duplication went away.
2 participants
https://issues.apache.org/jira/browse/IGNITE-29031