JosephLenton commented on PR #2916:
URL: https://github.com/apache/iceberg-rust/pull/2916#issuecomment-6658286420
Hey @blackmwk , I've spent all morning looking into this deeply to ensure it
is right. This will be a long comment, so please bear with me.
## An alternative
How about I change `RawLiteralEnum::Uuid` to `RawLiteralEnum::Fixed`. Would
you be on board with that? This is because:
- I can still get the UUID partitions working,
- it fixes a separate issue I found with Datum + Fixed (which I will get
into below),
- is this what you were alluding to?
## Example Usage
I have an example on a branch of the `datafusion-iceberg` crate. It is the
test `test_insert_into_partitioned_by_uuid` on the link here:
https://github.com/apache/datafusion-iceberg/commit/cb4aa25635b0dfd27a08dab1a290df3b3f2538ca#diff-f5d1a6793b0651f158f734b21a1bd0e001654916fbd1886f1ab474784e1282e0R920
The other example I have is an internal project at work, which I cannot
share. The Datafusion Iceberg test is the closest I can provide I'm afraid.
## Testing
I have also captured this behaviour in tests by adding many round trips to
show the behaviour is correct. I added them for Uuid, and also for Bytes and
Fixed to ensure they were right (as they all kind of overlap as byte groups).
These new tests are at:
- Avro Schema partition tests -
https://github.com/apache/iceberg-rust/pull/2916/changes#diff-fc6eafefdd01c65bd3fd8171485ded5ab051dc569d16ce926f7c863d8f399a9eR494
- Datum to/from Avro Value tests -
https://github.com/apache/iceberg-rust/pull/2916/changes#diff-95afb7dd393f71a7be83cf1f3cd85646099245a8bee07c860ee8c50216db582cR1585-R1665
These tests are essentially along the lines of:
1. build UUID data
2. write to Avro
3. read from Avro
4. assert we get UUIDs back.
## Test Outcomes
Here is a table of the tests using different implementations of
`RawLiteralEnum`. I've tested converting to UUID using `Bytes`, `UUID`, and the
now `Fixed` paths.
Here are the outcomes with pass ✅ and fail ❌:
| Test | `RawLiteralEnum::Bytes` | `RawLiteralEnum::Uuid` |
`RawLiteralEnum::Fixed` |
|---|---|---|---|
| `test_write_manifest_with_repeated_uuid_partition_type_as_spec_fixed_uuid`
| ❌ | ✅ | ✅ |
| `test_data_file_with_uuid_partition_roundtrips_through_avro` | ❌ | ✅ | ✅ |
| `test_data_file_with_16_byte_fixed_partition_roundtrips_through_avro` | ✅
| ✅ | ✅ |
| `test_data_file_with_16_byte_binary_partition_roundtrips_through_avro` | ✅
| ✅ | ✅ |
| `test_datum_uuid_converts_to_avro_fixed_value` | ❌ | ✅ | ✅ |
| `test_avro_uuid_value_converts_to_datum_uuid` | ✅ | ✅ | ✅ |
| `test_avro_fixed_value_converts_to_datum_uuid` | ✅ | ✅ | ✅ |
| `test_datum_16_fixed_converts_to_avro_fixed_value` | ❌ | ❌ | ✅ |
| `test_avro_fixed_value_converts_to_datum_16_fixed` | ✅ | ✅ | ✅ |
| `test_datum_16_binary_converts_to_avro_bytes_value` | ✅ | ✅ | ✅ |
| `test_avro_bytes_value_converts_to_datum_16_binary` | ✅ | ✅ | ✅ |
The tests show all cases are now covered by `RawLiteralEnum::Fixed`.
## Summary of breaking changes
* UUID partitions are written as UUIDs and work! 🎉
* `Datum::uuid()` serialises to `Value::Fixed`
* `Datum::fixed()` serialises to `Value::Fixed`
Let me know if you'd like me to go into any more details, or if you have
more concerns.
--
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]
