sunchao commented on code in PR #5407:
URL: https://github.com/apache/datafusion-comet/pull/5407#discussion_r3856188677
##########
native/core/src/parquet/cast_column.rs:
##########
@@ -176,6 +189,1091 @@ fn cast_timestamp_micros_to_millis_scalar(
ScalarValue::TimestampMillisecond(new_val, target_tz)
}
+fn normalize_variant_array(
+ array: &ArrayRef,
+ target_field: &FieldRef,
+) -> DataFusionResult<ArrayRef> {
+ let DataType::Struct(fields) = target_field.data_type() else {
+ return Err(DataFusionError::Execution(
+ "Variant extension field must use Struct storage".to_string(),
+ ));
+ };
+ if fields.len() != 2
+ || fields[0].name() != "value"
+ || fields[1].name() != "metadata"
+ || fields
+ .iter()
+ .any(|field| field.data_type() != &DataType::Binary)
+ {
+ return Err(DataFusionError::Execution(
+ "Variant output must contain Binary children [value,
metadata]".to_string(),
+ ));
+ }
+
+ let array = decode_variant_metadata_dictionary(array)?;
+ let array = widen_unsigned_variant_typed_value(&array)?;
Review Comment:
[P2] Normalize millisecond shredded timestamps before validation
Could we convert Spark-readable `TIMESTAMP(MILLIS, ...)` children before
constructing `VariantArray`, or retain Spark fallback for them? An ordinary
Parquet group read as `v VARIANT`, with `typed_value INT64
(TIMESTAMP(MILLIS,true))` and value `1704067200123`, returns `2024-01-01
00:00:00.123+00:00` on Spark 4.0.4 and 4.1.3. Arrow 58.4 restores
`Timestamp(Millisecond, Some("UTC"))`, however, and this normalizer throws
`Illegal shredded value type: Timestamp(ms, "UTC")`. The `false`/NTZ annotation
fails the same way; these files have no embedded `ARROW:schema`. Both Spark
reader modes and a MICROS control pass. The direct Variant scan gate admits
these files, but neither preprocessing step handles their timestamp units
before the constructor rejects them. Recursive millisecond-to-microsecond
normalization, preserving LTZ/NTZ, needs native-read coverage here.
##########
native/core/src/parquet/cast_column.rs:
##########
@@ -176,6 +189,1091 @@ fn cast_timestamp_micros_to_millis_scalar(
ScalarValue::TimestampMillisecond(new_val, target_tz)
}
+fn normalize_variant_array(
+ array: &ArrayRef,
+ target_field: &FieldRef,
+) -> DataFusionResult<ArrayRef> {
+ let DataType::Struct(fields) = target_field.data_type() else {
+ return Err(DataFusionError::Execution(
+ "Variant extension field must use Struct storage".to_string(),
+ ));
+ };
+ if fields.len() != 2
+ || fields[0].name() != "value"
+ || fields[1].name() != "metadata"
+ || fields
+ .iter()
+ .any(|field| field.data_type() != &DataType::Binary)
+ {
+ return Err(DataFusionError::Execution(
+ "Variant output must contain Binary children [value,
metadata]".to_string(),
+ ));
+ }
+
+ let array = decode_variant_metadata_dictionary(array)?;
+ let array = widen_unsigned_variant_typed_value(&array)?;
+ let variant = VariantArray::try_new(array.as_ref())?;
Review Comment:
[P2] Normalize restored fixed-size lists before VariantArray construction
Could we convert restored `FixedSizeList` typed containers to supported
`List` storage before this constructor, or keep these files on Spark? I
reproduced an Arrow-written file read with schema `v VARIANT`, containing an
ordinary physical Parquet LIST and `FixedSizeList<Struct<typed_value:Int64>,2>`
retained in `ARROW:schema`: Spark 4.0.4 and 4.1.3 return `[42,43]` with both
readers, but exact-head normalization throws `Illegal shredded value type:
FixedSizeList(...)`. The fixture uses signed Int64 children and no dictionary,
so the existing unsigned/dictionary fixes do not cover it. Casting only that
container to `List` makes the same normalizer return `[42,43]`. The new
`FixedSizeList` reconstruction branches below are unreachable because Arrow
58.4 rejects the container first; please cover this retained-schema
representation in a native-read regression.
--
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]
