peterxcli commented on PR #5177: URL: https://github.com/apache/datafusion-comet/pull/5177#issuecomment-5159927478
@andygrove thanks for the review! review change is pushed. please take another look, thanks! > 2. `div_euclid` is a behavior fix, not a refactor. Retitled the PR. The final patch rejects that invalid read-schema pair instead of using `div_euclid`. > 3. Refresh the PR description and mention overflow. Updated it to describe the planning rejection and that millis→micros overflow now errors instead of wrapping. > 4. Is micros→millis reachable from Spark or Iceberg? No. Spark read schemas use microsecond logical timestamps. Construction now returns `DataFusionError::Plan`, with a Spark source link. > 5. Keep scalar target-timezone coverage. The pair is now rejected before scalar or array evaluation. The planning test covers timezone-free and timezone-bearing fields. > 6. Use `DEFAULT_CAST_OPTIONS` at the third site. Done. The timezone-free `Date32 -> Timestamp` path now uses `DEFAULT_CAST_OPTIONS`. > 7. Explain why millis→micros always errors on overflow. Added links to Spark’s [Parquet call site](https://github.com/apache/spark/blob/v4.2.0/sql/core/src/main/java/org/apache/spark/sql/execution/datasources/parquet/ParquetVectorUpdaterFactory.java#L817-L833) and checked [`millisToMicros`](https://github.com/apache/spark/blob/v4.2.0/sql/api/src/main/scala/org/apache/spark/sql/catalyst/util/SparkDateTimeUtils.scala#L103-L108) implementation. -- 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]
