brgr-s commented on PR #2961:
URL: https://github.com/apache/iceberg-rust/pull/2961#issuecomment-5205187279
@mbutrovich I worked through your comments and will follow up with commits
adressing where we agree. Specifically, because it is not inlined: the test gap
is real, so more tests!
I just have three comments:
float/double equality-delete columns: `Datum` does not use IEE-754 float
semantics. `PrimitiveLiteral` stores `OrderedFloat<..>`, which treats `NaN ==
NaN` and `Hash` uses `CANONICAL_NAN_BITS` and `canonicalize_signed_zero`, so
both agree. However, this does introduce a change: on main, eq-deletes go
through arrow's kernel where `is_eq` is bitwise, so `-0.0 != 0.0` and different
NaN bits also do not match. I feel like the spec is not really clear on what is
correct... but at the end of the day, it only applies to input that the spec
dissallows and would only hit non-conforming writes. But the old implemenation
also already did not `Err` on this, so we'd introduce a regression for those
non-conforming writer. I'd opt for the test and against the error.
`if set.is_empty() { continue; }` untested: correct, but a test is limited.
If `continue` is removed, you get `num_cols = 0`, empty `keys`, and `contains`
is always `false`, so every row is kept for that pass. It is performance guard,
not a correctness guard. “doesn’t affect row-group/row-selection filtering” is
true by construction: after this PR only `task.predicat` drives row-group and
page-index filtering, eq delete sets never do.
equality-delete key column later dropped from the table: this is
pre-existing, but nothing would reach the probe: the scan would fail outright
on `generate_batch_transform`:
```
Ok(field_id_to_mapped_schema_map
.get(field_id)
.ok_or(Error::new(ErrorKind::Unexpected, "field not found"))? // <------
here
.0
.clone())
```
I checked
[`DeleteFilter.java:76`](https://github.com/apache/iceberg/blob/main/data/src/main/java/org/apache/iceberg/data/DeleteFilter.java#L76)
and
[`DeleteFilter.java:318`](https://github.com/apache/iceberg/blob/main/data/src/main/java/org/apache/iceberg/data/DeleteFilter.java#L318):
I think this is the same failure. I think this is worth an issue rather than
checking on an error string here. WDYT?
--
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]
