Copilot commented on code in PR #17113:
URL: https://github.com/apache/pinot/pull/17113#discussion_r2488961094
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/stats/MapColumnPreIndexStatsCollector.java:
##########
@@ -105,6 +111,67 @@ 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 BigDecimalColumnPreIndexStatsCollector) {
+ try {
+ keyStats.collect(new BigDecimal(value.toString()));
+ } catch (NumberFormatException e) {
+ LOGGER.error("Failed to parse BigDecimal for key '{}', value '{}':
{}", key, value, e.getMessage());
Review Comment:
The error message logs the failure but doesn't explain the impact or
recovery behavior. Consider adding context about how the statistics collection
continues after this error, e.g., "Failed to parse BigDecimal for key '{}',
value '{}': {}. Skipping value for statistics collection."
```suggestion
LOGGER.error("Failed to parse BigDecimal for key '{}', value
'{}': {}. Skipping value for statistics collection.", key, value,
e.getMessage());
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/stats/MapColumnPreIndexStatsCollector.java:
##########
@@ -105,6 +111,67 @@ 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 BigDecimalColumnPreIndexStatsCollector) {
+ try {
+ keyStats.collect(new BigDecimal(value.toString()));
+ } catch (NumberFormatException e) {
+ LOGGER.error("Failed to parse BigDecimal for key '{}', value '{}':
{}", key, value, e.getMessage());
+ // Skip collecting this value for statistics
+ }
+ continue;
+ }
+ if (value instanceof Number) {
+ Number valueNumber = (Number) value;
+ 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) {
+ keyStats.collect(valueNumber.doubleValue());
+ continue;
+ }
+ }
+ if (keyStats instanceof IntColumnPreIndexStatsCollector) {
+ keyStats.collect(Integer.parseInt(value.toString()));
+ continue;
+ }
+ if (keyStats instanceof LongColumnPreIndexStatsCollector) {
+ keyStats.collect(Long.parseLong(value.toString()));
+ continue;
+ }
+ if (keyStats instanceof FloatColumnPreIndexStatsCollector) {
+ keyStats.collect(Float.parseFloat(value.toString()));
+ continue;
+ }
+ if (keyStats instanceof DoubleColumnPreIndexStatsCollector) {
+ keyStats.collect(Double.parseDouble(value.toString()));
Review Comment:
These parsing operations lack error handling for `NumberFormatException`. If
`value.toString()` produces a non-parseable string (e.g., "invalid"), the code
will throw an unchecked exception. Unlike the BigDecimal case above (lines
130-137), these don't have try-catch blocks. Add try-catch blocks with
appropriate error handling similar to the BigDecimal case.
```suggestion
try {
keyStats.collect(Integer.parseInt(value.toString()));
} catch (NumberFormatException e) {
LOGGER.error("Failed to parse Integer for key '{}', value '{}':
{}", key, value, e.getMessage());
// Skip collecting this value for statistics
}
continue;
}
if (keyStats instanceof LongColumnPreIndexStatsCollector) {
try {
keyStats.collect(Long.parseLong(value.toString()));
} catch (NumberFormatException e) {
LOGGER.error("Failed to parse Long for key '{}', value '{}':
{}", key, value, e.getMessage());
// Skip collecting this value for statistics
}
continue;
}
if (keyStats instanceof FloatColumnPreIndexStatsCollector) {
try {
keyStats.collect(Float.parseFloat(value.toString()));
} catch (NumberFormatException e) {
LOGGER.error("Failed to parse Float for key '{}', value '{}':
{}", key, value, e.getMessage());
// Skip collecting this value for statistics
}
continue;
}
if (keyStats instanceof DoubleColumnPreIndexStatsCollector) {
try {
keyStats.collect(Double.parseDouble(value.toString()));
} catch (NumberFormatException e) {
LOGGER.error("Failed to parse Double for key '{}', value '{}':
{}", key, value, e.getMessage());
// Skip collecting this value for statistics
}
```
--
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]
