You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "tkalkirill (via GitHub)" <gi...@apache.org> on 2026/09/02 10:49:12 UTC
[PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
tkalkirill opened a new pull request, #13543:
URL: https://github.com/apache/ignite/pull/13543
https://issues.apache.org/jira/browse/IGNITE-29031
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3934838090
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/TableFunctionScan.java:
##########
@@ -58,14 +65,19 @@ private Row convertToRow(Object rowContainer) {
throw new IgniteSQLException("Unable to process table function data: row type is neither Collection or Object[].");
Object[] rowArr = rowContainer.getClass() == Object[].class
- ? (Object[])rowContainer
+ ? ((Object[])rowContainer).clone()
: ((Collection<?>)rowContainer).toArray();
if (rowArr.length != rowType.getFieldCount()) {
throw new IgniteSQLException("Unable to process table function data: row length [" + rowArr.length
+ "] doesn't match defined columns number [" + rowType.getFieldCount() + "].");
}
+ for (int i = 0; i < rowArr.length; i++) {
+ if (!(rowType.getFieldList().get(i).getType() instanceof OtherType))
Review Comment:
Also near passed on master and failed in current branch, seems regression:
```
@Test
public void testCollectionColumnTableFunctions() {
client.getOrCreateCache(new
CacheConfiguration<>("collection-column-table-functions")
.setSqlSchema("PUBLIC")
.setSqlFunctionClasses(CollectionColumnFunctionsLibrary.class));
// ArrayList column -> ARRAY.
assertQuery("SELECT ID, CARDINALITY(VALS), VALS[1] FROM " +
"listColumn()")
.returns(1, 2, 10)
.returns(2, 3, 30)
.check();
assertQuery("SELECT t.ID, v.VAL FROM listColumn() t, " +
" UNNEST(t.VALS) v(VAL) ORDER BY t.ID, v.VAL")
.returns(1, 10)
.returns(1, 20)
.returns(2, 30)
.returns(2, 40)
.returns(2, 50)
.check();
// String[] column -> MULTISET.
assertQuery("SELECT ID, CARDINALITY(NAMES) FROM arrayColumn()")
.returns(1, 1)
.returns(2, 2)
.check();
assertQuery("SELECT t.ID, n.NAME FROM arrayColumn() t, " +
" UNNEST(t.NAMES) n(NAME) ORDER BY t.ID, n.NAME")
.returns(1, "a")
.returns(2, "b")
.returns(2, "c")
.check();
}
/** */
public static final class CollectionColumnFunctionsLibrary {
/** */
private CollectionColumnFunctionsLibrary() {
// No-op.
}
/** Returns rows with a {@link List}-typed column. */
@QuerySqlTableFunction(columnTypes = {int.class, ArrayList.class},
columnNames = {"ID", "VALS"})
public static Iterable<Collection<?>> listColumn() {
return Arrays.asList(
Arrays.asList(1, new ArrayList<>(Arrays.asList(10, 20))),
Arrays.asList(2, new ArrayList<>(Arrays.asList(30, 40,
50)))
);
}
/** Returns rows with an array-typed column. */
@QuerySqlTableFunction(columnTypes = {int.class, String[].class},
columnNames = {"ID", "NAMES"})
public static Iterable<Collection<?>> arrayColumn() {
return Arrays.asList(
Arrays.asList(1, Arrays.asList("a")),
Arrays.asList(2, Arrays.asList("b", "c"))
);
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3933960388
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/TableFunctionScan.java:
##########
@@ -58,14 +65,19 @@ private Row convertToRow(Object rowContainer) {
throw new IgniteSQLException("Unable to process table function data: row type is neither Collection or Object[].");
Object[] rowArr = rowContainer.getClass() == Object[].class
- ? (Object[])rowContainer
+ ? ((Object[])rowContainer).clone()
: ((Collection<?>)rowContainer).toArray();
if (rowArr.length != rowType.getFieldCount()) {
throw new IgniteSQLException("Unable to process table function data: row length [" + rowArr.length
+ "] doesn't match defined columns number [" + rowType.getFieldCount() + "].");
}
+ for (int i = 0; i < rowArr.length; i++) {
+ if (!(rowType.getFieldList().get(i).getType() instanceof OtherType))
+ rowArr[i] = TypeUtils.toInternal(ctx, rowArr[i]);
Review Comment:
failed:
```
assertQuery("SELECT EXTRACT(YEAR FROM D) FROM f()")
.resultSize(1)
.check();
@QuerySqlTableFunction(columnTypes =
{java.util.Date.class}, columnNames = {"D"})
public static Iterable<Object[]> f() { // kill
return Collections.singletonList(new Object[] {java.sql.Date.valueOf("2020-01-01")});
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3957875042
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/ConverterUtils.java:
##########
@@ -145,6 +150,66 @@ static List<Expression> fromInternal(Class<?>[] targetTypes,
return list;
}
+ /** */
+ static List<Expression> fromInternal(RexToLixTranslator translator,
Review Comment:
Look now you have two identical code, differs only with "RexToLixTranslator translator" param
i mean:
ConverterUtils#fromInternal(java.lang.Class<?>[], List<Expression>)
and
org.apache.ignite.internal.processors.query.calcite.exec.exp.ConverterUtils#fromInternal(RexToLixTranslator, java.lang.Class<?>[], java.util.List<Expression>)
seems you can them 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
Irun this approach through all calcite tests and it`s ok
wdyt ? cons: more readable code, less code base
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3925727187
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/UserDefinedFunctionsIntegrationTest.java:
##########
@@ -464,6 +466,43 @@ public void testBigDecimalFunctionArgument() {
assertQuery("SELECT udf.decimalToInt(5.3)").returns(5).check();
}
+ /** */
+ @Test
+ public void testBinaryFunctions() {
Review Comment:
Added quite a lot of tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3934357375
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/IgniteTableFunction.java:
##########
@@ -100,6 +106,11 @@ public static IgniteTableFunction create(Method method, Class<?>[] colTypes, Str
return Iterable.class;
}
+ /** {@inheritDoc} */
+ @Override public List<FunctionParameter> getParameters() {
Review Comment:
need to be moved into abstract class cause `IgniteScalarFunction#getParameters` do the same as i can see
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3988193352
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/ConverterUtils.java:
##########
@@ -230,6 +276,12 @@ public static Expression convert(Expression operand, Type fromType, Type toType)
if (toType == BigDecimal.class)
throw new AssertionError("For conversion to decimal, ConverterUtils#convertToDecimal method should be used instead.");
+ if (fromType == byte[].class && toType == ByteString.class)
+ return Expressions.call(BuiltInMethod.BYTE_ARRAY_TO_BYTE_STRING.method, operand);
+
+ if (fromType == ByteString.class && toType == byte[].class)
+ return Expressions.call(BuiltInMethod.BYTE_STRING_TO_BYTE_ARRAY.method, operand);
Review Comment:
these conversion still has no test coverage, isn\`t it ?
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/IgniteFunctionParameter.java:
##########
@@ -0,0 +1,75 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.ignite.internal.processors.query.calcite.exec.exp;
+
+import java.util.List;
+import org.apache.calcite.adapter.java.JavaTypeFactory;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.schema.FunctionParameter;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.ignite.internal.processors.query.calcite.type.OtherType;
+
+/**
+ * Reflective Java function parameter represented with a SQL type.
+ *
+ * <p>The SQL representation is required to validate user-defined function arguments and to convert literal arguments
+ * while deriving a table function row type.
+ */
+final class IgniteFunctionParameter implements FunctionParameter {
Review Comment:
seems all tests are passed if you completelly remote this class, am i right ?
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/ConverterUtils.java:
##########
@@ -123,28 +124,73 @@ else if (targetType == java.sql.Timestamp.class) {
/** */
static List<Expression> fromInternal(Class<?>[] targetTypes,
List<Expression> expressions) {
- final List<Expression> list = new ArrayList<>();
+ return fromInternal(null, targetTypes, expressions);
+ }
+
+ /** */
+ static List<Expression> fromInternal(@Nullable Expression root,
+ Class<?>[] targetTypes,
+ List<Expression> expressions
+ ) {
+ final List<Expression> list = new ArrayList<>(expressions.size());
+
if (targetTypes.length == expressions.size()) {
for (int i = 0; i < expressions.size(); i++)
- list.add(fromInternal(expressions.get(i), targetTypes[i]));
+ list.add(fromInternal(root, expressions.get(i), targetTypes[i]));
}
else {
int j = 0;
- for (int i = 0; i < expressions.size(); i++) {
- Class<?> type;
+
+ for (Expression expression : expressions) {
+ Class<?> targetType;
+
if (!targetTypes[j].isArray()) {
- type = targetTypes[j];
+ targetType = targetTypes[j];
j++;
}
else
- type = targetTypes[j].getComponentType();
+ targetType = targetTypes[j].getComponentType();
- list.add(fromInternal(expressions.get(i), type));
+ list.add(fromInternal(root, expression, targetType));
}
}
+
return list;
}
+ /** */
+ private static Expression fromInternal(@Nullable Expression root, Expression operand, Type targetType) {
+ // Preserve Calcite conversions when no execution context is available, including temporal conversions.
+ if (root == null)
+ return fromInternal(operand, operand.getType(), targetType);
+
+ // Let the Java method call box compatible primitives instead of generating a reference cast.
+ 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);
+
+ if (Primitive.is(operand.getType()))
+ operand = Expressions.box(operand);
+
+ Expression converted = Expressions.call(
+ TypeUtils.class,
+ "fromInternal",
+ root,
+ operand,
+ Expressions.constant(targetType)
+ );
+
+ Primitive primitive = Primitive.of(targetType);
+
+ return primitive == null
+ ? Expressions.convert_(converted, targetType)
+ : Expressions.unbox(Expressions.convert_(converted, primitive.boxClass), primitive);
Review Comment:
redundant ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3925721910
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/IgniteTableFunction.java:
##########
@@ -132,4 +145,43 @@ private static void raiseValidationError(Method method, String errPostfix) {
throw new IgniteSQLException("Unable to create table function for method '" + mtdSign + "'. " + errPostfix);
}
+
+ /** Returns function parameters represented as SQL types where required. */
+ private static List<FunctionParameter> sqlParameters(Method method, List<FunctionParameter> functionParameters) {
+ var res = new ArrayList<>(functionParameters);
+
+ for (int i = 0; i < method.getParameterTypes().length; i++) {
+ if (method.getParameterTypes()[i] == byte[].class)
+ res.set(i, sqlBinaryParameter(res.get(i)));
+ }
+
+ return Collections.unmodifiableList(res);
+ }
+
+ /** Prevents Calcite from evaluating binary literals while deriving a table function row type. */
+ private static FunctionParameter sqlBinaryParameter(FunctionParameter delegate) {
Review Comment:
When testing with temporal types, it turned out that this is currently necessary and does not work without it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3967284932
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/ConverterUtils.java:
##########
@@ -145,6 +150,66 @@ static List<Expression> fromInternal(Class<?>[] targetTypes,
return list;
}
+ /** */
+ static List<Expression> fromInternal(RexToLixTranslator translator,
Review Comment:
Thanks, I did it a bit differently, and the duplication went away.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3933573891
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/ReflectiveCallNotNullImplementor.java:
##########
@@ -66,6 +68,17 @@ public ReflectiveCallNotNullImplementor(Method method) {
callExpr = Expressions.call(target, method, translatedOperands);
}
+
+ if (TypeUtils.isConvertableType(method.getReturnType())) {
Review Comment:
```
@QuerySqlFunction
public static java.util.Date kill1() {
return java.sql.Date.valueOf("2020-01-01");
}
```
and further call:
```
assertQuery("SELECT kill1()")
.resultSize(1)
.check();
```
will raise ClassCastException, while
```
@QuerySqlFunction
public static java.sql.Date kill2() {
return java.sql.Date.valueOf("2020-01-01");
}
```
passes, wdyt ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill merged PR #13543:
URL: https://github.com/apache/ignite/pull/13543
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3924678307
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/IgniteTableFunction.java:
##########
@@ -132,4 +145,43 @@ private static void raiseValidationError(Method method, String errPostfix) {
throw new IgniteSQLException("Unable to create table function for method '" + mtdSign + "'. " + errPostfix);
}
+
+ /** Returns function parameters represented as SQL types where required. */
+ private static List<FunctionParameter> sqlParameters(Method method, List<FunctionParameter> functionParameters) {
+ var res = new ArrayList<>(functionParameters);
+
+ for (int i = 0; i < method.getParameterTypes().length; i++) {
+ if (method.getParameterTypes()[i] == byte[].class)
+ res.set(i, sqlBinaryParameter(res.get(i)));
+ }
+
+ return Collections.unmodifiableList(res);
+ }
+
+ /** Prevents Calcite from evaluating binary literals while deriving a table function row type. */
+ private static FunctionParameter sqlBinaryParameter(FunctionParameter delegate) {
Review Comment:
I prefer to revise this approach, if it\`s hard to implement now - let\`s fill an additional issue, thanks !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on PR #13543:
URL: https://github.com/apache/ignite/pull/13543#issuecomment-5521283717
First of all you need to follow the common process [1] i.e. make a PR from your own ignite mirrow, not from origin = https://github.com/apache but from: https://github.com/tkalkirill/ignite
[1] https://cwiki.apache.org/confluence/spaces/IGNITE/pages/177047163/How+to+Contribute#HowtoContribute-GITworkflow
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3924006973
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/TableFunctionScan.java:
##########
@@ -66,6 +68,22 @@ private Row convertToRow(Object rowContainer) {
+ "] doesn't match defined columns number [" + rowType.getFieldCount() + "].");
}
- return rowFactory.create(rowArr);
+ return rowFactory.create(convertBinaryColumns(rowArr));
+ }
+
+ /** Converts binary column values to the internal representation. */
+ private Object[] convertBinaryColumns(Object[] row) {
+ Object[] convertedRow = row;
+
+ for (int i = 0; i < row.length; i++) {
+ if (row[i] instanceof byte[] && SqlTypeUtil.isBinary(rowType.getFieldList().get(i).getType())) {
Review Comment:
Tried to fix them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3934303872
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/ConverterUtils.java:
##########
@@ -111,6 +116,9 @@ else if (targetType == java.sql.Timestamp.class) {
if (isA(fromType, Primitive.LONG))
return Expressions.call(BuiltInMethod.INTERNAL_TO_TIMESTAMP.method, operand);
}
+ else if (targetType == byte[].class && fromType == ByteString.class)
Review Comment:
dead code
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/ConverterUtils.java:
##########
@@ -74,6 +76,9 @@ else if (fromType == java.sql.Timestamp.class) {
else if (targetType == Long.class)
return Expressions.call(BuiltInMethod.TIMESTAMP_TO_LONG_OPTIONAL.method, operand);
}
+ else if (fromType == byte[].class && targetType == ByteString.class)
Review Comment:
dead code
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/IgniteTableFunction.java:
##########
@@ -100,6 +106,11 @@ public static IgniteTableFunction create(Method method, Class<?>[] colTypes, Str
return Iterable.class;
}
+ /** {@inheritDoc} */
+ @Override public List<FunctionParameter> getParameters() {
Review Comment:
need to be moved into abstracn cause `IgniteScalarFunction#getParameters` do the same as i can see
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3957875042
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/ConverterUtils.java:
##########
@@ -145,6 +150,66 @@ static List<Expression> fromInternal(Class<?>[] targetTypes,
return list;
}
+ /** */
+ static List<Expression> fromInternal(RexToLixTranslator translator,
Review Comment:
Look now you have two identical code, differs only with "RexToLixTranslator translator" param
i mean:
ConverterUtils#fromInternal(java.lang.Class<?>[], List<Expression>)
and
ConverterUtils#fromInternal(RexToLixTranslator, java.lang.Class<?>[], java.util.List<Expression>)
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
Irun this approach through all calcite tests and it`s ok
wdyt ? cons: more readable code, less code base
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3924198956
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/IgniteTableFunction.java:
##########
@@ -132,4 +145,43 @@ private static void raiseValidationError(Method method, String errPostfix) {
throw new IgniteSQLException("Unable to create table function for method '" + mtdSign + "'. " + errPostfix);
}
+
+ /** Returns function parameters represented as SQL types where required. */
+ private static List<FunctionParameter> sqlParameters(Method method, List<FunctionParameter> functionParameters) {
+ var res = new ArrayList<>(functionParameters);
+
+ for (int i = 0; i < method.getParameterTypes().length; i++) {
+ if (method.getParameterTypes()[i] == byte[].class)
+ res.set(i, sqlBinaryParameter(res.get(i)));
+ }
+
+ return Collections.unmodifiableList(res);
+ }
+
+ /** Prevents Calcite from evaluating binary literals while deriving a table function row type. */
+ private static FunctionParameter sqlBinaryParameter(FunctionParameter delegate) {
Review Comment:
This change is only needed when a `byte[]` table-function argument is a binary literal, for example `binaryTableLength(x'010203')`. Calcite tries to materialize the literal as byte[] during row-type inference, before the runtime converter is involved. Dynamic parameters and other non-literal expressions work without this workaround.
Would you prefer that we drop binary-literal support from this fix and address it separately if needed, or keep the workaround with an explanatory comment and a dedicated test?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on PR #13543:
URL: https://github.com/apache/ignite/pull/13543#issuecomment-5521356091
@zstan Okay, subsequent tickets will do as described.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3967003286
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/IgniteFunctionParameter.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.ignite.internal.processors.query.calcite.exec.exp;
+
+import java.util.List;
+import org.apache.calcite.adapter.java.JavaTypeFactory;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.schema.FunctionParameter;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.ignite.internal.processors.query.calcite.type.OtherType;
+
+/**
+ * Reflective Java function parameter represented with a SQL type.
+ *
+ * <p>The SQL representation is required to validate user-defined function arguments and to convert literal arguments
+ * while deriving a table function row type.
+ */
+final class IgniteFunctionParameter implements FunctionParameter {
+ /** Original function parameter. */
+ private final FunctionParameter delegate;
+
+ /**
+ * Constructor.
+ *
+ * @param delegate Original function parameter.
+ */
+ private IgniteFunctionParameter(FunctionParameter delegate) {
+ this.delegate = delegate;
+ }
+
+ /** Returns function parameters represented with SQL types. */
+ static List<FunctionParameter> toSql(List<FunctionParameter> parameters) {
+ return parameters.stream().map(IgniteFunctionParameter::toSql).toList();
+ }
+
+ /** Returns a function parameter represented with a SQL type. */
+ static FunctionParameter toSql(FunctionParameter parameter) {
+ return new IgniteFunctionParameter(parameter);
+ }
+
+ /** {@inheritDoc} */
+ @Override public int getOrdinal() {
+ return delegate.getOrdinal();
+ }
+
+ /** {@inheritDoc} */
+ @Override public String getName() {
+ return delegate.getName();
+ }
+
+ /** {@inheritDoc} */
+ @Override public RelDataType getType(RelDataTypeFactory typeFactory) {
+ JavaTypeFactory tf = (JavaTypeFactory)typeFactory;
+ 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;
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3950377224
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/TableFunctionScan.java:
##########
@@ -58,14 +65,19 @@ private Row convertToRow(Object rowContainer) {
throw new IgniteSQLException("Unable to process table function data: row type is neither Collection or Object[].");
Object[] rowArr = rowContainer.getClass() == Object[].class
- ? (Object[])rowContainer
+ ? ((Object[])rowContainer).clone()
: ((Collection<?>)rowContainer).toArray();
if (rowArr.length != rowType.getFieldCount()) {
throw new IgniteSQLException("Unable to process table function data: row length [" + rowArr.length
+ "] doesn't match defined columns number [" + rowType.getFieldCount() + "].");
}
+ for (int i = 0; i < rowArr.length; i++) {
+ if (!(rowType.getFieldList().get(i).getType() instanceof OtherType))
Review Comment:
I tried it, now it works.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3921817226
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/UserDefinedFunctionsIntegrationTest.java:
##########
@@ -464,6 +466,43 @@ public void testBigDecimalFunctionArgument() {
assertQuery("SELECT udf.decimalToInt(5.3)").returns(5).check();
}
+ /** */
+ @Test
+ public void testBinaryFunctions() {
Review Comment:
I disagree that this PR need to cover only byte[], what about temporal types ? BO, appropriate tests need to be appended.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3924125501
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/util/TypeUtils.java:
##########
@@ -309,7 +309,7 @@ public static <Row> Function<Row, Row> resultTypeConverter(ExecutionContext<Row>
/** */
private static Function<Object, Object> fieldConverter(ExecutionContext<?> ectx, RelDataType fieldType) {
- Type storageType = ectx.getTypeFactory().getJavaClass(fieldType);
+ Type storageType = SqlTypeUtil.isBinary(fieldType) ? byte[].class : ectx.getTypeFactory().getJavaClass(fieldType);
Review Comment:
This was needed because the previous implementation was only for a `byte[]`, now it is not needed, I got rid of it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3957875042
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/ConverterUtils.java:
##########
@@ -145,6 +150,66 @@ static List<Expression> fromInternal(Class<?>[] targetTypes,
return list;
}
+ /** */
+ static List<Expression> fromInternal(RexToLixTranslator translator,
Review Comment:
Look now you have two identical code, differs only with "RexToLixTranslator translator" param
i mean:
ConverterUtils#fromInternal(java.lang.Class<?>[], List<Expression>)
and
ConverterUtils#fromInternal(RexToLixTranslator, java.lang.Class<?>[], java.util.List<Expression>)
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3958611215
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/ExecutionTest.java:
##########
@@ -63,6 +79,51 @@ public class ExecutionTest extends AbstractExecutionTest {
super.setup();
}
+ /** */
+ @Test
+ public void testTableFunctionOtherValues() {
+ checkTableFunctionOtherValues(new BasicSqlType(IgniteTypeSystem.INSTANCE, SqlTypeName.OTHER));
+ checkTableFunctionOtherValues(new OtherType(true));
+ }
+
+ /** */
+ private void checkTableFunctionOtherValues(RelDataType colType) {
+ ExecutionContext<Object[]> ctx = executionContext();
+
+ // Bypass type canonization to test both representations of OTHER regardless of the shared type cache state.
+ RelDataType rowType = new RelRecordType(Collections.singletonList(new RelDataTypeFieldImpl("V", 0, colType)));
+
+ Object[] vals = {
+ new byte[] {1, 2, 3},
+ new java.util.Date(0),
+ Date.valueOf("2020-01-01"),
+ Time.valueOf("02:03:04"),
+ Timestamp.valueOf("2020-01-01 02:03:04"),
+ LocalDate.of(2020, 1, 1),
+ LocalTime.of(2, 3, 4),
+ LocalDateTime.of(2020, 1, 1, 2, 3, 4),
+ Duration.ofHours(2),
+ Period.ofMonths(3),
+ null
+ };
+
+ for (Object val : vals) {
+ Object[] row = {val};
+
+ for (Object container : new Object[] {row, Collections.singletonList(val)}) {
+ TableFunctionScan<Object[]> scan = new TableFunctionScan<>(ctx, rowType,
+ () -> Collections.singletonList(container), ctx.rowHandler().factory(ctx.getTypeFactory(), rowType));
+
+ Object[] res = scan.iterator().next();
+
+ assertNotSame(row, res);
+ assertEquals(1, res.length);
+ assertSame(val, res[0]);
+ assertSame(val, row[0]);
Review Comment:
Why?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3949760363
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/ConverterUtils.java:
##########
@@ -111,6 +116,9 @@ else if (targetType == java.sql.Timestamp.class) {
if (isA(fromType, Primitive.LONG))
return Expressions.call(BuiltInMethod.INTERNAL_TO_TIMESTAMP.method, operand);
}
+ else if (targetType == byte[].class && fromType == ByteString.class)
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3949228558
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/ReflectiveCallNotNullImplementor.java:
##########
@@ -66,6 +68,17 @@ public ReflectiveCallNotNullImplementor(Method method) {
callExpr = Expressions.call(target, method, translatedOperands);
}
+
+ if (TypeUtils.isConvertableType(method.getReturnType())) {
Review Comment:
Try to fix it.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/TableFunctionScan.java:
##########
@@ -58,14 +65,19 @@ private Row convertToRow(Object rowContainer) {
throw new IgniteSQLException("Unable to process table function data: row type is neither Collection or Object[].");
Object[] rowArr = rowContainer.getClass() == Object[].class
- ? (Object[])rowContainer
+ ? ((Object[])rowContainer).clone()
: ((Collection<?>)rowContainer).toArray();
if (rowArr.length != rowType.getFieldCount()) {
throw new IgniteSQLException("Unable to process table function data: row length [" + rowArr.length
+ "] doesn't match defined columns number [" + rowType.getFieldCount() + "].");
}
+ for (int i = 0; i < rowArr.length; i++) {
+ if (!(rowType.getFieldList().get(i).getType() instanceof OtherType))
+ rowArr[i] = TypeUtils.toInternal(ctx, rowArr[i]);
Review Comment:
Try to fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3949638385
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/util/TypeUtils.java:
##########
@@ -94,6 +94,8 @@ public class TypeUtils {
LocalTime.class,
Duration.class,
Period.class,
+ char.class,
+ Character.class,
Review Comment:
You're right, I went too far with `char/Character` support. I've removed the related conversions and tests from this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3958559153
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/ConverterUtils.java:
##########
@@ -111,6 +116,9 @@ else if (targetType == java.sql.Timestamp.class) {
if (isA(fromType, Primitive.LONG))
return Expressions.call(BuiltInMethod.INTERNAL_TO_TIMESTAMP.method, operand);
}
+ else if (targetType == byte[].class && fromType == ByteString.class)
Review Comment:
Added a `org.apache.ignite.internal.processors.query.calcite.integration.OperatorsExtensionIntegrationTest#testByteArrayFunctions` that reproduces the issue if this is removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3921681002
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/TableFunctionScan.java:
##########
@@ -66,6 +68,22 @@ private Row convertToRow(Object rowContainer) {
+ "] doesn't match defined columns number [" + rowType.getFieldCount() + "].");
}
- return rowFactory.create(rowArr);
+ return rowFactory.create(convertBinaryColumns(rowArr));
+ }
+
+ /** Converts binary column values to the internal representation. */
+ private Object[] convertBinaryColumns(Object[] row) {
+ Object[] convertedRow = row;
+
+ for (int i = 0; i < row.length; i++) {
+ if (row[i] instanceof byte[] && SqlTypeUtil.isBinary(rowType.getFieldList().get(i).getType())) {
Review Comment:
1. we already have common converter : TypeUtils#toInternal - it need to be used
2. explain why do you need row.clone() ? .toArray() - already produced a new array.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/util/TypeUtils.java:
##########
@@ -309,7 +309,7 @@ public static <Row> Function<Row, Row> resultTypeConverter(ExecutionContext<Row>
/** */
private static Function<Object, Object> fieldConverter(ExecutionContext<?> ectx, RelDataType fieldType) {
- Type storageType = ectx.getTypeFactory().getJavaClass(fieldType);
+ Type storageType = SqlTypeUtil.isBinary(fieldType) ? byte[].class : ectx.getTypeFactory().getJavaClass(fieldType);
Review Comment:
plz explain why do we need changes in this class ? All changes need to be covered by tests, i miss them.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/IgniteTableFunction.java:
##########
@@ -132,4 +145,43 @@ private static void raiseValidationError(Method method, String errPostfix) {
throw new IgniteSQLException("Unable to create table function for method '" + mtdSign + "'. " + errPostfix);
}
+
+ /** Returns function parameters represented as SQL types where required. */
+ private static List<FunctionParameter> sqlParameters(Method method, List<FunctionParameter> functionParameters) {
+ var res = new ArrayList<>(functionParameters);
+
+ for (int i = 0; i < method.getParameterTypes().length; i++) {
+ if (method.getParameterTypes()[i] == byte[].class)
+ res.set(i, sqlBinaryParameter(res.get(i)));
+ }
+
+ return Collections.unmodifiableList(res);
+ }
+
+ /** Prevents Calcite from evaluating binary literals while deriving a table function row type. */
+ private static FunctionParameter sqlBinaryParameter(FunctionParameter delegate) {
Review Comment:
looks like a hack as for me
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3921874874
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/IgniteTableFunction.java:
##########
@@ -132,4 +145,43 @@ private static void raiseValidationError(Method method, String errPostfix) {
throw new IgniteSQLException("Unable to create table function for method '" + mtdSign + "'. " + errPostfix);
}
+
+ /** Returns function parameters represented as SQL types where required. */
+ private static List<FunctionParameter> sqlParameters(Method method, List<FunctionParameter> functionParameters) {
+ var res = new ArrayList<>(functionParameters);
+
+ for (int i = 0; i < method.getParameterTypes().length; i++) {
Review Comment:
I\`d prefer not to relay on implicit size alignment
```suggestion
for (int i = 0; i < functionParameters.size(); i++) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r4153740965
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/ConverterUtils.java:
##########
@@ -230,6 +276,12 @@ public static Expression convert(Expression operand, Type fromType, Type toType)
if (toType == BigDecimal.class)
throw new AssertionError("For conversion to decimal, ConverterUtils#convertToDecimal method should be used instead.");
+ if (fromType == byte[].class && toType == ByteString.class)
+ return Expressions.call(BuiltInMethod.BYTE_ARRAY_TO_BYTE_STRING.method, operand);
+
+ if (fromType == ByteString.class && toType == byte[].class)
+ return Expressions.call(BuiltInMethod.BYTE_STRING_TO_BYTE_ARRAY.method, operand);
Review Comment:
Add OperatorsExtensionIntegrationTest#testBinaryFunctionWithJavaReturnType and OperatorsExtensionIntegrationTest#testBinaryFunctionWithJavaReturnTypeDynamicParameters
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3933573891
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/ReflectiveCallNotNullImplementor.java:
##########
@@ -66,6 +68,17 @@ public ReflectiveCallNotNullImplementor(Method method) {
callExpr = Expressions.call(target, method, translatedOperands);
}
+
+ if (TypeUtils.isConvertableType(method.getReturnType())) {
Review Comment:
```
@QuerySqlFunction
public static java.util.Date kill() {
return java.sql.Date.valueOf("2020-01-01");
}
```
and further call:
```
assertQuery("SELECT kill1()")
.resultSize(1)
.check();
```
will raise ClassCastException, while
```
@QuerySqlFunction
public static java.sql.Date kill2() {
return java.sql.Date.valueOf("2020-01-01");
}
```
passes, wdyt ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3934041232
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/util/TypeUtils.java:
##########
@@ -94,6 +94,8 @@ public class TypeUtils {
LocalTime.class,
Duration.class,
Period.class,
+ char.class,
+ Character.class,
Review Comment:
UDF like : static boolean f(char c)
and further `SELECT f('')` ->
literal CHAR(0) passes SqlTypeAssignmentRule (CHAR assignable from CHAR,
precision ignored) so no CAST is added;
TypeUtils.fromInternal(root, \"\", char.class) ->
StringIndexOutOfBoundsException
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3956024252
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/ExecutionTest.java:
##########
@@ -63,6 +79,51 @@ public class ExecutionTest extends AbstractExecutionTest {
super.setup();
}
+ /** */
+ @Test
+ public void testTableFunctionOtherValues() {
+ checkTableFunctionOtherValues(new BasicSqlType(IgniteTypeSystem.INSTANCE, SqlTypeName.OTHER));
+ checkTableFunctionOtherValues(new OtherType(true));
+ }
+
+ /** */
+ private void checkTableFunctionOtherValues(RelDataType colType) {
+ ExecutionContext<Object[]> ctx = executionContext();
+
+ // Bypass type canonization to test both representations of OTHER regardless of the shared type cache state.
+ RelDataType rowType = new RelRecordType(Collections.singletonList(new RelDataTypeFieldImpl("V", 0, colType)));
+
+ Object[] vals = {
+ new byte[] {1, 2, 3},
+ new java.util.Date(0),
+ Date.valueOf("2020-01-01"),
+ Time.valueOf("02:03:04"),
+ Timestamp.valueOf("2020-01-01 02:03:04"),
+ LocalDate.of(2020, 1, 1),
+ LocalTime.of(2, 3, 4),
+ LocalDateTime.of(2020, 1, 1, 2, 3, 4),
+ Duration.ofHours(2),
+ Period.ofMonths(3),
+ null
+ };
+
+ for (Object val : vals) {
+ Object[] row = {val};
+
+ for (Object container : new Object[] {row, Collections.singletonList(val)}) {
+ TableFunctionScan<Object[]> scan = new TableFunctionScan<>(ctx, rowType,
+ () -> Collections.singletonList(container), ctx.rowHandler().factory(ctx.getTypeFactory(), rowType));
+
+ Object[] res = scan.iterator().next();
+
+ assertNotSame(row, res);
+ assertEquals(1, res.length);
+ assertSame(val, res[0]);
+ assertSame(val, row[0]);
Review Comment:
redundant
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/IgniteFunctionParameter.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.ignite.internal.processors.query.calcite.exec.exp;
+
+import java.util.List;
+import org.apache.calcite.adapter.java.JavaTypeFactory;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.schema.FunctionParameter;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.ignite.internal.processors.query.calcite.type.OtherType;
+
+/**
+ * Reflective Java function parameter represented with a SQL type.
+ *
+ * <p>The SQL representation is required to validate user-defined function arguments and to convert literal arguments
+ * while deriving a table function row type.
+ */
+final class IgniteFunctionParameter implements FunctionParameter {
+ /** Original function parameter. */
+ private final FunctionParameter delegate;
+
+ /**
+ * Constructor.
+ *
+ * @param delegate Original function parameter.
+ */
+ private IgniteFunctionParameter(FunctionParameter delegate) {
+ this.delegate = delegate;
+ }
+
+ /** Returns function parameters represented with SQL types. */
+ static List<FunctionParameter> toSql(List<FunctionParameter> parameters) {
+ return parameters.stream().map(IgniteFunctionParameter::toSql).toList();
+ }
+
+ /** Returns a function parameter represented with a SQL type. */
+ static FunctionParameter toSql(FunctionParameter parameter) {
+ return new IgniteFunctionParameter(parameter);
+ }
+
+ /** {@inheritDoc} */
+ @Override public int getOrdinal() {
+ return delegate.getOrdinal();
+ }
+
+ /** {@inheritDoc} */
+ @Override public String getName() {
+ return delegate.getName();
+ }
+
+ /** {@inheritDoc} */
+ @Override public RelDataType getType(RelDataTypeFactory typeFactory) {
+ JavaTypeFactory tf = (JavaTypeFactory)typeFactory;
+ 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;
Review Comment:
optimization: if type.getSqlTypeName() == SqlTypeName.OTHER - above derived `RelDataType type` will be dropped
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/LogicalRelImplementor.java:
##########
@@ -788,7 +788,7 @@ else if (rel instanceof Intersect)
RowFactory<Row> rowFactory = ctx.rowHandler().factory(ctx.getTypeFactory(), rowType);
- return new ScanNode<>(ctx, rowType, new TableFunctionScan<>(rowType, dataSupplier, rowFactory));
+ return new ScanNode<>(ctx, rowType, new TableFunctionScan<>(ctx, rowType, dataSupplier, rowFactory));
Review Comment:
rowFactory = ctx.rowHandler().factory(ctx.getTypeFactory(), rowType); - gathered from `ctx` and `rowType` that already passed, so - no need to pass `rowFactory` as param? Now - it can be derived inside ScanNode constructor.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/ConverterUtils.java:
##########
@@ -111,6 +116,9 @@ else if (targetType == java.sql.Timestamp.class) {
if (isA(fromType, Primitive.LONG))
return Expressions.call(BuiltInMethod.INTERNAL_TO_TIMESTAMP.method, operand);
}
+ else if (targetType == byte[].class && fromType == ByteString.class)
Review Comment:
sorry, but still miss it ( do we have a test for it ? suggest it plz ?
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/ConverterUtils.java:
##########
@@ -145,6 +150,66 @@ static List<Expression> fromInternal(Class<?>[] targetTypes,
return list;
}
+ /** */
+ static List<Expression> fromInternal(RexToLixTranslator translator,
+ Class<?>[] targetTypes,
+ List<Expression> expressions
+ ) {
+ final List<Expression> list = new ArrayList<>();
Review Comment:
```suggestion
final List<Expression> list = new ArrayList<>(expressions.size());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3957875042
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/ConverterUtils.java:
##########
@@ -145,6 +150,66 @@ static List<Expression> fromInternal(Class<?>[] targetTypes,
return list;
}
+ /** */
+ static List<Expression> fromInternal(RexToLixTranslator translator,
Review Comment:
Look now you have two identical code, differs only with "RexToLixTranslator translator" param
i mean:
ConverterUtils#fromInternal(java.lang.Class<?>[], List<Expression>)
and
ConverterUtils#fromInternal(RexToLixTranslator, java.lang.Class<?>[], java.util.List<Expression>)
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 ? cons: more readable code, less code base
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-29031 SQL Calcite: Support byte[] in UDF and UDTF parameters and results [ignite]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #13543:
URL: https://github.com/apache/ignite/pull/13543#discussion_r3957875042
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/ConverterUtils.java:
##########
@@ -145,6 +150,66 @@ static List<Expression> fromInternal(Class<?>[] targetTypes,
return list;
}
+ /** */
+ static List<Expression> fromInternal(RexToLixTranslator translator,
Review Comment:
Look now you have two identical code, differs only with "RexToLixTranslator translator" param
i mean:
ConverterUtils#fromInternal(java.lang.Class<?>[], List<Expression>)
and
ConverterUtils#fromInternal(RexToLixTranslator, java.lang.Class<?>[], java.util.List<Expression>)
seems you can them 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
Irun this approach through all calcite tests and it`s ok
wdyt ? cons: more readable code, less code base
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org