comphead commented on PR #15418:
URL: https://github.com/apache/datafusion/pull/15418#issuecomment-2758423217
> Thank you @ctsk. I believe we can merge this as is. However I'd like to
raise one thing that comes to mind whenever I look this code. I'm not very
comfortable with adding a `CoalescePartitions` after calling `execute()`.
Modifying the plan post-execute() feels a bit off to me. Does it seem like a
smell to you as well?
🤔
```
let left =
coalesce_partitions_if_needed(Arc::clone(&self.left));
let left_stream = left.execute(0, Arc::clone(&context))?;
```
I think coalesce added before `execute`?
However just noticed the execute currently happens in main thread instead of
`future::once`. But it shouldn't be an issue and the stream is lazy evaluated?
WDYT @berkaysynnada
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
