Benjamin0313 commented on PR #16665: URL: https://github.com/apache/iceberg/pull/16665#issuecomment-4911823317
@manuzhang Thanks for the thorough review — all three gaps are fixed, plus two more I found while auditing for the same class of issue. **Your three findings** (fixed in 8081b7501, tests in 14d438d67): 1. **Schema pruning**: added `TIME → TimeType` to `PruneColumnsWithoutReordering`'s type map, so scans selecting a time column no longer fail. Note `TimeType` is a parameterized case class, so it is matched by `TimeType.class` like `DecimalType`. Test: `TestSparkSchemaUtil.testPruneColumnsWithTimeField`. 2. **Filter pushdown**: `SparkV2Filters.convertLiteral` now converts time literals from Spark nanoseconds to Iceberg microseconds (all 16 predicate call sites go through it). Test: `TestSparkV2Filters.testTimeFilterConversion`. 3. **StructInternalRow**: `getLong`, `get(ordinal, dataType)`, and `collectionToArrayData` now convert time values to the nanoseconds Spark expects, covering both the `Long` micros and `LocalTime` representations. Tests: `TestStructInternalRowTime` (4 cases, covering the data-task / local-scan / aggregate-pushdown paths). **Two more gaps found and fixed in the same audit:** 4. **`SparkUtil.internalToSpark` had no TIME case**, so values served from partition metadata — identity-partitioned time columns, the `_partition` metadata column (via the recursive STRUCT case), and column defaults — were exposed as microseconds where Spark expects nanoseconds (1000x too small). Verified end-to-end: without the fix, `SELECT t` from a table `PARTITIONED BY (t)` returns `00:00:37.230123456` instead of `10:20:30.123456`. Fixed with unit tests (`TestSparkUtil`) and SQL round-trip tests on a time-partitioned table (`TestTimePartitionedTable`). 5. **The Parquet batch-read guard only rejected top-level time fields**, so a time field nested in the `_partition` struct could still reach the vectorized path via `MetadataColumns.isMetadataColumn`. It now uses `TypeUtil.find` to check nested types, matching the ORC guard. One note for reviewers: Spark 4.1 gates the TIME data type in SQL behind the internal flag `spark.sql.timeType.enabled` (default off outside Spark's own tests), so the new SQL tests enable it explicitly. The existing format-level tests never hit the analyzer, which is why this wasn't visible earlier. -- 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]
