Copilot commented on code in PR #17113:
URL: https://github.com/apache/pinot/pull/17113#discussion_r2482361419
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/stats/MapColumnPreIndexStatsCollector.java:
##########
@@ -105,6 +117,58 @@ public void collect(Object entry) {
updatePartition(key);
}
}
+ if (keyStats instanceof NoDictColumnStatisticsCollector) {
+ keyStats.collect(value);
+ continue;
+ }
+ if (keyStats instanceof StringColumnPreIndexStatsCollector) {
+ if (value instanceof String || value instanceof Number || value
instanceof Boolean) {
+ keyStats.collect(String.valueOf(value));
+ continue;
+ }
+ try {
+ keyStats.collect(JsonUtils.objectToString(value));
+ continue;
+ } catch (JsonProcessingException e) {
+ throw new RuntimeException("Failed to serialize value for key '" +
key + "': " + value, e);
+ }
+ }
+ if (keyStats instanceof IntColumnPreIndexStatsCollector) {
+ Number valueNumber = parseFlexibleNumber(value);
+ if (valueNumber == null) {
+ continue;
+ }
+ keyStats.collect(valueNumber.intValue());
+ continue;
+ }
+ if (keyStats instanceof LongColumnPreIndexStatsCollector) {
+ Number valueNumber = parseFlexibleNumber(value);
+ if (valueNumber == null) {
+ continue;
+ }
+ keyStats.collect(valueNumber.longValue());
+ continue;
+ }
+ if (keyStats instanceof FloatColumnPreIndexStatsCollector) {
+ Number valueNumber = parseFlexibleNumber(value);
+ if (valueNumber == null) {
+ continue;
+ }
+ keyStats.collect(valueNumber.floatValue());
+ continue;
+ }
+ if (keyStats instanceof DoubleColumnPreIndexStatsCollector) {
+ Number valueNumber = parseFlexibleNumber(value);
+ if (valueNumber == null) {
+ continue;
+ }
Review Comment:
The `parseFlexibleNumber` method is called multiple times for the same value
across different numeric type checks (Int, Long, Float, Double). If the value
fails to parse, each check still attempts to parse it. Consider parsing once
and reusing the result, or restructuring the logic to avoid redundant parsing
attempts.
```suggestion
// Parse the value once for all numeric collector types
Number valueNumber = null;
if (keyStats instanceof IntColumnPreIndexStatsCollector
|| keyStats instanceof LongColumnPreIndexStatsCollector
|| keyStats instanceof FloatColumnPreIndexStatsCollector
|| keyStats instanceof DoubleColumnPreIndexStatsCollector) {
valueNumber = parseFlexibleNumber(value);
if (valueNumber == null) {
continue;
}
}
if (keyStats instanceof IntColumnPreIndexStatsCollector) {
keyStats.collect(valueNumber.intValue());
continue;
}
if (keyStats instanceof LongColumnPreIndexStatsCollector) {
keyStats.collect(valueNumber.longValue());
continue;
}
if (keyStats instanceof FloatColumnPreIndexStatsCollector) {
keyStats.collect(valueNumber.floatValue());
continue;
}
if (keyStats instanceof DoubleColumnPreIndexStatsCollector) {
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/stats/MapColumnPreIndexStatsCollector.java:
##########
@@ -105,6 +117,58 @@ public void collect(Object entry) {
updatePartition(key);
}
}
+ if (keyStats instanceof NoDictColumnStatisticsCollector) {
+ keyStats.collect(value);
+ continue;
+ }
+ if (keyStats instanceof StringColumnPreIndexStatsCollector) {
+ if (value instanceof String || value instanceof Number || value
instanceof Boolean) {
+ keyStats.collect(String.valueOf(value));
+ continue;
+ }
+ try {
+ keyStats.collect(JsonUtils.objectToString(value));
+ continue;
+ } catch (JsonProcessingException e) {
+ throw new RuntimeException("Failed to serialize value for key '" +
key + "': " + value, e);
+ }
+ }
+ if (keyStats instanceof IntColumnPreIndexStatsCollector) {
+ Number valueNumber = parseFlexibleNumber(value);
+ if (valueNumber == null) {
+ continue;
+ }
+ keyStats.collect(valueNumber.intValue());
+ continue;
+ }
+ if (keyStats instanceof LongColumnPreIndexStatsCollector) {
+ Number valueNumber = parseFlexibleNumber(value);
+ if (valueNumber == null) {
+ continue;
+ }
+ keyStats.collect(valueNumber.longValue());
+ continue;
+ }
+ if (keyStats instanceof FloatColumnPreIndexStatsCollector) {
+ Number valueNumber = parseFlexibleNumber(value);
+ if (valueNumber == null) {
+ continue;
+ }
+ keyStats.collect(valueNumber.floatValue());
+ continue;
+ }
+ if (keyStats instanceof DoubleColumnPreIndexStatsCollector) {
+ Number valueNumber = parseFlexibleNumber(value);
+ if (valueNumber == null) {
+ continue;
+ }
+ keyStats.collect(valueNumber.doubleValue());
+ continue;
+ }
+ if (keyStats instanceof BigDecimalColumnPreIndexStatsCollector) {
+ keyStats.collect(new BigDecimal(value.toString()));
+ continue;
+ }
keyStats.collect(value);
Review Comment:
This instanceof chain for type checking is difficult to maintain and extend.
Consider using polymorphism by adding a `collectValue(Object value)` method to
`AbstractColumnStatisticsCollector` that handles type conversion internally, or
using a strategy pattern with a map of collector types to conversion functions.
```suggestion
keyStats.collectValue(value);
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/stats/MapColumnPreIndexStatsCollector.java:
##########
@@ -113,6 +177,30 @@ public void collect(Object entry) {
}
}
+ @Nullable
+ private Number parseFlexibleNumber(Object input) {
+ if (input instanceof Number) {
+ return (Number) input;
+ }
+
+ String s = input.toString().trim();
+ if (s.isEmpty()) {
+ return null;
+ }
+ try {
+ // Try BigDecimal first — it supports everything cleanly
+ return new BigDecimal(s);
+ } catch (NumberFormatException e) {
+ try {
+ // Try locale parsing fallback
+ NumberFormat nf = NumberFormat.getInstance(Locale.US);
+ return nf.parse(s);
+ } catch (Exception ignored) {
+ return null;
+ }
+ }
+ }
Review Comment:
The `parseFlexibleNumber` method uses overly broad exception handling
(`catch (Exception ignored)`). This swallows all exceptions including
unexpected runtime errors. Consider catching only the specific exception type
(`ParseException`) that `NumberFormat.parse()` throws to avoid masking other
potential issues.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/stats/MapColumnPreIndexStatsCollector.java:
##########
@@ -105,6 +117,58 @@ public void collect(Object entry) {
updatePartition(key);
}
}
+ if (keyStats instanceof NoDictColumnStatisticsCollector) {
+ keyStats.collect(value);
+ continue;
+ }
+ if (keyStats instanceof StringColumnPreIndexStatsCollector) {
+ if (value instanceof String || value instanceof Number || value
instanceof Boolean) {
+ keyStats.collect(String.valueOf(value));
+ continue;
+ }
+ try {
+ keyStats.collect(JsonUtils.objectToString(value));
+ continue;
+ } catch (JsonProcessingException e) {
+ throw new RuntimeException("Failed to serialize value for key '" +
key + "': " + value, e);
+ }
+ }
+ if (keyStats instanceof IntColumnPreIndexStatsCollector) {
+ Number valueNumber = parseFlexibleNumber(value);
+ if (valueNumber == null) {
+ continue;
+ }
+ keyStats.collect(valueNumber.intValue());
+ continue;
+ }
+ if (keyStats instanceof LongColumnPreIndexStatsCollector) {
+ Number valueNumber = parseFlexibleNumber(value);
+ if (valueNumber == null) {
+ continue;
+ }
+ keyStats.collect(valueNumber.longValue());
+ continue;
+ }
+ if (keyStats instanceof FloatColumnPreIndexStatsCollector) {
+ Number valueNumber = parseFlexibleNumber(value);
+ if (valueNumber == null) {
+ continue;
+ }
+ keyStats.collect(valueNumber.floatValue());
+ continue;
+ }
+ if (keyStats instanceof DoubleColumnPreIndexStatsCollector) {
+ Number valueNumber = parseFlexibleNumber(value);
+ if (valueNumber == null) {
+ continue;
+ }
+ keyStats.collect(valueNumber.doubleValue());
+ continue;
+ }
+ if (keyStats instanceof BigDecimalColumnPreIndexStatsCollector) {
+ keyStats.collect(new BigDecimal(value.toString()));
Review Comment:
Creating a `BigDecimal` directly from `value.toString()` can throw
`NumberFormatException` for invalid numeric strings, but this exception is not
caught. This is inconsistent with the numeric parsing done via
`parseFlexibleNumber` for other numeric types. Use `parseFlexibleNumber` and
construct the BigDecimal from the returned Number, or add appropriate exception
handling.
```suggestion
Number valueNumber = parseFlexibleNumber(value);
if (valueNumber == null) {
continue;
}
keyStats.collect(new BigDecimal(valueNumber.toString()));
```
--
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]
