nssalian commented on code in PR #17424:
URL: https://github.com/apache/iceberg/pull/17424#discussion_r3686379859
##########
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:
I'll tell you I had this locally and chose not to do it because we had just
two. Let me do this so it's better future looking.
--
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]
