andygrove commented on PR #4715: URL: https://github.com/apache/datafusion-comet/pull/4715#issuecomment-5441533093
> **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. The expression wiring itself is straightforward and the SQL fixture covers a good range of multiplier types. Three things concern me, and two of them are about changes that are not in the description. **Removing `CalendarIntervalType` from `QueryPlanSerde.supportedDataType`** ```scala - _: DecimalType | _: DateType | _: BooleanType | _: NullType | CalendarIntervalType => + _: DecimalType | _: DateType | _: BooleanType | _: NullType => ``` `supportedDataType` gates sinks, native and columnar shuffle eligibility, local table scans, and literal support. Dropping `CalendarIntervalType` from it narrows all of those at once, and none of that is mentioned in the "What changes are included" list. The comment in `literals.scala` explains the intent, but the blast radius deserves its own discussion and its own tests. What breaks today that this is fixing, and what did you check does not regress? This also collides with #5025, which is refactoring these predicates and relies on `CalendarIntervalType` staying in that base arm. Whichever lands first will make the other's behavior different from what its author expects. Worth coordinating with that PR. **`supportedSinkDataType` now ORs two different predicates** ```scala case _ => supportedDataType(dt) || isTypeSupported(dt, "", ListBuffer.empty) ``` Passing an empty name and a throwaway `ListBuffer` means any fallback reason `isTypeSupported` produces is discarded, so a user who does hit an unsupported type at a sink gets a less specific message. Mixing `DataTypeSupport` into `CometSink` also pulls that trait's config-dependent behavior into sink eligibility, which is a wider change than "add year-month intervals". Could the sink predicate stay a single, explicit function? **Duplicate work with #5161** Both this PR and #5161 add the same two arms to `planner.rs`: `Interval(YearMonth)` in the null-literal match and in the `IntVal` match, with almost the same error message. They will conflict on merge and one of them will have to be rebased away. Since #5161 is about scan support and this one is about an expression, it might be cleanest to land the shared `planner.rs` and `literals.scala` interval plumbing in one place and rebase the other on top. **Two smaller notes** `classOf[MultiplyYMInterval]` goes in between `MonthsBetween` and `Minute` in the expression map. That map is broadly alphabetical, so it belongs after `MonthsBetween` but the `Minute` entry that follows is already out of order too. Worth fixing both while you are there. The description lists only `-Pspark-3.5` runs. The codegen dispatcher's `IntervalYearVector` handling is Comet's own per-version code and Comet supports 3.4 through 4.1+. Did the fixture pass on 3.4 and 4.1 as well? -- 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]
