andygrove commented on PR #6773:
URL:
https://github.com/apache/datafusion-comet/pull/6773#issuecomment-6090347528
Addressed in c29bb0a4c7:
1. Done: `FanoutPartition::write` takes the writer from `Option::insert`, so
the `expect` is gone.
2. Added the debug assertion, and `child()`'s doc now says only the task's
total has children. I
didn't split the type. The unpartitioned and clustered writers' files
report straight to the
task's total, so the total needs `share()` as much as a partition's
counter does, and
`MeteredParquetWriterBuilder` would have to take either type, for a rule
with one caller.
3. Only the push does the before-and-after diff. The release sets the total
to zero once every feed
has released, and `close_early` subtracts the partition's held rows
before its close releases
them. So `sync_held` would have one caller. It also takes `&mut self`
while the loop after it
still writes through `partition`, so that loop would have to look the
partition up again.
Instead, the doc comment on `held_bytes` now states the invariant and
names the three places
that keep it. A `debug_assert_eq!` at the end of `write` checks the total
against the feeds,
so a fourth place that misses it fails the tests: with `close_early`'s
subtraction removed,
`a_fanout_write_short_of_memory_closes_the_partitions_holding_the_most`
fails on it.
4. Added to `by_memory`'s doc.
5. Done: `payload_rows_from` takes the `Vec<&str>`, as `payload_batch_from`
did before this PR.
--
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]
