pepijnve commented on code in PR #23407:
URL: https://github.com/apache/datafusion/pull/23407#discussion_r3566211404
##########
datafusion/physical-plan/src/sorts/merge.rs:
##########
@@ -171,24 +152,40 @@ impl<C: CursorValues> SortPreservingMergeStream<C> {
in_progress: BatchBuilder::new(schema, stream_count, batch_size,
reservation),
streams,
metrics,
- done: false,
- drain_in_progress_on_done: false,
cursors: (0..stream_count).map(|_| None).collect(),
prev_cursors: (0..stream_count).map(|_| None).collect(),
round_robin_tie_breaker_mode: false,
num_of_polled_with_same_value: vec![0; stream_count],
current_reset_epoch: 0,
poll_reset_epochs: vec![0; stream_count],
loser_tree: vec![],
- loser_tree_adjusted: false,
batch_size,
fetch,
produced: 0,
- uninitiated_partitions: (0..stream_count).collect(),
enable_round_robin_tie_breaker,
}
}
+ pub(crate) fn into_stream(self) -> SendableRecordBatchStream
+ where
+ C: 'static,
+ {
+ let schema_clone = Arc::clone(self.in_progress.schema());
+
+ let cloned_metrics = self.metrics.clone();
+
+ let mut this = self;
+ let stream = Gen::new(|co| async move {
Review Comment:
> > [...] across is that this looks much nicer imo at the call site in
SortPreservingMergeStream
>
>
>
> The only difference I see is no need to do the `if let Err(...` the rest
looks the same
It's in the eye of the beholder I guess. It is indeed essentially the same,
but some incidental complexity is being removed. The `try_stream` basically
encodes the pattern where you only emit the stream's value type. The fact that
error results are terminal for the stream is enforced since you can't emit an
`Err`. That restriction is a good thing imo since it leaves less room for
mistakes.
Reading the `Gen`/`Co` based code at first I was a bit puzzled by what this
was trying to do and why it was written like this. It made sense after reading
the genawaiter documentation.
Might be a skill issue on my part, but I think there's benefit to avoiding
whatever bits of clutter we can.
That's just my take. Best to get more opinions on this one. Maybe I'm just
bikeshedding again.
> This is possible, but aside from not adding a dependency I don't see much
value, we can always start with the dep and move to in house impl (or vice
versa) but we can first see if the dependency is enough for us and not needing
to add more maintenance burden
As a consumer of DataFusion "just an extra dependency" is more of a nuisance
than you might think. It's probably fine for this crate (I don't expect any
vulnerabilities from something like genawaiter), but know that the burden of
tracking all these dependencies, dealing with the barrage of vulnerabilities in
the transitive closure of the dependency graph, and triaging them is a
non-trivial burden on a small team. It's especially bad in the Rust ecosystem
(kind of similar to JS) where small libraries are the norm.
Again something where some extra opinions would be useful. Not sure how
these decisions get made.
@alamb you've voiced resistance against dependencies and complained about
the dependency graph recently. What's your take?
--
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]
