andygrove commented on PR #4901: URL: https://github.com/apache/datafusion-comet/pull/4901#issuecomment-5441445293
> **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. This is a clean, small change and routing through the codegen dispatcher is clearly the right choice for interval division, where the rounding and overflow rules are fiddly enough that reimplementing them natively would be asking for divergence. The SQL fixture covers a good spread: all seven divisor types, literals on both sides, half-up microsecond rounding, and the `Long.MinValue / -1` case. A few things. **Map ordering in `QueryPlanSerde`** `classOf[DivideDTInterval] -> CometDivideDTInterval` is inserted between `DateDiff` and `DateFormatClass`. The surrounding entries are alphabetical (`DateAdd`, `DateDiff`, `DateFormatClass`, `DateFromUnixDate`, `Days`, ...), so this one is out of place. It belongs after `Days`. Small thing, but that map is long enough that keeping it sorted is the only way anyone finds anything in it. **Decimal divisor coverage stops at `DECIMAL(10, 2)`** The fixture uses a `decimal(10,2)` column and a `DECIMAL(10, 2)` literal. Spark's `DivideDTInterval` accepts any `NumericType` divisor, so `DECIMAL(38, 18)` and `DECIMAL(38, 0)` are both legal. Does the codegen dispatcher handle those, or is there an input-type limit that would make them fall back? A couple of rows with a wide decimal divisor would answer that, and if they do fall back, that is worth a line in `expressions.md`. **Testing on one Spark version** The description says the suite was run with `-Pspark-3.5`. `DivideDTInterval` and `DayTimeIntervalType` exist from 3.2 onward so I would expect this to be fine everywhere, but the codegen dispatcher's `DurationVector` handling is Comet's own code and Comet supports 3.4 through 4.1+. Did you run the fixture against 3.4 and 4.1 as well? If not it is worth doing before merge, since a per-version gap in the dispatcher's type handling would only show up in the `[expressions]` CI jobs. **Is there a performance number?** The rationale is compatibility rather than speed, which is fine. But the change moves `DivideDTInterval` from "falls back to Spark" to "runs through the dispatcher", and the dispatcher has its own per-batch cost. Do you know whether this is actually faster than the fallback for a realistic batch, or is the win purely that the surrounding operator stays native? If it is the latter, saying so in the description would set the right expectation. -- 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]
