Copilot commented on code in PR #17719:
URL: https://github.com/apache/pinot/pull/17719#discussion_r2820075328
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/stats/MapColumnPreIndexStatsCollector.java:
##########
@@ -276,12 +312,33 @@ private AbstractColumnStatisticsCollector
createKeyStatsCollector(String key, Ob
case BOOLEAN:
case STRING:
case MAP:
- case OBJECT:
return new StringColumnPreIndexStatsCollector(key, config);
default:
- LOGGER.warn("Unknown data type {} for key {} and value type {}", type,
key, value.getClass().getName());
+ LOGGER.warn("Unknown data type {} for key {}", dataType, key);
return new StringColumnPreIndexStatsCollector(key, config);
+ }
+ }
+
+ private AbstractColumnStatisticsCollector
promoteNumericKeyStatsToStringCollector(String key, Object value) {
+ Object firstRawValue = _firstRawValuesByKey.get(key);
+ _firstRawValuesByKey.putIfAbsent(key, value);
Review Comment:
This line is redundant and has no effect. The key already exists in
_firstRawValuesByKey (added at line 112 when the key was first seen), so
putIfAbsent will not update the map. Since the current value is already being
collected on line 332, this line should be removed.
```suggestion
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/stats/MapColumnPreIndexStatsCollector.java:
##########
@@ -276,12 +312,33 @@ private AbstractColumnStatisticsCollector
createKeyStatsCollector(String key, Ob
case BOOLEAN:
case STRING:
case MAP:
- case OBJECT:
return new StringColumnPreIndexStatsCollector(key, config);
default:
- LOGGER.warn("Unknown data type {} for key {} and value type {}", type,
key, value.getClass().getName());
+ LOGGER.warn("Unknown data type {} for key {}", dataType, key);
return new StringColumnPreIndexStatsCollector(key, config);
+ }
Review Comment:
The switch statement is missing a case for TIMESTAMP data type. The
convertToDataType method (line 362) can return FieldSpec.DataType.TIMESTAMP,
but this case is not explicitly handled in the switch. TIMESTAMP should be
handled as LONG since its stored type is LONG. Add a case for TIMESTAMP that
returns a LongColumnPreIndexStatsCollector.
--
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]
