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 2 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. |
| Object[] convertedRow = row; | ||
|
|
||
| for (int i = 0; i < row.length; i++) { | ||
| if (row[i] instanceof byte[] && SqlTypeUtil.isBinary(rowType.getFieldList().get(i).getType())) { |
There was a problem hiding this comment.
- we already have common converter : TypeUtils#toInternal - it need to be used
- explain why do you need row.clone() ? .toArray() - already produced a new array.
There was a problem hiding this comment.
Tried to fix them.
| /** */ | ||
| 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); |
There was a problem hiding this comment.
plz explain why do we need changes in this class ? All changes need to be covered by tests, i miss them.
There was a problem hiding this comment.
This was needed because the previous implementation was only for a byte[], now it is not needed, I got rid of it.
| } | ||
|
|
||
| /** Prevents Calcite from evaluating binary literals while deriving a table function row type. */ | ||
| private static FunctionParameter sqlBinaryParameter(FunctionParameter delegate) { |
There was a problem hiding this 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?
There was a problem hiding this comment.
I prefer to revise this approach, if it`s hard to implement now - let`s fill an additional issue, thanks !
There was a problem hiding this comment.
When testing with temporal types, it turned out that this is currently necessary and does not work without it.
|
|
||
| /** */ | ||
| @Test | ||
| public void testBinaryFunctions() { |
There was a problem hiding this comment.
I disagree that this PR need to cover only byte[], what about temporal types ? BO, appropriate tests need to be appended.
There was a problem hiding this comment.
Added quite a lot of tests.
2 participants
https://issues.apache.org/jira/browse/IGNITE-29031