Re: [PR] Allow struct field access projections to be pushed down into scans [datafusion]

Wed, 28 Jan 2026 05:10:45 -0800 


adriangb commented on PR #19538:
URL: https://github.com/apache/datafusion/pull/19538#issuecomment-3811223705

   > > I've gotten the logical optimizer side of this working in #20036. It's 
not too bad, +700 LOC. But this is already a big PR and then we can look at the 
full picture and e.g. see if we can simplify the physical optimizer side. Wdyt 
of merging that into here @alamb ?
   > 
   > I am not quite sure what you are proposing? You mean merge #20036 into 
this PR?
   > 
   > #20036 seems to have more than 700 LOC <img alt="Screenshot 2026-01-28 at 
7 28 29 AM" width="234" height="63" 
src="https://private-user-images.githubusercontent.com/490673/541600956-07501426-72ae-4840-869b-d8ed35c10f34.png?jwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Njk2MDU5MzgsIm5iZiI6MTc2OTYwNTYzOCwicGF0aCI6Ii80OTA2NzMvNTQxNjAwOTU2LTA3NTAxNDI2LTcyYWUtNDg0MC04NjliLWQ4ZWQzNWMxMGYzNC5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjYwMTI4JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI2MDEyOFQxMzA3MThaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT1iZWMxMTBkNTY2ZDM1NTI2NDhjOGIzMzA2NWU2ZDRiNDljYjQxYmVkYmViNTZlODE0ZTE2MGMwNGY2MDEzMTZkJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.RvMSnqstbGK9QTyLfs4tLZXM5TOMh5gQJ02dpZX7wlg";>
   > 
   > I am sorry but I have a really hard time finding the bandwidth to review 
such large PRs in depth. I am really struggling (across all the 
arrow/datafusion projects)
   
   I meant 700 LOC more than this PR. But yes it’s a lot to review. I can split 
it up, but it will be harder to see the big picture. Eg if I sequence (1) Expr 
APIs, (2) logical optimizer and (3) physical optimizer they will each be ~500 
LOC each and easier to review, but are you okay making the Expr/PhysicalExpr 
change with the justification of “optimizer rules that use this coming later”? 
I can add unit tests, etc to show that those APIs work as intended.
   
   I’m also happy to tap some other folks to help review. I know there are a 
lot of people interested in this work.


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

Reply via email to