Copilot commented on code in PR #19273:
URL: https://github.com/apache/pinot/pull/19273#discussion_r3794131447
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/map/MapKeyIndexReader.java:
##########
@@ -33,12 +33,29 @@ public class MapKeyIndexReader implements
ForwardIndexReader {
private final FieldSpec _keyFieldSpec;
private final PreparedMapKey _mapKey;
private final Object _defaultNullValue;
+ private final String _defaultNullValueString;
public MapKeyIndexReader(ForwardIndexReader forwardIndexReader, String
keyName, FieldSpec keyFieldSpec) {
_forwardIndexReader = forwardIndexReader;
_mapKey = new PreparedMapKey(keyName);
_keyFieldSpec = keyFieldSpec;
_defaultNullValue = keyFieldSpec.getDefaultNullValue();
+ _defaultNullValueString = _defaultNullValue.toString();
+ }
Review Comment:
`MapKeyIndexReader` caches the default-null substitution via
`_defaultNullValue.toString()`. Pinot already centralizes string rendering of
default nulls in `FieldSpec#getDefaultNullValueString()` (e.g., it handles
`byte[]` and other types consistently; see
`pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java:431-433`).
Using `toString()` here can yield inconsistent results (notably for `byte[]`,
where `toString()` is an identity string). Consider caching
`keyFieldSpec.getDefaultNullValueString()` instead to match the codebase
convention and avoid type-specific surprises.
--
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]
