RussellSpitzer commented on code in PR #17424:
URL: https://github.com/apache/iceberg/pull/17424#discussion_r3778617598
##########
parquet/src/main/java/org/apache/iceberg/parquet/VariantShreddingAnalyzer.java:
##########
@@ -475,83 +446,80 @@ 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;
+ }
+
+ /**
+ * Widens {@code current} with {@code candidate}, or null if they belong
to different families.
+ */
+ private static PhysicalType mergeFamily(PhysicalType current, PhysicalType
candidate) {
+ if (current == null) {
+ return candidate;
}
- if (mostCapableInteger != null) {
- combinedCounts.put(mostCapableInteger, integerTotalCount);
+ if (current == candidate) {
+ return current;
}
- if (mostCapableDecimal != null) {
- combinedCounts.put(mostCapableDecimal, decimalTotalCount);
+ List<PhysicalType> family = familyOf(current);
+ if (family == null) {
+ return null;
}
- // 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;
+ return wider(current, candidate, family);
}
- private static boolean isIntegerType(PhysicalType type) {
- return type == PhysicalType.INT8
- || type == PhysicalType.INT16
- || type == PhysicalType.INT32
- || type == PhysicalType.INT64;
+ /** Returns the widening family for {@code type}, or null if none applies.
*/
+ 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;
}
- private static boolean isDecimalType(PhysicalType type) {
- return type == PhysicalType.DECIMAL4
- || type == PhysicalType.DECIMAL8
- || type == PhysicalType.DECIMAL16;
+ /**
+ * Returns the wider of {@code first} and {@code second} within {@code
family}, or null when
+ * {@code second} is not in the family. {@code first} is always a member.
+ */
+ private static PhysicalType wider(
+ PhysicalType first, PhysicalType second, List<PhysicalType> family) {
+ int firstIdx = family.indexOf(first);
Review Comment:
paranoid nit :
Precondition (firstIdx >= 0)
--
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]
