zhangfengcdt commented on PR #6531: URL: https://github.com/apache/datafusion-comet/pull/6531#issuecomment-5954824593
> Thanks @zhangfengcdt. Declining these writes at the gate is a sensible fix until iceberg-rust distinguishes signed zeros. The rule reads `outputSpecId`, fails closed, names the field and type in the reason, and the signed-zero test proves the native writer does not engage and compares partition directories and a pruned read against iceberg-java. A few things I'd like addressed before merge. > > **1. Orphaned comment in `CometIcebergWriteActionSuite`.** The existing `// iceberg-java renders a float or double partition value ... (#5836)` comment now sits directly above the new signed-zero test, but it describes the path-rendering test. Could you move it down to `float and double partition paths match iceberg-java` so each test carries its own comment? > > **2. Something should track lifting the gate.** The PR closes #6138, but the native bug is still there and is only declined. Could you link [apache/iceberg-rust#3325](https://github.com/apache/iceberg-rust/issues/3325) in the comment on `requireNoFloatingPointPartitionField` and in the user guide paragraph? Could you also say which open issue tracks removing the rule once the pin carries the upstream fix? Keeping #6138 open, filing a short tracking issue, or linking #5643 would all work. I'd like something to stay open so the gate does not outlive the need for it. > > **3. Is the `zip` in `floatingPointPartitionFields` safe on every pinned Iceberg version?** It pairs `spec.fields()` with `spec.partitionType().fields()`, which is only right if the second list has exactly one entry per partition field. The contributor guide notes that a v1 spec keeps a dropped partition field as a `void` transform whose source column may later be dropped (#5691, #5693, #6141). Did the cross-version checks on 1.5.2, 1.8.1, 1.10.0 and 1.11.0 include that case? If any version omits such a field from `partitionType()`, the pairing would shift and a float field could be missed, which would fail open. Would you consider resolving each field's type from its `sourceId` and the spec's schema instead? A detection case with a dropped partition column plus a surviving float identity field would also settle it. > > **4. User guide wording on partition directory names.** The accepted-divergences bullet says directory names match iceberg-java "for every partition type" and describes how `float` and `double` values are rendered. Identity float and double writes can no longer reach the native writer, so that sentence describes a path users cannot hit. Could you reword it or point to the new fallback paragraph so it does not suggest native float partition writes exist? Thanks @andygrove for the review and I think the above comments should have been addressed now: 1. I moved the comments back to the individual test to match iceberg-java. 2. I linked the apache/iceberg-rust#3325 in the comment and the user guide so we can remove the rule when the upstream issue is resolved. I already have a fix PR https://github.com/apache/iceberg-rust/pull/3327 for tracking here. 3. I switched to resolve each field type from its sourceId and the spec's schema. 4. I added a detection case for a dropped partition field beside a live double field, including the dropped source column on 1.11+, and one for a dropped double field left as `void`. Also noted that iceberg-java cannot write the dropped-source case itself. 5. I reworded in the user guide so it covers only partition types that the native writer accepts. Also, merge the main to bugfix branch. -- 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]
