Simplify ColumnReader SPI: drop iterator API, replace type checks with getValueType()#18918
Open
Jackie-Jiang wants to merge 1 commit into
Open
Simplify ColumnReader SPI: drop iterator API, replace type checks with getValueType()#18918Jackie-Jiang wants to merge 1 commit into
Jackie-Jiang wants to merge 1 commit into
Conversation
Jackie-Jiang
added
release-notes
Jackie-Jiang
added
ingestion
Contributor
There was a problem hiding this comment.
Pull request overview
This PR simplifies the ColumnReader SPI (in pinot-spi) by removing the sequential/iterator read API and boolean type-predicate methods, standardizing consumers on docId-based random access and a single @Nullable PinotDataType getValueType() for type introspection. This reduces SPI surface area and consolidates type/cardinality detection while keeping null semantics consistent for column-major segment building.
Changes:
- Removed
ColumnReaderiterator-style methods (hasNext/next/isNextNull/skipNext/rewind+nextXxx/nextXxxMV) and boolean type checks, replacing them withgetValueType()and docId-based reads. - Updated column-major consumers (
SegmentColumnarIndexCreator,ColumnarSegmentPreIndexStatsContainer) to loopdocId = 0..getTotalDocs()usinggetValue(docId)/ typed accessors guarded bygetValueType(). - Migrated key implementations and tests (segment-local readers + Arrow readers) to the new SPI shape and updated test expectations accordingly.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pinot-spi/src/main/java/org/apache/pinot/spi/data/readers/MultiValueResult.java | Reformats documentation to align examples with docId-based access. |
| pinot-spi/src/main/java/org/apache/pinot/spi/data/readers/ColumnReader.java | Removes iterator API + type predicates; adds getValueType() and toValueType(...) helper; rewrites contract docs. |
| pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/PinotSegmentColumnReaderImpl.java | Drops iterator state; implements getValueType() + docId getValue. |
| pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/DefaultValueColumnReader.java | Drops iterator state; implements getValueType() + docId getValue. |
| pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java | Switches column-major indexing loop to docId-based reads; preserves typed fast path via getValueType(). |
| pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/stats/ColumnarSegmentPreIndexStatsContainer.java | Switches stats collection to docId-based reads; retains typed primitive fast path via getValueType(). |
| pinot-segment-local/src/main/java/org/apache/pinot/segment/local/columntransformer/DataTypeColumnTransformer.java | Simplifies isNoOp() to reader.getValueType() == destType. |
| pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/readers/PinotSegmentColumnReaderImplTest.java | Removes sequential-API assertions; validates getValueType() and docId-based consistency (formatting issues noted). |
| pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/readers/InMemoryColumnReader.java | Updates test reader to implement getValueType() and docId-based access only. |
| pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/readers/DefaultValueColumnReaderTest.java | Updates tests to docId-based reads; adds getValueType() checks (doc/comment nits noted). |
| pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/creator/impl/ColumnReaderInterfaceTest.java | Updates factory/contract tests to docId-based reads. |
| pinot-segment-local/src/test/java/org/apache/pinot/segment/local/columntransformer/DataTypeColumnTransformerTest.java | Updates mocks/tests to use getValueType() rather than boolean type predicates. |
| pinot-plugins/pinot-input-format/pinot-arrow/src/main/java/org/apache/pinot/plugin/inputformat/arrow/ArrowColumnReader.java | Drops iterator API; adds getValueType(); keeps docId reads; doc/annotation mismatch noted. |
| pinot-plugins/pinot-input-format/pinot-arrow/src/main/java/org/apache/pinot/plugin/inputformat/arrow/BatchedArrowColumnReader.java | Drops iterator API; adds getValueType(); doc/annotation mismatch noted. |
| pinot-plugins/pinot-input-format/pinot-arrow/src/test/java/org/apache/pinot/plugin/inputformat/arrow/ArrowFileColumnReaderFactoryTest.java | Updates Arrow tests to docId-based reads + getValueType() expectations. |
| pinot-plugins/pinot-input-format/pinot-arrow/src/test/java/org/apache/pinot/plugin/inputformat/arrow/ArrowColumnReaderFactoryTest.java | Removes sequential-API past-end behavior tests; updates remaining assertions to getValueType(). |
Jackie-Jiang
force-pushed
the
columnreader-cleanup
branch
from
12e2420 to
1358cbc
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #18918 +/- ##
============================================
+ Coverage 64.81% 64.84% +0.03%
Complexity 1347 1347
============================================
Files 3396 3397 +1
Lines 212503 212358 -145
Branches 33484 33469 -15
============================================
- Hits 137732 137707 -25
+ Misses 63610 63524 -86
+ Partials 11161 11127 -34
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Jackie-Jiang
force-pushed
the
columnreader-cleanup
branch
from
1358cbc to
6471bb8
Compare
Jackie-Jiang
force-pushed
the
columnreader-cleanup
branch
from
6471bb8 to
a4001c0
Compare
Jackie-Jiang
force-pushed
the
columnreader-cleanup
branch
from
a4001c0 to
368838c
Compare
Comment on lines
+64
to
+72
| /// 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()`. |
Comment on lines
+87
to
+90
| @Override | ||
| public Object getValue(int docId) { | ||
| return _values[docId]; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Cleans up the
ColumnReaderSPI (pinot-spi), used for column-major segment building, by removing aredundant second read API and simplifying type introspection.
ColumnReaderpreviously exposed two overlapping ways to read a column:hasNext(),next(),isNextNull(),skipNext(), and thenextXxx()/nextXxxMV()type-specific methods, andgetXxx(docId)/getValue(docId).Every production consumer can be expressed with random access alone, so this removes the iterator API
entirely. Both column-major consumers —
SegmentColumnarIndexCreator.indexColumn(String, ColumnReader)and
ColumnarSegmentPreIndexStatsContainer.collectColumn(...)— now read via afor (docId = 0; docId < getTotalDocs(); docId++)loop overgetValue(docId), preserving the previousnull semantics.
The build traverses each column twice over the same cached reader — a statistics pass, then an
index-writing pass — so
rewind()is retained (asdefault void rewind()), but with a narrower contract:reset the reader to document 0 before a repeated forward traversal (it is no longer an iterator cursor
op).
indexColumn(...)calls it before its pass; a random-access reader keeps the default no-op, while astreaming/batched reader overrides it to reset its position.
It also replaces the boolean type-check methods (
isSingleValue()+isInt()…isBytes()) with a single@Nullable PinotDataType getValueType()that encodes both value type and cardinality (e.g.INTvsINT_ARRAY). It returnsnullwhengetValue(docId)'s logical type has no matching typed accessor —BOOLEAN(aBoolean),TIMESTAMP(aTimestamp), and the complex types — which are read throughgetValue(docId); aJSONcolumn reportsSTRING, since it is stored and read as its textString. Astatic
ColumnReader.toValueType(DataType, boolean)helper centralizes the mapping for implementationsbacked by a
DataType, andDataTypeColumnTransformer.isNoOp()collapses togetValueType() == destType.All implementations (
PinotSegmentColumnReaderImpl,DefaultValueColumnReader,ArrowColumnReader,BatchedArrowColumnReader) and their tests are migrated; the interface members are reordered and theJavadoc rewritten for clarity.
Backward incompatibility
ColumnReaderis apinot-spiinterface first shipped in 1.5.0. Removing methods from it isbinary-incompatible for any external plugin that implements or calls
ColumnReader. It is arecently-added, internal-facing SPI for the still-evolving column-major segment build, so the expected
blast radius is nil, but the PR is labeled
backward-incompataccordingly.Release note: the
ColumnReaderSPI no longer exposes the sequential/iterator read API(
hasNext/next/nextXxx/nextXxxMV/skipNext/isNextNull) or the boolean type-check methods(
isSingleValue/isInt/…/isBytes). Read column values by document id viagetValue(docId)/getXxx(docId), and usegetValueType()to obtain the directly-readable value type and cardinality.rewind()is retained as a default method that resets the reader to document 0 for a repeated forwardtraversal.