andygrove commented on PR #5025: URL: https://github.com/apache/datafusion-comet/pull/5025#issuecomment-5441526074
> **Note on this review:** this was generated by an LLM (Claude Code) at my request while I worked through a review backlog. I have not verified the individual findings myself. Please treat everything below as suggestions to evaluate rather than as authoritative review feedback, and push back on anything that is wrong or already handled. Collapsing five near-identical recursive type predicates into one is worth doing. That duplication is exactly why new types keep getting supported in one place and not another. That said, I do not think this is behavior-preserving, and for a refactor that gates shuffle eligibility that matters a lot. **Columnar shuffle now accepts `CalendarIntervalType`** The old `supportedSerializableDataType` for columnar shuffle listed: ```scala case _: BooleanType | ... | _: DateType | _: NullType => true ``` with no `CalendarIntervalType`. The new call is `supportedDataType(dt, allowComplex = true, allowDuplicateStructFieldNames = false)`, and `CalendarIntervalType` is in the unconditional base arm of `supportedDataType`, not behind `allowIntervals`. So calendar intervals are now eligible for columnar shuffle where they previously were not. Columnar shuffle converts unsafe rows to Arrow in `shuffle/row.rs`. Does that path handle `CalendarInterval`? If it does not, this turns a clean fallback into a runtime failure. If it does, then the change is fine but it is a functional change that should be called out and tested rather than arriving as a side effect of a refactor. **`CometLocalTableScanExec` no longer consults `super` for complex types** The old override was: ```scala case _: NullType | _: YearMonthIntervalType | _: DayTimeIntervalType => true case _ => super.isTypeSupported(dt, name, fallbackReasons) ``` so a `StructType` went to `DataTypeSupport.isTypeSupported`. The new version returns `true` from `supportedDataType(..., allowComplex = true, ...)` for any nested struct of supported leaves and never reaches `super`. Whatever `DataTypeSupport` was doing for complex types at that boundary, including any config gating and any `fallbackReasons` messages it appended, is now skipped. Is that intended? If it is, the comment should say that local scans deliberately no longer honour `DataTypeSupport`'s complex-type rules. There is a related side effect: when the new check returns `true`, nothing is appended to `fallbackReasons`, which is correct, but when it returns `false` and `super` also returns `false`, the reason now comes only from `super`. Worth confirming the messages users see are unchanged. **Six boolean parameters is a lot of surface** `supportedDataType(dt, allowComplex, allowIntervals, allowTimeType, allowAnyStringType, allowDuplicateStructFieldNames)` has defaults that pull in different directions: `allowComplex` and `allowIntervals` default to restrictive, while `allowTimeType`, `allowAnyStringType`, and `allowDuplicateStructFieldNames` default to permissive. A caller who forgets a flag gets stricter behavior in two cases and looser behavior in three, and at a call site `supportedDataType(dt, allowComplex = true, allowIntervals = true)` gives no hint that three other switches exist and are silently on. Would named per-boundary entry points be better? Something like `supportedForNativeShuffle(dt)`, `supportedForColumnarShuffle(dt)`, `supportedForSink(dt)`, each a one-liner over the core function. Then the boundary's policy lives in one named place, the call sites are unambiguous, and adding a sixth switch does not require auditing every caller. **The description should show the equivalence** For a refactor across shuffle, sinks, and local scans, "preserving their boundary-specific behavior" is a claim that needs evidence. Could the description include a small table, one row per call site, listing the old predicate's accepted set and the flags chosen to reproduce it? That is what a reviewer needs in order to check the claim, and it is how the `CalendarIntervalType` difference above would have been caught before review. **One process question** The description says this depends on #4976. Is that merged? If not, this should presumably wait, and it would be worth marking as such. -- 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]
