real-mj-song opened a new pull request, #18797:
URL: https://github.com/apache/pinot/pull/18797
**TL;DR:** The column-major build added in #18638 removed per-row
`GenericRow` materialization, but it still consumes every value through the
generic `Object` path — so each primitive is **boxed twice**, once on the stats
pass and once on the index pass. This PR adds a typed, boxing-free fast path
for single-value `INT`/`LONG`/`FLOAT`/`DOUBLE` columns:
`ColumnReader.nextInt()` → `collect(int)` / `indexOfSV(int)` →
`IndexCreator.addInt(value, dictId)`. Purely additive; nulls, multi-value, and
every other type fall back to the unchanged Object path. **No SPI changes, no
new public API.**
Tracks #18629. Builds on #18638 (column-major build) and the `ColumnReader`
/ `ColumnReaderFactory` SPI (#16727).
### Problem
#18629 set out to build a segment from a columnar source without "the
per-row `GenericRow` allocation **and** per-primitive boxing." #18638 delivered
the column-major path and removed the `GenericRow` allocation, but it routes
values through the generic `Object` API:
```
columnReader.next() // Object — boxes the primitive
-> ColumnarValueNormalizer.normalize(..) // Object
-> dictionaryCreator.indexOfSV(Object) // boxed dictionary lookup
-> creator.add(Object, dictId) // unboxes again to write
```
So the per-primitive boxing #18629 aimed to remove is still on the hot path
— and twice over, since `buildColumnar()` reads every column once for stats and
once for indexing. (This is the follow-up requested in the #18638 review.)
### What this PR does
The `ColumnReader` SPI (#16727) already exposes typed accessors
(`nextInt()`, `isInt()`, …) and the downstream sinks already have primitive
overloads — `AbstractColumnStatisticsCollector.collect(int)`,
`SegmentDictionaryCreator.indexOfSV(int)`, and `addInt(value, dictId)` on
`ForwardIndexCreator` / `CombinedInvertedIndexCreator` /
`DictionaryBasedInvertedIndexCreator`. They were simply never wired to the
column-major build. This PR wires them:
- **Stats pass** (`ColumnarSegmentPreIndexStatsContainer`) — when the reader
can serve a single-value column as a primitive (`isInt()`/`isLong()`/…), read
with `nextInt()`/… and feed `collect(int)`/… directly.
- **Index pass** (`SegmentColumnarIndexCreator`) — same dispatch:
`nextInt()` → `indexOfSV(int)` → `creator.addInt(value, dictId)` across the
column's index creators (creators with no typed override keep the boxing
default and stay correct).
Key choices:
- **Gated on the logical `FieldSpec` type** (`INT`/`LONG`/`FLOAT`/`DOUBLE`),
so `BOOLEAN` (stored `INT`) and `TIMESTAMP` (stored `LONG`) — which require
value coercion the typed read would skip — correctly stay on the Object path.
- **Null docs route through the existing Object path** for byte-for-byte
parity, including null-value-vector marking and default substitution. The typed
primitive accessor is only called when `isNextNull()` is false.
- **Multi-value and non-primitive types are unchanged.**
Net effect: for single-value primitive columns, neither build pass boxes —
removing the remaining per-primitive boxing on both column reads.
### Scope
Single-value `INT`/`LONG`/`FLOAT`/`DOUBLE` only. The typed multi-value sinks
(`addIntMV`, `nextIntMV` / `MultiValueResult`) already exist but are
intentionally **not** wired here — the multi-value element-null contract is a
distinct concern and is better handled on its own.
### Compatibility
Additive and behavior-preserving: **no SPI changes**, no new public API; the
row-major path and the Object-path column-major fallback are untouched. The
only non-fast-path edit documents the existing two-pass `rewind()` precondition
on the `init(config, ColumnReaderFactory)` overload (the consumer that imposes
it), leaving the `ColumnReaderFactory` SPI itself generic.
### Testing
- The existing column-major equivalence suites already drive the new typed
branches and continue to pass — in particular `ColumnarRowMajorEquivalenceTest`
builds a segment column-major vs. row-major over single-value
`INT`/`LONG`/`FLOAT`/`DOUBLE` columns with ~10% nulls and asserts per-doc value
and min/max/cardinality equality.
- Added two `ArrowColumnarBuildIntegrationTest` cases that pin null-edge
shapes only an Arrow source can produce, with **absolute** expected values
(independent of the row-major oracle, so they also catch a regression in the
shared normalization):
- element-level multi-value nulls + whole-array null + all-null-elements +
empty list → null elements dropped; all-null / empty list resolves to the
column default;
- an all-null single-value column → cardinality 1, every doc reads the
column default.
### References
Tracks #18629 · builds on #18638 · `ColumnReader` SPI #16727
--
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]
