Neuw84 commented on PR #17296:
URL: https://github.com/apache/iceberg/pull/17296#issuecomment-5106399967
Have been tinkering a little bit more on that **0**.
I went looking for where a zero batch size can come from, because the answer
decides whether the
`== 0` guard should exist at all. What I found:
**A zero batch size is reachable, and only ever from an explicit caller.**
There is no validation
anywhere on the path:
- `Parquet.ReadBuilder.recordsPerBatch(int)` - no check, default 10000.
- `new ArrowReader(TableScan, int batchSize, boolean)` - public API, no
check, passes straight through
to `recordsPerBatch`.
- Spark: `read.parquet.vectorization.batch-size` / the matching SQL conf,
default
`PARQUET_BATCH_SIZE_DEFAULT = 5000`, flows through
`SparkReadConf.parquetBatchSize()` ->
`ParquetBatchReadConf` -> `recordsPerBatch`. A user setting `0` is
accepted.
`DEFAULT_BATCH_SIZE` is also 5000, the same value as
`PARQUET_BATCH_SIZE_DEFAULT`, so the guard reads
like "treat 0 as unset and fall back to the default".
**But a zero batch size cannot actually work end to end.** In
`VectorizedParquetReader.FileIterator.next()`:
```java
int numValuesToRead = (int) Math.min(nextRowGroupStart - valuesRead,
batchSize);
...
valuesRead += numValuesToRead;
```
with `batchSize == 0` this is always `0`, `valuesRead` never advances and
`hasNext()` stays true, so
the iterator returns empty batches forever. The iterator uses its own
`batchSize` from `ReadConf`, so
the readers' `== 0` guard cannot rescue that; it only prevents zero-capacity
allocations inside the
readers.
**What the guard does buy is avoiding a much worse failure, and that is
where the ordering matters.**
`NullabilityHolder.isNullAt` indexes a raw array with no bounds check:
```java
public byte isNullAt(int index) {
return isNull[index];
}
```
and both `ColumnVector` (arrow) and `IcebergArrowColumnVector` (Spark) call
it through
`holder.nullabilityHolder()`. With the old ordering, a zero batch size
produced a `DEFAULT_BATCH_SIZE`
sized vector paired with a zero length holder, so the vector reports 5000
values while any null check
on it throws `ArrayIndexOutOfBoundsException`. Resolving the size first
keeps the two consistent.
So my read is: the guard is worth keeping as defence in depth, the
inconsistency was the real
problem, and the actual fix for a user passing 0 belongs at the entry
points. Two follow-ups I would
suggest, both out of scope of this pull request ( as I see it):
1. Reject a non-positive batch size in `Parquet.ReadBuilder.recordsPerBatch`
and the `ArrowReader`
constructor, since it can only produce an infinite stream of empty
batches. That would let the
`== 0` guards go away entirely.
2. `VectorizedArrowReader.setBatchSize` (the parent, line 136) resolves the
value into its own field
but passes the raw argument to
`vectorizedColumnIterator.setBatchSize(batchSize)`. Same
inconsistency, one level up. I left it alone because changing what the
column iterator receives is
a behaviour change unrelated to this leak.
--
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]
