avantgardnerio commented on PR #19285: URL: https://github.com/apache/datafusion/pull/19285#issuecomment-3719217963
> @haohuaijin > > > we can't support topk_aggregate for string aggregate func type before, so maybe you need add a new benchmark > > Thanks for your review and nudge. I added a new benchmark. How does the new benchmark compare in performance between: 1. running with this topk optimization enabled 2. running without ? The original justification for merging the primitive version of this rule was that it was equally as fast with it enabled or disabled, but when enabled memory was constant. If that remains true for strings, I think we can merge. If it is constant memory but slower for strings, I think we need to look at heuristics for when to enable vs not. -- 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]
