Copilot commented on code in PR #18918:
URL: https://github.com/apache/pinot/pull/18918#discussion_r3532298210
##########
pinot-plugins/pinot-input-format/pinot-arrow/src/main/java/org/apache/pinot/plugin/inputformat/arrow/ArrowToPinotTypeConverter.java:
##########
@@ -59,6 +61,40 @@ public final class ArrowToPinotTypeConverter {
private ArrowToPinotTypeConverter() {
}
+ /// Returns the [PinotDataType] that a column with the given Arrow element
[Types.MinorType] can be read as through a
+ /// type-specific `ColumnReader` accessor, or `null` when it has no
dedicated accessor and must be read through
+ /// `ColumnReader.getValue()`. `singleValue` selects the scalar vs `_ARRAY`
variant.
+ ///
+ /// The `MinorType` already encodes every distinction the accessors care
about, so a plain switch suffices: only
+ /// signed 32/64-bit ints (`INT` / `BIGINT`), single/double floats, 128-bit
`DECIMAL`, `VARCHAR`, and `VARBINARY`
+ /// have accessors. Everything else — unsigned/narrow ints, half floats,
`DECIMAL256`, `LARGEVARCHAR` /
+ /// `LARGEVARBINARY` / fixed-size binary, `BIT` (boolean), temporal, and
complex types — is a distinct `MinorType`
+ /// that falls through to `null` and is read via `getValue()`.
Review Comment:
The doc comment refers to `ColumnReader.getValue()` / `getValue()` without
parameters, but the SPI method is `getValue(int)`. Using the correct signature
avoids confusion for implementers/callers reading this helper’s contract.
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/readers/InMemoryColumnReader.java:
##########
@@ -202,6 +84,11 @@ public boolean isNull(int docId) {
return _isNull[docId];
}
+ @Override
+ public Object getValue(int docId) {
+ return _values[docId];
+ }
Review Comment:
`ColumnReader#getValue(int)` is documented to return an `Object[]` for
multi-value columns, but this mock currently returns the raw stored value (e.g.
an `int[]`), which breaks the SPI contract and can make tests accidentally
depend on implementation details. Consider boxing primitive arrays when
`_isSingleValue` is false so MV `getValue()` matches the interface contract.
--
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]
