c-thiel commented on code in PR #331:
URL: https://github.com/apache/iceberg-rust/pull/331#discussion_r1670513358
##########
crates/iceberg/testdata/view_metadata/ViewMetadataV2Valid.json:
##########
@@ -0,0 +1,58 @@
+{
Review Comment:
@nastra I added more tests that cover non-happy cases.
However, there isn't much in
https://github.com/apache/iceberg/tree/5ea78e3fbe5d8c9c846a1b86bcd2b77d13a31acd/core/src/test/java/org/apache/iceberg/view
we can use for now. The `ViewMetadataBuilder` I started with is on the same
level as the `TableMetadataBuilder` - it can do almost nothing except changing
the UUID. The functions tested in the java part are simply not implemented yet.
We do have a more complete implementation over at our Iceberg catalog and want
to PR them here (https://github.com/hansetag/iceberg-catalog/issues/46), I just
want to start with the foundation before heading into the more controversial
builder bits.
Would this be alright for you?
Besides the Metadata-Builder, I would like to create 2 follow-up issues:
- Currently in `Snapshots` we use a fallible method for the timestamp()
method. For views I am using a non-fallible version, creating a small
inconsistency. I would create a new issues to remove `unwrap` from timestamps
where they are used.
- Allow for empty namespaces, following our discussion below. I am not sure
exactly how to handle this, but right now Rust does not allow the creation of
the empty Namespace as used in spark
(https://github.com/apache/iceberg-rust/blob/48f9e3e8bff0224569da8dd5beb89ed0c8cc4513/crates/iceberg/src/catalog/mod.rs#L111).
--
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]
