zhuqi-lucas commented on PR #16196: URL: https://github.com/apache/datafusion/pull/16196#issuecomment-2938168285
Thank you @pepijnve , since we have both POC for two ways: 1. Current PR unified solution, but one more exec (YieldStreamExec) exposed to customers. 2. Original PR, insert YieldStream to each stream which we need to handle, but no extra exec changes which will expose to customers. We can get more input from folks, we can continue the discussion here, thanks. cc @ozankabak @alamb @berkaysynnada > @zhuqi-lucas great work. I've continued playing around with alternative structures in the meantime, but I keep coming back to your `YieldStream` as the most elegant solution. It's seems like the simplest solution that works well for both the plain `Stream` implementations and the implementations that wrap an async function. > > Looking at the current PR makes me less enthusiastic about `YieldStreamExec`. Still feels like an implementation detail leaking out to me. I'm thinking it might be useful to go through the various `Stream` implementations in parallel with your work and update them the way you originally did for `AggregateExec`. That will let us really compare the two alternatives we've been discussing so far. I'm happy to give that a try. -- 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
