hhhizzz commented on PR #10288: URL: https://github.com/apache/arrow-rs/pull/10288#issuecomment-5015953111
> Thank you @hhhizzz -- I spent quite a while studying this PR and I found it very well structured and a very clever design > > In order to merge it, I think we need a few more tests to cover some edge cases (I left specific comments) > > I also think it would help a lot to document some of the very clean, though complex design of `RowSelectionCursor` and `MaskCursor` (I left some specific suggestions inline as well), though I think we could do this as a follow onn Thanks for the careful review. I have addressed the coverage and readability points locally: the two unreachable branches are gone, the trailing-skip invariant now has dedicated tests, the Mask path has been extracted from `next_inner`, and `MaskCursor` now includes a source-level walkthrough. I also reran the suggested llvm-cov command. The full parquet test suite passed, and the newly extracted cursor and accumulator paths are covered. -- 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]
