zhangfengcdt commented on PR #6531: URL: https://github.com/apache/datafusion-comet/pull/6531#issuecomment-5957011598
> Thanks for this. I went through it against `branch-1.1`. The signed-zero bug in the native writer is real in the release, and the fallback is narrow: it declines exactly the float and double identity case that plan time can see. It also fixes a wrong answer at a cost that is documented. A few things I'd like to see before it merges: > > 1. `spark/src/main/scala/org/apache/comet/serde/operator/CometIcebergNativeWrite.scala:276` > Thanks for linking [apache/iceberg-rust#3325](https://github.com/apache/iceberg-rust/issues/3325) here and in the user guide. The comment says #5643 > tracks removing this rule, but #5643's checklist doesn't mention it yet, and this PR closes > #6138. Could you add a line to #5643 for float and double identity partitions, marked as > pending [apache/iceberg-rust#3325](https://github.com/apache/iceberg-rust/issues/3325) and #3327? Then something stays open after #6138 closes. It > would also help if that line says the two `assertNativeWriteDoesNotEngage` calls in > `CometIcebergWriteActionSuite` should go back to `assertNativeWriteEngages` when the rule is > removed. > 2. `spark/src/test/scala/org/apache/comet/CometIcebergWriteDetectionSuite.scala:887` > Resolving through `sourceId` is a nice improvement. Because `Schema.findField` also indexes > nested fields, the rule should catch a float inside a struct as well. Would you add a detection > case for an identity partition on a nested field? For example `s STRUCT<v: DOUBLE>` with > `PARTITIONED BY (s.v)`, or `updateSpec().addField("s.v")` if the DDL route is awkward. That > pins the rule against failing open for nested sources. > 3. General comment on the Iceberg versions > The four-version check in the approving review ran against `5995d96`, before the switch to > `sourceId` and the new `icebergVersionAtLeast(1, 11)` branch, and PR CI only runs Iceberg 1.11. > Could you run `CometIcebergWriteDetectionSuite` and the two changed `CometIcebergWriteActionSuite` > tests under `-Pspark-3.4` and `-Pspark-3.5` (Iceberg 1.5.2 and 1.8.1) at the current head, and > post the result here? The v1 `removeField` cases and the INSERT into an all-`void` spec are the > parts that depend on the Iceberg version. > 4. General comment on a backport > 1.1.0 ships the native writer with this bug behind the opt-in flags, and the gate on > `branch-1.1` has the same shape and the same helpers. Should we backport this to `branch-1.1`? > If we do, the 1.1.x release notes could tell anyone who enabled the native writer that existing > tables with a float or double identity partition may already hold rows filed under the other > zero. Sure, I think we can do the first two items in this PR and the back port will be after the merging. For the nested fields, I added the detection case and ran the suites across versions. Detection suite plus the two changed action tests, clean build per profile: | Profile | Iceberg | Result | | --- | --- | --- | | `-Pspark-3.4 -Pscala-2.12` | 1.5.2 | 59 passed, 1 cancelled (the existing format-version=3 test) | | `-Pspark-3.5 -Pscala-2.13` | 1.8.1 | 60 passed | | `-Pspark-4.1` | 1.11.0 | 60 passed | The v1 `removeField` cases and the insert into an all-`void` spec pass on all three. The dropped-source-column stage runs only on 1.11, per its version check. I added a line to #5643 for this restriction in the comment (may need to be added to the description for clarity). For the backport: yes, I think it qualifies as a correctness fix, and I'm happy to open the `branch-1.1` backport once this merges. -- 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]
