andygrove commented on PR #5603: URL: https://github.com/apache/datafusion-comet/pull/5603#issuecomment-5981491575
Thanks for all the work on this, @RRXXZZYY, and for sticking with such a long review. You chased down every boundary a duplicate-name struct can cross, turned around each round of findings quickly and carefully, and the tests and probes along the way taught us a lot about where Java Arrow breaks on these structs. I'm going to close this PR, though, because main has since gone the other way. Instead of teaching Java Arrow to keep duplicate-name children, Comet now declines these structs wherever they would cross into Java Arrow: #5866 for shuffle and row conversion, #5766 for the codegen dispatcher, and #6004 for the cache serializer. The branch conflicts with all three. Running duplicate-name structs natively would need the positional allocation and the reader overrides from this PR, which is a lot of code on the per-batch import path for a shape that is rare in practice. The struct-writer leak that this PR also fixed is being fixed on its own in #6552. Your work here also pointed at what is left. Checking what to keep from this PR turned up three places where native code still builds duplicate-name structs that then fail at the JVM import: `arrays_zip` (#6251, with a fix in #6324), struct casts (#6591) and the opt-in native `from_json` (#6592). The cached-view panic from my earlier review no longer reproduces on main. Thanks again for the contribution! -- 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]
