nssalian commented on PR #16665: URL: https://github.com/apache/iceberg/pull/16665#issuecomment-4772389898
Nice work reviving this. I traced the Spark side to double-check the conversions and they line up: `TimeType` is stored internally as nanoseconds-of-day with `LocalTime` as the external type, so the `x1000` / `/1000` boundaries and the `SparkValueConverter` cast are all correct. The Parquet-vs-ORC batch-read asymmetry is also fine, since non-primitive types never reach the Parquet vectorized path anyway, while ORC's does support nested types. One gap on testing. The read paths and the ORC write round-trip are covered, but the Spark-write conversions for Parquet (`TimeMicrosWriter`) and Avro (`time-micros`) aren't exercised by any test, and `TestSparkValueConverter` has no TIME case (it only covers null conversion). A `x1000` / `/1000` slip on the write side would be caught for ORC but slip through for Parquet and Avro. Could you: 1. Add a `time` column to `TestSparkParquetWriter`'s `COMPLEX_SCHEMA`? It already round-trips Spark `InternalRow` -> file -> `InternalRow` via `RandomData.generateSpark`, and `RandomData` now handles time, so this would exercise `TimeMicrosWriter` for free. 2. Add a small TIME round-trip to `TestSparkValueConverter`: `convert(schema, rowWithLocalTime)` then `convertToSpark(...)`, and assert the value survives. 3. For Avro, either add a Spark-write round-trip or confirm `SparkAvroWriter`'s `time-micros` path is reached by an existing end-to-end DataFrame write test. Happy to review once these are added. -- 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]
