sunchao commented on code in PR #7089:
URL: https://github.com/apache/datafusion-comet/pull/7089#discussion_r4315163262
##########
native/core/src/parquet/schema_adapter.rs:
##########
@@ -687,6 +690,21 @@ fn spark_has_updater(
return false;
}
+ // Spark has no updater that reads TIMESTAMP(NANOS) as a timestamp; it
reads it only as a
+ // long, under `spark.sql.legacy.parquet.nanosAsLong`. INT96 is coerced to
microseconds, so
+ // a nanosecond timestamp is TIMESTAMP(NANOS), except below a list, where
an INT96 leaf of a
+ // legacy list layout is left as a timezone-free nanosecond timestamp
(#7029). There only
+ // the UTC-adjusted form is known to be TIMESTAMP(NANOS). See #7034.
+ if let (
+ DataType::Timestamp(TimeUnit::Nanosecond, physical_tz),
+ DataType::Timestamp(TimeUnit::Microsecond, _),
+ ) = (physical_type, target_type)
+ {
+ if physical_tz.is_some() || !in_list {
Review Comment:
[P2] A timezone-bearing nanosecond Arrow type can still originate from
INT96, including outside a list. PyArrow can write `timestamp('ns', tz='UTC')`
using `use_deprecated_int96_timestamps=True` while retaining that type in
`ARROW:schema`. Arrow-rs restores `Timestamp(Nanosecond, Some("UTC"))`, and
DataFusion 55.2.0’s `Int96Coercer` leaves it unchanged because its matching arm
requires `Timestamp(Nanosecond, None)`. This condition consequently rejects a
valid INT96 file even without filters, where Spark and the previous adapter
accept it. Distinguish the actual Parquet physical/logical encoding, or ensure
timezone-bearing INT96 leaves are normalized before applying this rule.
Evidence: Reproduced with PyArrow 25.0.1 using
`pq.write_table(pa.table({'t': pa.array([86400001000000],
type=pa.timestamp('ns', tz='UTC'))}), path,
use_deprecated_int96_timestamps=True)`. The footer reports physical `INT96`
with no logical timestamp annotation. The native probe confirmed Arrow type
`Timestamp(Nanosecond, Some("UTC"))` both before and after
production-configured `Int96Coercer`, then failed the scan with
`ParquetSchemaConvert { column: "[t]", physical_type: "INT64", spark_type:
"timestamp" }`. Spark 3.5.9 returned `1970-01-02 00:00:00.001`. The base
implementation accepts this type pair. Native evidence is in
`/tmp/pr7089-737d-review/slim-tests.log`, test
`review_int96_with_arrow_timezone`.
##########
native/core/src/parquet/schema_adapter.rs:
##########
@@ -687,6 +690,21 @@ fn spark_has_updater(
return false;
}
+ // Spark has no updater that reads TIMESTAMP(NANOS) as a timestamp; it
reads it only as a
+ // long, under `spark.sql.legacy.parquet.nanosAsLong`. INT96 is coerced to
microseconds, so
+ // a nanosecond timestamp is TIMESTAMP(NANOS), except below a list, where
an INT96 leaf of a
+ // legacy list layout is left as a timezone-free nanosecond timestamp
(#7029). There only
+ // the UTC-adjusted form is known to be TIMESTAMP(NANOS). See #7034.
+ if let (
+ DataType::Timestamp(TimeUnit::Nanosecond, physical_tz),
Review Comment:
[P2] Preserve INT96 during raw-footer validation. With
`spark.comet.parquet.rowFilterPushdown.enabled=true`, reading an ordinary INT96
timestamp alongside a pushed predicate such as `id > 0` now fails with
`Expected: timestamp, Found: INT64`. `EagerPageIndexReader::get_metadata` calls
`check_file_conversions` on `parquet_to_arrow_schema` output before DataFusion
performs INT96 coercion. A scalar INT96 column therefore reaches this new
branch as `Timestamp(Nanosecond, None)` and is rejected by `!in_list`. Spark
reads the row successfully, and disabling row-filter pushdown restores the
native read. Apply the same INT96 normalization before the footer check, or
carry the actual physical encoding into this decision, and add a filtered INT96
regression test.
Evidence: A disposable native scan test wrote `message spark_schema {
REQUIRED INT32 id; OPTIONAL INT96 t; }` with `id=1` and INT96 words `(1000000,
0, 2440589)`. It requested `t: Timestamp(Microsecond, UTC)`, used predicate `id
> 0`, and configured production `coerce_int96=us` and `coerce_int96_tz=UTC`.
With row filters disabled it returned `1970-01-02T00:00:00.001+00:00`. With row
filters enabled it failed: `Parquet column cannot be converted in file .
Column: [[t]], Expected: timestamp, Found: INT64`. Spark 3.5.9 read the
identical filtered fixture successfully. The base implementation accepts this
type pair. Native evidence is in `/tmp/pr7089-737d-review/slim-tests.log`, test
`review_int96_with_row_filter`.
--
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]
