RussellSpitzer commented on code in PR #17424:
URL: https://github.com/apache/iceberg/pull/17424#discussion_r3679219293
##########
parquet/src/main/java/org/apache/iceberg/parquet/VariantShreddingAnalyzer.java:
##########
@@ -483,62 +459,56 @@ void observe(VariantValue value) {
}
}
- PhysicalType getMostCommonType() {
- if (mostCommonComputed) {
- return mostCommonCached;
+ PhysicalType admittedType() {
+ if (admittedTypeComputed) {
+ return admittedTypeCached;
}
- Map<PhysicalType, Integer> combinedCounts = Maps.newHashMap();
-
- int integerTotalCount = 0;
- PhysicalType mostCapableInteger = null;
-
- int decimalTotalCount = 0;
- PhysicalType mostCapableDecimal = null;
+ Set<PhysicalType> families = Sets.newHashSet();
Review Comment:
I generally like to avoid having multiple local variables that only work if
they in certain combinations and I think we can make this a bit tighter if
instead of tracking all these things separately we do something like.
if admitted not set - set
if admtted set and this type is wider, set admitted to wider type
if admitted set and this is not in the family - return null exit early on
everything
here is a quick draft i had the llm do
```java
PhysicalType admittedType() {
PhysicalType admitted = null;
for (int i = 0; i < typeCounts.length; i++) {
if (typeCounts[i] == 0) {
continue;
}
PhysicalType merged = mergeFamily(admitted, PHYSICAL_TYPES[i]);
if (merged == null) {
// Mixed type families: do not shred this field.
return null;
}
admitted = merged;
}
return admitted;
}
/**
* Merges {@code candidate} into the currently admitted type.
*
* <p>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).
*/
private static PhysicalType mergeFamily(PhysicalType current, PhysicalType
candidate) {
if (current == null) {
return candidate;
}
if (current == candidate) {
return current;
}
if (isIntegerType(current) && isIntegerType(candidate)) {
// INT8..INT64 are contiguous in PhysicalType declaration order.
return current.ordinal() >= candidate.ordinal() ? current : candidate;
}
if (isDecimalType(current) && isDecimalType(candidate)) {
// DECIMAL4..DECIMAL16 are contiguous in PhysicalType declaration order.
return current.ordinal() >= candidate.ordinal() ? current : candidate;
}
return null;
}
```
--
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]
