Copilot commented on code in PR #17183:
URL: https://github.com/apache/pinot/pull/17183#discussion_r2522564854
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/DefaultValueColumnReader.java:
##########
@@ -76,19 +74,254 @@ public Object next()
}
@Override
- public void rewind()
- throws IOException {
+ public void rewind() {
_currentIndex = 0;
}
+ @Override
+ public boolean isNextNull() {
+ if (!hasNext()) {
+ throw new IllegalStateException("No more values available");
+ }
+ return false; // Default values are never null
+ }
+
+ @Override
+ public void skipNext() {
+ if (!hasNext()) {
+ throw new IllegalStateException("No more values available");
+ }
+ _currentIndex++;
+ }
+
+ @Override
+ public int nextInt() {
+ if (!hasNext()) {
+ throw new IllegalStateException("No more values available");
+ }
+ _currentIndex++;
+ return ((Number) _defaultValue).intValue();
+ }
+
+ @Override
+ public long nextLong() {
+ if (!hasNext()) {
+ throw new IllegalStateException("No more values available");
+ }
+ _currentIndex++;
+ return ((Number) _defaultValue).longValue();
+ }
+
+ @Override
+ public float nextFloat() {
+ if (!hasNext()) {
+ throw new IllegalStateException("No more values available");
+ }
+ _currentIndex++;
+ return ((Number) _defaultValue).floatValue();
+ }
+
+ @Override
+ public double nextDouble() {
+ if (!hasNext()) {
+ throw new IllegalStateException("No more values available");
+ }
+ _currentIndex++;
+ return ((Number) _defaultValue).doubleValue();
+ }
+
+ @Override
+ public int[] nextIntMV() {
+ if (!hasNext()) {
+ throw new IllegalStateException("No more values available");
+ }
+ _currentIndex++;
+ return getIntMV(0); // Use existing getIntMV logic
+ }
+
+ @Override
+ public long[] nextLongMV() {
+ if (!hasNext()) {
+ throw new IllegalStateException("No more values available");
+ }
+ _currentIndex++;
+ return getLongMV(0);
+ }
+
+ @Override
+ public float[] nextFloatMV() {
+ if (!hasNext()) {
+ throw new IllegalStateException("No more values available");
+ }
+ _currentIndex++;
+ return getFloatMV(0);
+ }
+
+ @Override
+ public double[] nextDoubleMV() {
+ if (!hasNext()) {
+ throw new IllegalStateException("No more values available");
+ }
+ _currentIndex++;
+ return getDoubleMV(0);
+ }
+
+ @Override
+ public String[] nextStringMV() {
+ if (!hasNext()) {
+ throw new IllegalStateException("No more values available");
+ }
+ _currentIndex++;
+ return getStringMV(0);
+ }
+
+ @Override
+ public byte[][] nextBytesMV() {
+ if (!hasNext()) {
+ throw new IllegalStateException("No more values available");
+ }
+ _currentIndex++;
+ return getBytesMV(0);
+ }
+
@Override
public String getColumnName() {
return _columnName;
}
@Override
- public void close()
- throws IOException {
+ public int getTotalDocs() {
+ return _numDocs;
+ }
+
+ @Override
+ public boolean isNull(int docId) {
+ validateDocId(docId);
+ // Default values are never null
+ return false;
+ }
+
+ // Single-value accessors
+
+ @Override
+ public int getInt(int docId) {
+ validateDocId(docId);
+ return ((Number) _defaultValue).intValue();
+ }
+
+ @Override
+ public long getLong(int docId) {
+ validateDocId(docId);
+ return ((Number) _defaultValue).longValue();
+ }
+
+ @Override
+ public float getFloat(int docId) {
+ validateDocId(docId);
+ return ((Number) _defaultValue).floatValue();
+ }
+
+ @Override
+ public double getDouble(int docId) {
+ validateDocId(docId);
+ return ((Number) _defaultValue).doubleValue();
+ }
+
+ @Override
+ public String getString(int docId) {
+ validateDocId(docId);
+ return (String) _defaultValue;
+ }
+
+ @Override
+ public byte[] getBytes(int docId) {
+ validateDocId(docId);
+ return (byte[]) _defaultValue;
+ }
+
+ // Multi-value accessors
+
+ @Override
+ public int[] getIntMV(int docId) {
+ validateDocId(docId);
+ Object[] defaultArray = (Object[]) _defaultValue;
+ int[] result = new int[defaultArray.length];
+ for (int i = 0; i < defaultArray.length; i++) {
+ result[i] = ((Number) defaultArray[i]).intValue();
+ }
+ return result;
+ }
Review Comment:
The multi-value accessor methods create a new array on every call by
converting from Object[] to primitive arrays. This could be optimized by
caching the converted primitive array during initialization since default
values never change.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/PinotSegmentColumnReaderImpl.java:
##########
@@ -98,6 +214,94 @@ public String getColumnName() {
return _columnName;
}
+ @Override
+ public int getTotalDocs() {
+ return _numDocs;
+ }
+
+ @Override
+ public boolean isNull(int docId) {
+ validateDocId(docId);
+ return _segmentColumnReader.isNull(docId);
+ }
+
+ // Single-value accessors
+
+ @Override
+ public int getInt(int docId) {
+ return _segmentColumnReader.getInt(docId);
Review Comment:
The single-value accessor methods (`getInt()`, `getLong()`, etc.) do not
validate `docId` bounds before delegating to `_segmentColumnReader`, unlike
`isNull()` which calls `validateDocId()`. This inconsistency could lead to
unclear error messages. Either add validation to all accessors or document why
validation is only needed for `isNull()`.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/PinotSegmentColumnReaderFactory.java:
##########
@@ -130,21 +138,19 @@ public Map<String, ColumnReader> getAllColumnReaders()
/**
* Internal method to initialize all column readers during factory
initialization.
*/
- private Map<String, ColumnReader> initializeAllColumnReaders()
- throws IOException {
+ private Map<String, ColumnReader> initializeAllColumnReaders() {
if (_targetSchema == null) {
throw new IllegalStateException("Factory not initialized. Call init()
first.");
Review Comment:
The error message states 'Factory not initialized. Call init() first.' but
this exception can also be thrown if `_colsToRead` is null. The error message
should be more specific about what's missing, or both `_targetSchema` and
`_colsToRead` should be validated.
```suggestion
if (_targetSchema == null || _colsToRead == null) {
StringBuilder errorMsg = new StringBuilder("Factory not initialized.
Call init() first. ");
if (_targetSchema == null) {
errorMsg.append("_targetSchema is null. ");
}
if (_colsToRead == null) {
errorMsg.append("_colsToRead is null.");
}
throw new IllegalStateException(errorMsg.toString().trim());
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/DefaultValueColumnReader.java:
##########
@@ -76,19 +74,254 @@ public Object next()
}
@Override
- public void rewind()
- throws IOException {
+ public void rewind() {
_currentIndex = 0;
}
+ @Override
+ public boolean isNextNull() {
+ if (!hasNext()) {
+ throw new IllegalStateException("No more values available");
+ }
+ return false; // Default values are never null
+ }
+
+ @Override
+ public void skipNext() {
+ if (!hasNext()) {
+ throw new IllegalStateException("No more values available");
+ }
+ _currentIndex++;
+ }
+
+ @Override
+ public int nextInt() {
+ if (!hasNext()) {
+ throw new IllegalStateException("No more values available");
+ }
+ _currentIndex++;
+ return ((Number) _defaultValue).intValue();
+ }
+
+ @Override
+ public long nextLong() {
+ if (!hasNext()) {
+ throw new IllegalStateException("No more values available");
+ }
+ _currentIndex++;
+ return ((Number) _defaultValue).longValue();
+ }
+
+ @Override
+ public float nextFloat() {
+ if (!hasNext()) {
+ throw new IllegalStateException("No more values available");
+ }
+ _currentIndex++;
+ return ((Number) _defaultValue).floatValue();
+ }
+
+ @Override
+ public double nextDouble() {
+ if (!hasNext()) {
+ throw new IllegalStateException("No more values available");
+ }
+ _currentIndex++;
+ return ((Number) _defaultValue).doubleValue();
+ }
+
+ @Override
+ public int[] nextIntMV() {
+ if (!hasNext()) {
+ throw new IllegalStateException("No more values available");
+ }
+ _currentIndex++;
+ return getIntMV(0); // Use existing getIntMV logic
Review Comment:
The sequential MV accessor methods (e.g., `nextIntMV()`) always call the
random access method with `docId=0` (e.g., `getIntMV(0)`). This ignores the
current iterator position tracked by `_currentIndex` and always returns the
same value. It should use `getIntMV(_currentIndex)` before incrementing.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/readers/ColumnReader.java:
##########
@@ -59,6 +166,110 @@ public interface ColumnReader extends Closeable,
Serializable {
Object next()
throws IOException;
+ /**
+ * Check if the next value to be read is null.
+ */
+ boolean isNextNull() throws IOException;
+
+ /**
+ * Move the reader to skip the next value in the column.
+ * This is typically called if isNextNull() returns true to skip the null
value before calling nextInt(), etc
+ * which can't handle null values.
Review Comment:
The Javadoc comment for `skipNext()` states that type-specific methods like
`nextInt()` 'can't handle null values', but this is misleading. The methods can
handle nulls by relying on `isNextNull()` checks, as shown in the usage
patterns. The documentation should clarify that calling `nextInt()` on a null
value results in undefined behavior or a platform-specific default.
```suggestion
* This is typically called if isNextNull() returns true to skip the null
value before calling type-specific methods
* like nextInt(), nextLong(), etc. These methods must only be called
when isNextNull() returns false; calling them
* on a null value results in undefined behavior or a platform-specific
default.
```
--
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]
