zhuqi-lucas commented on PR #16196: URL: https://github.com/apache/datafusion/pull/16196#issuecomment-2924675924
> @ozankabak > > > Pipeline-breaking operators creating cancellation issues is a universal problem -- this is not an `AggregateExec` issue. Therefore, I firmly believe that the solution has to be universal as well -- I don't agree with adding a `YieldStream` type construct to all such operators. > > From my perspective > > 1. this PR fixes a real bug that users (@pepijnve and we at InfluxData) have hit. > 2. Therfore, this PR better than what is on main and it is my opinion that we should merge it > > I fully admit there may be better solutions that are more universal. But until we have such a solution coded up, I don't see why we should block this PR. > > Therefore my alternate proposal is: > > 1. We add end to end tests showing that this PR enables query cancellation for aggregate queries > 2. We merge it in > 3. As time permits, we can explore alternate, more universal strategies for cancellation > > > IMO @berkaysynnada's suggestion to delegate this responsibility to sources is better than what is done here (more separation of concern, more maintainable), but I concede that it is not a perfect solution. Maybe we can find a universal solution akin to what @pepijnve suggested. > > @pepijnve suggested at least [one](https://github.com/apache/datafusion/pull/16196#issuecomment-2922223237) potential issue with adding yields to DataSources. Also, I don't think it will apply to queries like `SELECT SUM(value) FROM range(1, 50000000000);` (where there is no source > > > Let's not merge this until we all agree on a solution. Thanks. > > 100% agree with not merging this until we are in agreement Thank you @alamb , @ozankabak i totally agree even we use DataSources inject the yield or cancellation, it's also not unified solution. I have added the end to end tests showing that this PR enables query cancellation for aggregate queries. Next step, we can choose to hold on or merging it as the first step, create a follow-up to POC the cancellation way? -- 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
