peterxcli commented on PR #5177: URL: https://github.com/apache/datafusion-comet/pull/5177#issuecomment-5227372821
@andygrove thanks for another round of review, addressed all of your review. please take another look. TIA! > The overflow fix may not reach the actual Parquet reader. Moved the checked millis -> micros conversion into `parquet_convert_array`, the real Parquet reader path. It now uses `try_unary` with `mul_checked(1_000)`, so overflow returns an error instead of `null`. > Please verify the fix end to end against Spark behavior. Added a native Parquet scan regression covering TimestampType and TimestampNTZType, positive and negative overflow, dictionary on/off, and ANSI on/off. It verifies both Spark and Comet report overflow. > Is the millis→micros arm in `array_with_timezone` reachable? Confirmed it is unreachable for Parquet reads. Removed the dead arm, its imports, and its unit test. The conversion is now tested where it actually runs. > The millisecond-target rejection only checks top-level timestamps. Narrowed the comment to explicitly state that the guard applies to top-level timestamp columns, so it does not imply nested timestamp validation. -- 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]
