kumarUjjawal commented on PR #22571: URL: https://github.com/apache/datafusion/pull/22571#issuecomment-4560787640
> Looks good to me overall @kumarUjjawal -- i had a few questions > > I see you say > > > The heap still only stores non-NULL values. This avoids making the TopK heap implementations handle NULL values directly. > > I don't have any sense of how complicated this would be / not be -- did you try it? > > Also, perhaps @avantgardnerio you can help review this PR as I think you are familiar with the original logic > I don't have any sense of how complicated this would be / not be -- did you try it? I did try that as my first approach but I find it broad and since the only question I needed to answer was did we see a NULL group key and since SQL DISTINCT has only one NULL group, a boolean seemed good choice. Let me know what you think, I can go back to the other approach. -- 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]
