FelixYBW commented on PR #13280:
URL: https://github.com/apache/gluten/pull/13280#issuecomment-6309490290
Thanks for the review. I went through all 18 Copilot comments. They come
down to 7 distinct points. e10fa72c4 fixes 3 of them; the rest are false
positives or optional.
**Fixed in e10fa72c4**
1. **`ColumnarBatches.adopt` could move buffers out of vectors that are
still in use** (new in this PR). When a foreign batch's vectors share Gluten's
allocator root, `adopt` transfers their buffers instead of copying them. That
empties a vector for anyone else holding it, in two cases:
- A mixed batch: a Spark Arrow operator's output that passes Gluten
`RefCountedArrowColumnVector` columns through next to its own columns. The
pass-through columns may still be retained elsewhere.
- The same vector used in more than one column, e.g. `SELECT a, a`. The
second column reads the vector the first one just emptied.
Main doesn't have this problem only because its `offload` rejects foreign
and mixed batches outright. The fix copies such vectors and keeps the zero-copy
move for plain Spark vectors used once.
2. **`ArrowColumnVectors.fromRows` leaked the root when writing failed.** It
now closes the root before rethrowing.
3. **`RefCountedArrowColumnVector`'s reference count wasn't race-safe.** It
now uses compare-and-set, never goes below 0, and `retain()` after release
throws `IllegalStateException`.
**2 and 3 are also bugs in the current code on main**, not regressions from
this PR:
- `ArrowWritableColumnVector.retain()` / `close()` do the same non-atomic
check-then-decrement, with a non-volatile `closed` flag.
- `ColumnarPartialProjectExec` and `ColumnarPartialGenerateExec` allocate
with `ArrowWritableColumnVector.allocateColumns(...)` and don't release the
vectors if a projection throws. Only the task allocator's cleanup at task end
gets the memory back.
They're fixed here because the new code puts both in one place.
New tests in `ArrowColumnVectorTest`:
- `testOffloadSameRootVectorUsedTwice`
- `testOffloadMixedBatchKeepsGlutenColumn`
- `testFromRowsReleasesRootOnFailure`
- `testRefCount` now also checks that retain-after-release throws.
The two `adopt` tests fail on the previous commit with
`IndexOutOfBoundsException` (emptied vectors) and pass with the fix.
`ArrowColumnVectorTest` and `ColumnarBatchTest` pass locally (12/12, Spark 4.0
/ Scala 2.13 / JDK 17).
**False positives**
4. **"`setValueCount` is missing" in `ColumnarRangeExec` and `ExecUtil`, and
"`toBatch` should set value counts"** (5 comments).
`VectorSchemaRoot.setRowCount` already calls `setValueCount(rowCount)` on every
vector.
5. **"`ArrowWriter` is never closed"**. Spark's `ArrowWriter` has no
`close()`; its only methods are `write`, `finish` and `reset`.
**Optional, not changed**
6. **`adopt`'s copy path uses default sizing.** `VectorAppender` already
sets the value count. Pre-sizing the target would only reduce reallocations
when copying across allocator roots.
7. **`base("s")` in `ArrowEvalPythonExecSuite`** is a readability nit; the
test is correct as written.
--
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]
