peterxcli commented on PR #5025: URL: https://github.com/apache/datafusion-comet/pull/5025#issuecomment-5444049168
Thanks for the detailed pass @andygrove — one of the findings was real. **Columnar shuffle + `CalendarIntervalType`: confirmed and fixed.** The old columnar predicate rejected it and the refactor accidentally accepted it via the unconditional base arm. The native unsafe-row-to-Arrow converter (`native/shuffle/src/spark_unsafe/row.rs`) has no interval support, so this would have surfaced as a native `unreachable!` panic instead of a clean Spark fallback. Fixed by adding an `allowCalendarInterval` capability (default `true` — every other boundary and the default expression-side callers accepted it pre-refactor), with the columnar shuffle call site passing `false`, plus unit-test coverage for the top-level and nested cases. **Local scan and `super`:** the code is `if (supported) true else super.isTypeSupported(...)`, so `super` *is* consulted on every rejection and its fallback messages are unchanged. When the new predicate accepts, `super` would also have accepted and appends nothing — and `DataTypeSupport.isTypeSupported` has no config gating for complex types, so nothing is skipped. The one behavioral delta is intended: intervals nested inside structs/arrays/maps are now accepted, which is what this stack (with #4976) adds. I added a comment making the super call's reason-recording role explicit. **Flag surface:** the defaults reproduce the pre-refactor shared `supportedDataType` exactly, which is why they point in different directions — restrictive flags opt boundaries *into* more (complex, intervals), permissive ones let boundaries opt *out* of what the legacy predicate already accepted (time types, collations, duplicate field names). Flipping everything restrictive would mean touching every legacy expression-side caller. All boundary call sites use named arguments and each boundary has exactly one call site, so the per-boundary policy already lives in one named place; happy to add named wrappers (`supportedForColumnarShuffle` etc.) as a follow-up if you'd prefer that shape. **Description:** updated with a per-call-site equivalence table showing the old accepted set, the flags chosen, and the deltas. **#4976** merged on July 28 and this branch has since been updated from main; the `DurationVector` duplication is gone. -- 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]
