fresh-borzoni commented on code in PR #3447:
URL: https://github.com/apache/fluss/pull/3447#discussion_r3461385156
##########
fluss-client/src/main/java/org/apache/fluss/client/converter/FlussTypeToPojoTypeConverter.java:
##########
@@ -31,9 +31,30 @@
import java.time.LocalTime;
import java.time.OffsetDateTime;
import java.time.ZoneOffset;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
/** Shared utilities for Fluss type and Pojo type. */
public class FlussTypeToPojoTypeConverter {
+
+ private static final Map<Class<?>, Map<String, Object>>
ENUM_CONSTANTS_CACHE =
Review Comment:
This static cache is keyed by the user's enum Class and never evicted. Under
a Flink user-code classloader it pins the class (and its classloader) across
job restarts, so metaspace leak.
The rest of this path builds per-field converters once in the closure.
Could we build the name -> constant map there and capture it, instead of a
static map? That was the original idea and it drops the static state entirely.
##########
fluss-client/src/test/java/org/apache/fluss/client/converter/PojoArrayToFlussArrayTest.java:
##########
@@ -242,6 +243,12 @@ public void testArrayWithAllTypes() {
assertThat(resultMap2).containsEntry("test_3", 3);
assertThat(resultMap2).containsEntry("test_4", 4);
+ // Verify enum array
+ InternalArray enumArray = row.getArray(22);
+ assertThat(enumArray.size()).isEqualTo(3);
+ assertThat(enumArray.getString(0).toString()).isEqualTo("PENDING");
Review Comment:
This only checks the write direction. Could we add the read-back (Row ->
POJO) for an enum array, and a List<MyEnum> field too?
The collection read paths (FlussArrayToPojoArray, and map keys/values via
FlussMapToPojoMap) aren't exercised for enums yet.
##########
fluss-client/src/test/java/org/apache/fluss/client/converter/ConvertersTestFixtures.java:
##########
@@ -151,6 +155,7 @@ public static TestPojo sample() {
put("test_2", 2);
}
},
+ StatusEnum.OK,
Review Comment:
The round-trip test only ever sees OK.
Since the lowercase case is the one that was broken, could sample() use
error (or add a case that does) so a lowercase enum goes through the full POJO
-> Row -> POJO path?
##########
fluss-client/src/main/java/org/apache/fluss/client/converter/FlussTypeToPojoTypeConverter.java:
##########
@@ -31,9 +31,30 @@
import java.time.LocalTime;
import java.time.OffsetDateTime;
import java.time.ZoneOffset;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
/** Shared utilities for Fluss type and Pojo type. */
public class FlussTypeToPojoTypeConverter {
+
+ private static final Map<Class<?>, Map<String, Object>>
ENUM_CONSTANTS_CACHE =
+ new ConcurrentHashMap<>();
+
+ /**
+ * Builds a map of enum name (uppercase) to enum constant for the given
enum class. This map is
+ * cached to avoid recreating it for every enum conversion.
+ */
+ private static Map<String, Object> buildEnumConstantsMap(Class<?>
enumClass) {
+ Map<String, Object> map = new HashMap<>();
+ for (Object constant : enumClass.getEnumConstants()) {
+ map.put(constant.toString(), constant);
Review Comment:
Should this key on name() rather than toString()? toString() is overridable
and not guaranteed unique, so two constants can collapse the map and a row
reads back as the wrong constant
##########
fluss-client/src/main/java/org/apache/fluss/client/converter/ConverterCommons.java:
##########
@@ -148,6 +148,15 @@ static void validateCompatibility(DataType fieldType,
PojoType.Property prop) {
}
return;
}
+ if (actual.isEnum()) {
+ if (typeRoot != DataTypeRoot.STRING) {
+ throw new IllegalArgumentException(
+ String.format(
+ "Enum field '%s' must be a string type, got
%s",
Review Comment:
Small thing: this fires for CHAR too, but CHAR is a string type (it's in
SUPPORTED_TYPES), so the message reads as self-contradictory.
Probably better wording is "STRING column", 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
