peterxcli commented on PR #5292: URL: https://github.com/apache/datafusion-comet/pull/5292#issuecomment-5294043499
@andygrove Thanks for the detailed review. I addressed each point: > Could `make_interval` return `"integer"` or `"long"` so `arithmetic_overflow_error` gets the same string Spark produces? Changed the helper to return `"integer"` for month/day arithmetic overflow and `"long"` for microsecond arithmetic overflow. ANSI execution forwards that label to `arithmetic_overflow_error`, and native unit tests pin both values. I kept the SQL assertion as `overflow. If necessary set` because [Spark 4.1.3 preserves `integer overflow`](https://github.com/apache/spark/blob/v4.1.3/sql/core/src/test/scala/org/apache/spark/sql/errors/QueryExecutionErrorsSuite.scala#L704-L713), while [SPARK-55714 canonicalizes it to `overflow` in Spark 4.2](https://github.com/apache/spark/blob/v4.2.0/sql/api/src/main/scala/org/apache/spark/sql/errors/ExecutionErrors.scala#L118-L144). This is documented beside the assertion; the native unit test provides the tighter check on Comet’s returned type. > Would it be safer to make the `CalendarIntervalType` arm throw with a message pointing callers at `toArrowField`? Changed `toArrowType` to throw an `UnsupportedOperationException` directing callers to `toArrowField`, and added unit-test coverage. > Could the `_dispatch` SQL fixtures either be removed or repurposed? Removed both dispatch fixtures. With `MakeInterval` fully native, they no longer exercised a distinct path. > Matching `SparkMakeDate`’s downcast style would give a diagnostic instead of a panic. Replaced all seven argument-downcast `.unwrap()` calls with descriptive `DataFusionError::Execution` errors. > It might be worth adding a mirror row covering the negative boundaries. Added a row reaching `i32::MIN` exactly for both computed months and days while also exercising negative hours, minutes, and seconds near the microsecond boundary. > A chained native consumer would pin the metadata-preservation invariant. Added an end-to-end query that constructs `ARRAY<INTERVAL>` using native `make_interval` below a native shuffle and consumes `intervals[0]` using native `GetArrayItem` above it. This pins the `CalendarInterval` metadata across both native-expression and shuffle boundaries. -- 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]
