ctsk commented on PR #15757: URL: https://github.com/apache/datafusion/pull/15757#issuecomment-2817276675
The implementation looks good to me. Two points: 1. The testing seems quite verbose - I briefly browsed around and think `DFSchema::matches_arrow_schema` can help you shorten it. 2. There are might be some more places where this is applicable: https://github.com/apache/datafusion/blob/d1cd58ce4173476fe15072213000bd0a4e258534/datafusion/expr/src/logical_plan/plan.rs#L663-L683 https://github.com/apache/datafusion/blob/d1cd58ce4173476fe15072213000bd0a4e258534/datafusion/expr/src/logical_plan/plan.rs#L3771-L3783 https://github.com/apache/datafusion/blob/d1cd58ce4173476fe15072213000bd0a4e258534/datafusion/optimizer/src/eliminate_cross_join.rs#L317-L332 https://github.com/apache/datafusion/blob/d1cd58ce4173476fe15072213000bd0a4e258534/datafusion/optimizer/src/eliminate_cross_join.rs#L339-L354 Could be also applied here, but would change the behaviour in the error case - I don't know if that matters. https://github.com/apache/datafusion/blob/d1cd58ce4173476fe15072213000bd0a4e258534/datafusion/expr/src/logical_plan/builder.rs#L1082-L1099 Note that all of these are mere suggestions, I am not very familiar with this part of datafusion and apologize if any of them are wrong. Cheers! -- 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
