featzhang commented on code in PR #28144:
URL: https://github.com/apache/flink/pull/28144#discussion_r3228547863
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/extraction/DataTypeExtractor.java:
##########
@@ -544,6 +550,15 @@ else if (clazz == java.time.Period.class) {
return ClassDataTypeConverter.extractDataType(clazz).orElse(null);
}
+ /** Maps an {@link Enum} class to STRING bridged to the enum class. */
+ private @Nullable DataType extractEnumType(Type type) {
+ final Class<?> clazz = toClass(type);
+ if (clazz == null || !clazz.isEnum()) {
+ return null;
+ }
+ return DataTypes.STRING().bridgedTo(clazz);
Review Comment:
The enum extraction returns `STRING` bridged to the enum class, which is
clean.
One consideration: what if the enum has custom `toString()` methods? The
converter uses `Enum.name()` which is always the constant name, but users might
expect `toString()`.
For example:
```java
enum Status {
ACTIVE { public String toString() { return "active"; } },
INACTIVE { public String toString() { return "inactive"; } }
}
```
Using `name()` would give "ACTIVE", but `toString()` gives "active". The
current behavior is consistent with standard serialization practices, but worth
documenting if this becomes a user question.
##########
flink-table/flink-table-type-utils/src/main/java/org/apache/flink/table/data/conversion/StringEnumConverter.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * 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.flink.table.data.conversion;
+
+import org.apache.flink.annotation.Internal;
+import org.apache.flink.table.data.StringData;
+import org.apache.flink.table.types.DataType;
+import org.apache.flink.table.types.logical.CharType;
+import org.apache.flink.table.types.logical.VarCharType;
+
+/**
+ * Converter for {@link CharType}/{@link VarCharType} of {@link Enum} external
type. The enum is
+ * represented as its {@link Enum#name()}.
+ */
+@Internal
+@SuppressWarnings({"rawtypes", "unchecked"})
+public class StringEnumConverter implements DataStructureConverter<StringData,
Enum> {
+
+ private static final long serialVersionUID = 1L;
+
+ private final Class<? extends Enum> enumClass;
+
+ public StringEnumConverter(Class<? extends Enum> enumClass) {
+ this.enumClass = enumClass;
+ }
+
+ @Override
+ public StringData toInternal(Enum external) {
+ return StringData.fromString(external.name());
+ }
+
+ @Override
+ public Enum toExternal(StringData internal) {
+ return Enum.valueOf(enumClass, internal.toString());
Review Comment:
The converter uses `Enum.valueOf()` which throws `IllegalArgumentException`
if the string doesn't match any constant name.
Should the converter handle invalid enum values gracefully? For example, if
the table contains "INVALID_VALUE" but the enum only has FIRST/SECOND/THIRD.
Current behavior: exception propagates to the runtime
Alternative: return null or a default value
Depending on Flink's error handling policy for data conversion, this might
be fine. Just worth noting that malformed data will cause job failures rather
than null values.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
