viirya commented on PR #59582:
URL: https://github.com/apache/spark/pull/59582#issuecomment-6616652366
@dongjoon-hyun, thank you for the detailed review. I checked the items
against the code and agree with all of them, and cfce388a0f5 follows your
suggestions. @HyukjinKwon, this replaces the options in my question with the
API below, so I'd like your opinion on it.
**Opt-in instead of the internal marker (items 2 and 4)**
As you suggest, the polarity is flipped: a source that follows the public
contract of `ColumnVector` cannot know an internal marker. `ReusedColumnVector`
is gone, and `ColumnVector` has a public method, false by default, which a
producer overrides to let a consumer hold the vector:
```java
public boolean isRetainable() {
return false;
}
```
Its doc says what returning true promises: the producer neither overwrites
nor frees the vector when the next batch is requested or when it is closed,
`closeIfFreeable` frees it and may be called from another thread, and the
producer frees what the consumer has not released when the task completes. It
also says that a consumer that holds such vectors does not account for their
memory (item 3), and that it applies to the top-level columns. Path 1 is taken
only if every column of the first batch is a retainable `ArrowColumnVector`,
and holds a later batch only if all its pass-through columns are retainable,
copying them otherwise, so the two checks agree. Nothing in the tree returns
true, so `ArrowBackedDataSourceV2` opts in for its fresh vectors, and a new
test covers the same reader with an opt-out.
I chose a method on `ColumnVector` over a public marker interface, since
`closeIfFreeable` already works this way and a wrapper can delegate it, and
over a capability of the DSv2 reader factory, which would not reach the cache
or other columnar operators and would need to pass through `UnionExec` and
`QueryStageExec`, as you noted for `vectorTypes`. The name is open.
**Arrow cache (items 1, 3 and 15)**
`ArrowCachedBatchSerializer` is back to master: it closes the previous root
in `next()`, and its plain `ArrowColumnVector`s are not retainable, so cache +
UDF takes the row queue path. The cache test asserts that path, and uses
`ARROW_EXECUTION_MAX_RECORDS_PER_BATCH` to get 200 batches (item 14; my earlier
comment was wrong about the batches).
**The default (items 5 to 11)**
With nothing in the tree opting in, enabling columnar input by default would
bring Path 1 to no built-in source, while items 5 to 11 apply to the paths that
remain. So this PR no longer changes the default: it stays `false` on master,
and the 4.4 entry of the migration guide is back, with a sentence on
`isRetainable`. Items 5 to 9 also happen with the default configuration of 4.2
and 4.3, so I'll file them as bugs and fix them separately.
**Smaller items**
Input batches without rows are now released and skipped on all columnar
paths (item 12), and `isArrow` is now `canHoldInput`, with the comments of
paths 2 and 3 updated (item 16). Item 13 is covered by the doc of
`isRetainable`, which leaves the release of unreleased vectors at task
completion to the producer.
--
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]
