dwsmith1983 commented on code in PR #5654:
URL: https://github.com/apache/datafusion-comet/pull/5654#discussion_r4095784630
##########
native/core/src/parquet/cast_column.rs:
##########
@@ -270,12 +284,22 @@ impl PhysicalExpr for CometCastColumnExpr {
let input_physical_field = self.input_physical_field.data_type();
let target_field = self.target_field.data_type();
+ // Relabeling only swaps metadata, so it is right when every requested
field reads
+ // the file field at its own position. A mapping that reorders fields
(ids resolved
+ // to other positions) has to go through the nested conversion below.
+ let positional = self
Review Comment:
> Could you add `Closes #6192` to the description, plus a ParquetReadSuite
test that runs the drop-and-re-add read through checkSparkAnswerAndOperator for
a struct, a list element and a map value?
Added `Closes #6192` to the description and three ParquetReadSuite tests,
`nested column dropped and re-added under the same name with a new field id:
struct`, `: list element` and `: map value`. They share one file, written with
field ids on, holding `struct<x (id 1), y (id 2)>` as a top level column, as a
list element and as a map value, and each reads its column with x carrying id 3
and y keeping id 2. Each runs through `checkSparkAnswerAndOperator`, asserts
`CometNativeScanExec` is in the plan, and pins the expected rows explicitly: x
is null and y keeps its values, including a null struct inside the list and the
map, empty containers, and a null column.
One thing the run turned up: Spark 3.5's own vectorized reader raises on the
list and map reads (`ParquetColumnVector` rejects the `_fake_name_` field that
`clipParquetSchema` generates for the unmatched id below a list or map), and
Spark 4.0.4 still has that check, so for those two the comparison with Spark
runs from 4.1 on and the pinned rows carry the check on 3.5 and 4.0. The struct
case compares with Spark on every version.
With the gate in `cast_column.rs` forced to always take the relabel
shortcut, all three fail with the old x values: the struct test returns
`[1,[1,10]]` and `[2,[5,50]]` where Spark returns `[1,[null,10]]` and
`[2,[null,50]]`, the list test returns `[2,20], [3,30]` and the map test `k ->
[4,40]` in place of nulls. With the gate as pushed all three pass, on Spark 3.5
and 4.1.3.
--
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]
