xiangfu0 commented on PR #18638:
URL: https://github.com/apache/pinot/pull/18638#issuecomment-4664542467
Thanks for this — the design reads cleanly (caller-managed vs. file-managed
factories, `VectorAppender` bulk-copy per batch, loud dictionary rejection),
and the reader-level unit tests cover nulls, MV element-nulls, multi-batch
boundaries, rewind, and column subsetting well. I focused the review on the
error/edge paths and the end-to-end columnar-build contract, where I found a
few things worth addressing before merge.
## Blocking
### 1. Off-heap memory leak on every error path in
`ArrowAccumulators.populate()`
The per-column accumulator vectors are `allocateNew()`'d into a local map,
but `populate()` can throw *after* one or more allocations and before it
returns the `Result`:
- the dictionary-rejection `Preconditions.checkArgument` (L73) when a
*later* column is dictionary-encoded,
- `reader.loadNextBatch()` on a corrupt/truncated source (L90),
- `source.accept(appender, null)` (L98),
- the `"Arrow source contains no non-empty record batches"` IOException
(L103).
In all of these the already-allocated vectors are never closed and never
handed to the factory (`_accumulatorVectors` is only assigned from the returned
`Result`), so the off-heap buffers leak. For `ArrowFileColumnReaderFactory` the
subsequent `_allocator.close()` then throws `IllegalStateException` on the
outstanding allocation — masking the original error; for the caller-managed
factory the leak lands in the caller's allocator and surfaces when they close
it. In a long-lived minion/executor building many segments, repeated failures
accumulate native memory.
```java
Map<String, FieldVector> accumulators = new LinkedHashMap<>();
try {
// ... allocate accumulators, walk batches, build readers ...
return new Result(accumulators, readers, availableColumns);
} catch (Throwable t) {
closeAll(accumulators); // release off-heap before propagating
throw t;
}
```
### 2. Boolean / `BitVector` columns break the columnar build
`ArrowColumnReader` advertises `BitVector → INT`, `isInt()` returns true for
it (L127), and `getInt()` returns 0/1 (L301). But `getValue()` (L369-381)
returns `_vector.getObject(docId)`, which for a `BitVector` is a `Boolean`. The
columnar stats path consumes columns through `getValue()`
(`ColumnarSegmentPreIndexStatsContainer` →
`statsCollector.collect(columnReader.getValue(i))`), and
`IntColumnPreIndexStatsCollector.collect(Object)` does `collect((int) entry)` →
`ClassCastException: Boolean cannot be cast to Integer`. So a Bit column — a
documented-supported INT source — fails the build. Suggest `getValue()` return
`Integer` 0/1 for `BitVector` (consistent with `getInt()`), or drop Bit from
the supported-INT matrix.
### 3. Nullable single-value numeric columns NPE at stats collection
`ColumnarSegmentPreIndexStatsContainer` iterates
`statsCollector.collect(columnReader.getValue(i))` with no null guard.
`ArrowColumnReader.getValue()` correctly returns `null` for null Arrow slots
(per the SPI contract), and `IntColumnPreIndexStatsCollector.collect((int)
null)` → NPE. The index-creation pass
(`SegmentColumnarIndexCreator.indexColumn`) *does* handle `null →
defaultNullValue`, but stats collection runs first in `buildColumnar()`, so a
single null in an INT/LONG/FLOAT/DOUBLE column aborts the build.
Root cause is the shared columnar stats container (introduced with the
ColumnReader SPI in #16727), not the Arrow code — but this is the first factory
to exercise it, and the integration test uses only fully-populated columns, so
the gap ships hidden. Suggest either teaching the stats container to
skip/handle nulls (or fail fast with a clear message), plus a null-bearing
integration test.
### 4. "Missing target column → driver fills defaults" is documented but not
implemented
`ArrowFileColumnReaderFactory` Javadoc (L47-49) — and
`ArrowColumnReaderFactory` by extension — states that absent target-schema
columns are tolerated and "schema-evolution defaults are the columnar build
driver's responsibility." In practice both
`ColumnarSegmentPreIndexStatsContainer` (`Preconditions.checkState(columnReader
!= null, ...)`) and `SegmentIndexCreationDriverImpl.buildColumnar()` (`throw
new IllegalStateException("No column reader found for column: ...")`) abort
when a physical schema column is missing from the source. Either implement
default-fill, or correct the Javadoc to state that every physical column must
be present in the Arrow source.
## Should fix
### 5. Test coverage of the new error/edge paths
No tests exercise: dictionary-encoded rejection, the empty-source
IOException, the `typeMismatch` IOExceptions, a target column absent from the
source, or the non-INT multi-value branches (`getLongMV` / `getFloatMV` /
`getDoubleMV` / `getBigDecimalMV` / `getStringMV` / `getBytesMV` — each has its
own element-vector cast and null handling that a copy/paste slip would break
silently). The `ArrowColumnarBuildIntegrationTest` equivalence assertion covers
only single-value, non-null INT/LONG/STRING in a single batch — i.e. it
sidesteps exactly the cases in #2 and #3. Recommend extending it to nulls, the
full primitive matrix, and MV, and adding `assertThrows` tests for the guard
paths.
### 6. `isNextNull()` / `skipNext()` are unbounded
These (L109-118) don't bounds-check `_nextDocId`, whereas the random-access
`get*(docId)` methods all `checkBounds`. A typed-pattern loop that runs past
the end reads beyond the validity buffer instead of failing fast. Either add a
`hasNext()`/bounds guard for parity, or document that the sequential methods
are caller-gated on `hasNext()`.
## Nits
- `getBigDecimalMV` / `getStringMV` / `getBytesMV` cast
`list.getDataVector()` to the element vector type without an `instanceof` check
(L417 / L434 / L456), so a type mismatch throws a raw `ClassCastException`
instead of the clean `typeMismatch(...)` IOException the primitive MV path
produces.
- Empty source throws, whereas the row-major `ArrowRecordReader` path yields
a 0-doc segment and the columnar framework explicitly handles `totalDocs == 0`
— a behavioral divergence between the two Arrow paths.
- `ColumnReaderFactory` / `ColumnReader` are `Serializable`, but
`ArrowColumnReaderFactory` holds non-transient `final ArrowReader` /
`BufferAllocator` and `ArrowColumnReader` a non-transient `final FieldVector`,
so serializing any instance throws `NotSerializableException`.
`ArrowFileColumnReaderFactory` is correctly all-transient + a `File`. Worth a
note on the limitation.
- `ArrowAccumulators` class Javadoc (L41) references `{@link
InMemoryArrowColumnReaderFactory}`, which doesn't exist (renamed to
`ArrowColumnReaderFactory`) — broken `@link`.
Overall the core abstraction is sound; the issues cluster in error/edge
handling and in docs that currently promise more than the build path delivers.
Happy to look again once #1–#4 are addressed.
--
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]
