RussellSpitzer commented on code in PR #17424:
URL: https://github.com/apache/iceberg/pull/17424#discussion_r3686359374
##########
parquet/src/main/java/org/apache/iceberg/parquet/VariantShreddingAnalyzer.java:
##########
@@ -475,83 +444,65 @@ void observe(VariantValue value) {
typeCounts[type.ordinal()]++;
// Track max precision and scale for decimal types
- if (isDecimalType(type)) {
+ if (DECIMAL_TYPES.contains(type)) {
if (value.asPrimitive().get() instanceof BigDecimal bd) {
maxDecimalIntegerDigits = Math.max(maxDecimalIntegerDigits,
bd.precision() - bd.scale());
maxDecimalScale = Math.max(maxDecimalScale, bd.scale());
}
}
}
- PhysicalType getMostCommonType() {
- if (mostCommonComputed) {
- return mostCommonCached;
- }
-
- Map<PhysicalType, Integer> combinedCounts = Maps.newHashMap();
-
- int integerTotalCount = 0;
- PhysicalType mostCapableInteger = null;
-
- int decimalTotalCount = 0;
- PhysicalType mostCapableDecimal = null;
-
+ /**
+ * Returns the single type family that all observations fall into after
numeric widening, or
+ * null if observations span multiple families.
+ */
+ PhysicalType admittedType() {
+ PhysicalType admitted = null;
for (int i = 0; i < typeCounts.length; i++) {
- int count = typeCounts[i];
- if (count == 0) {
+ if (typeCounts[i] == 0) {
continue;
}
- PhysicalType type = PHYSICAL_TYPES[i];
-
- if (isIntegerType(type)) {
- integerTotalCount += count;
- if (mostCapableInteger == null
- || INTEGER_PRIORITY.get(type) >
INTEGER_PRIORITY.get(mostCapableInteger)) {
- mostCapableInteger = type;
- }
- } else if (isDecimalType(type)) {
- decimalTotalCount += count;
- if (mostCapableDecimal == null
- || DECIMAL_PRIORITY.get(type) >
DECIMAL_PRIORITY.get(mostCapableDecimal)) {
- mostCapableDecimal = type;
- }
- } else {
- combinedCounts.put(type, count);
+ PhysicalType merged = mergeFamily(admitted, PHYSICAL_TYPES[i]);
+ if (merged == null) {
+ return null;
}
+ admitted = merged;
}
+ return admitted;
+ }
- if (mostCapableInteger != null) {
- combinedCounts.put(mostCapableInteger, integerTotalCount);
+ /**
+ * Merges {@code candidate} into the currently admitted type. Returns the
wider type when both
+ * are in the same integer or decimal family, {@code candidate} when
nothing is admitted yet,
+ * {@code current} when the types are identical, and null when the types
are from incompatible
+ * families (including FLOAT vs DOUBLE, TIMESTAMPTZ vs TIMESTAMPTZ_NANOS,
etc.).
+ */
+ private static PhysicalType mergeFamily(PhysicalType current, PhysicalType
candidate) {
+ if (current == null) {
+ return candidate;
}
-
- if (mostCapableDecimal != null) {
- combinedCounts.put(mostCapableDecimal, decimalTotalCount);
+ if (current == candidate) {
+ return current;
}
-
- // Pick the most common type with tie-breaking
- mostCommonCached =
- combinedCounts.entrySet().stream()
- .max(
- Map.Entry.<PhysicalType, Integer>comparingByValue()
- .thenComparingInt(
- entry ->
TIE_BREAK_PRIORITY.getOrDefault(entry.getKey(), -1)))
- .map(Map.Entry::getKey)
- .orElse(null);
- mostCommonComputed = true;
- return mostCommonCached;
- }
-
- private static boolean isIntegerType(PhysicalType type) {
- return type == PhysicalType.INT8
- || type == PhysicalType.INT16
- || type == PhysicalType.INT32
- || type == PhysicalType.INT64;
+ PhysicalType widened = wider(current, candidate, INTEGER_TYPES);
Review Comment:
may be overkill if we never add another family .. but
```java
private static PhysicalType mergeFamily(PhysicalType current, PhysicalType
candidate) {
if (current == null) {
return candidate;
}
if (current == candidate) {
return current;
}
List<PhysicalType> family = familyOf(current);
if (family == null) {
return null; // current has no widening family (STRING, FLOAT, …)
}
return wider(current, candidate, family); // null if candidate not in that
family
}
private static List<PhysicalType> familyOf(PhysicalType type) {
if (INTEGER_TYPES.contains(type)) {
return INTEGER_TYPES;
}
if (DECIMAL_TYPES.contains(type)) {
return DECIMAL_TYPES;
}
return null;
}
```
I dunno ... i'll let you decide :)
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
