Re: [PR] Implement special `GroupColumn` support for byte view [datafusion]

[via GitHub]

Rachelint commented on PR #12809:
URL: https://github.com/apache/datafusion/pull/12809#issuecomment-2408987937

   > Thank you so much @Rachelint -- this looks so great. I found it well 
commented, well structured, and well tested.
   > 
   > cc @jayzhan211 your GroupColumn pattern is really working well
   > 
   > There are two test cases I think we need to cover (below), but otherwise I 
think this PR is good to go.
   > 
   > I am also testing this PR with some other things to see if we can get the 
string view code enabled by default (finally): #12092
   > 
   > I also ran test coverage of this PR like this:
   > 
   > ```shell
   > cargo llvm-cov --html  -p datafusion-physical-plan --lib
   > ```
   > 
   > Here is the report: 
[coverage.zip](https://github.com/user-attachments/files/17355629/coverage.zip)
   > 
   > In general very nice job with coverage. There are a few items that appear 
to be untested:
   > <img alt="Screenshot 2024-10-13 at 8 46 38 AM" width="1200" 
src="https://private-user-images.githubusercontent.com/490673/376035489-5266a941-f661-4547-825e-8a7a17a4bede.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mjg4MjU0MjAsIm5iZiI6MTcyODgyNTEyMCwicGF0aCI6Ii80OTA2NzMvMzc2MDM1NDg5LTUyNjZhOTQxLWY2NjEtNDU0Ny04MjVlLThhN2ExN2E0YmVkZS5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjQxMDEzJTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI0MTAxM1QxMzEyMDBaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT1hYjA3MDVmNDY0ODZiODNjNmQ2Zjc0ZjM4NTVhNjUyM2ViMjg1YWZjNmRjNTZmODk3OTY5NDljY2RkZjcwZjdlJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.1syGvm17IDTIEhKRMu37Vbzb6WHp5xVZeikYbsk0Ztk";>
 <img alt="Screenshot 2024-10-13 at 8 47 09 AM" width="1109" 
src="https://private-user-images.githubusercontent.com/490673/376035494-196b9850-7230-4d
 
d1-918d-eedb9690e8c6.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mjg4MjU0MjAsIm5iZiI6MTcyODgyNTEyMCwicGF0aCI6Ii80OTA2NzMvMzc2MDM1NDk0LTE5NmI5ODUwLTcyMzAtNGRkMS05MThkLWVlZGI5NjkwZThjNi5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjQxMDEzJTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI0MTAxM1QxMzEyMDBaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT0wY2I2NTMzMzVmOTU4NzA2MzFhNTdjZjE1MTM5MDQ4OWVmZDdkOWQwMDExNmJhOThjZjJiYTJlN2UxMzYwYWIxJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.AUGZo-hDzO1UWpxEJwzO4L8vOR9itcdUrGRhoHYG6j4">
   > 
   > I also think it would be great to add some additional testing in fhe form 
of aggregate fuzz testing (mostly for the `take_n` logic). I have some ideas 
(in #12847) that I hope to refine tomorrow
   
   Comments about readability improvement are fixed.
   
   I am adding test for better test coverage.


-- 
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