twalthr commented on a change in pull request #17450: URL: https://github.com/apache/flink/pull/17450#discussion_r727097560
########## File path: flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/logical/utils/LogicalTypeCasts.java ########## @@ -319,7 +322,8 @@ private static boolean supportsCasting( } else if (sourceRoot == STRUCTURED_TYPE || targetRoot == STRUCTURED_TYPE) { return supportsStructuredCasting( sourceType, targetType, (s, t) -> supportsCasting(s, t, allowExplicit)); - } else if (sourceRoot == RAW || targetRoot == RAW) { + } else if ((sourceRoot == RAW && !targetRoot.getFamilies().contains(BINARY_STRING)) Review comment: very nit: We use `hasFamily` for better code readability. Can we rewrite this to `(sourceRoot == RAW && !hasFamily(targetType, BINARY_STRING)`? ########## File path: flink-table/flink-table-common/src/test/java/org/apache/flink/table/types/LogicalTypeCastsTest.java ########## @@ -253,7 +256,15 @@ .build(), false, true - } + }, + + // raw to binary + { + new RawType(Integer.class, new ValueSerializer(Integer.class)), Review comment: nit: use `IntSeriaizer.INSTANCE`? should be more stable once we remove ValueSerializer ########## File path: flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/logical/utils/LogicalTypeCasts.java ########## @@ -401,6 +405,8 @@ private static boolean supportsConstructedCasting( } } return true; + } else if (sourceRoot == ARRAY && (targetRoot == CHAR || targetRoot == VARCHAR)) { Review comment: why this change? ########## File path: flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala ########## @@ -40,15 +40,14 @@ import org.apache.flink.table.types.logical.utils.LogicalTypeCasts.supportsExpli import org.apache.flink.table.types.logical.utils.LogicalTypeChecks.getFieldTypes import org.apache.flink.table.types.logical.utils.LogicalTypeMerging.findCommonType import org.apache.flink.util.Preconditions.checkArgument - import org.apache.calcite.avatica.util.DateTimeUtils.MILLIS_PER_DAY import org.apache.calcite.avatica.util.{DateTimeUtils, TimeUnitRange} import org.apache.calcite.util.BuiltInMethod +import org.apache.flink.table.api.DataTypes.BYTES import java.lang.{StringBuilder => JStringBuilder} import java.nio.charset.StandardCharsets import java.util.Arrays.asList - Review comment: make sure that you have configured your imports correctly, Scala needs special settings. but it should be the same order as in our code style guides. ########## File path: flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/CastFunctionMiscITCase.java ########## @@ -26,9 +26,20 @@ import org.junit.runners.Parameterized; +import java.nio.ByteBuffer; import java.util.Arrays; import java.util.List; +import static org.apache.flink.table.api.DataTypes.BIGINT; +import static org.apache.flink.table.api.DataTypes.BINARY; +import static org.apache.flink.table.api.DataTypes.BOOLEAN; +import static org.apache.flink.table.api.DataTypes.BYTES; +import static org.apache.flink.table.api.DataTypes.FIELD; +import static org.apache.flink.table.api.DataTypes.INT; +import static org.apache.flink.table.api.DataTypes.ROW; +import static org.apache.flink.table.api.DataTypes.STRING; +import static org.apache.flink.table.api.DataTypes.VARBINARY; +import static org.apache.flink.table.api.DataTypes.of; Review comment: nit: static imports of functions that are called `of` don't contribute to code readability, I would not import it, other types are fine -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org