Add support for Carrara URIs#862
Conversation
jp-dark
left a comment
There was a problem hiding this comment.
The overall code looks good, but this is missing testing. The Carrara testing can go in a separate repo if it makes setting up CI easier, but it should be linked to in this PR.
bkmartinjr
left a comment
There was a problem hiding this comment.
Code looks OK except for the try/catch (which should be if/else)
Biggest issue is lack of testing. Suggest at least:
- a read path check on new and old format (ie., have two test datasets, make sure you can read both)
- simple unit tests for the data protocol detection (to make sure it continues to work as expected in the transition to 2.30-based implementation)
alancleary
left a comment
There was a problem hiding this comment.
In addition to @jp-dark and @bkmartinjr's reviews and my specific code comments, here's a few general comments:
- In
src/utils/uri.{h,cc}let's go ahead and update the other functions to usestd::string_viewinstead ofstd::string&, for consistency. - Since this PR is a breaking change, opening a header array that's registered under the root group should raise a deprecation warning, perhaps with a link to instructions on how to migrate
- Should this PR include documentation on how to migrate an existing dataset to the new group protocol?
jp-dark
left a comment
There was a problem hiding this comment.
Now that core 2.30 is merged into main, the detect_tiledb_data_protocol should be replaced with core tiledb::Context.data_protocol method.
e91b279 to
7739d94
Compare
jp-dark
left a comment
There was a problem hiding this comment.
I have one suggested change (the verbage of the deprecation warning), but otherwise this looks ready to me.
Not available for re-review
This PR adds support for TileDB Carrara URIs.
Due to changes to the TileDB data model for Carrara the
vcf_headersarray is now registered under themetadatagroup rather than the top level VCF group. Backwards compatibility for read/write is maintained but new datasets are created with this changes.To migrate and old dataset to the new format you need to unregister the
vcf_headersarray from the root group and registered it under themetadatagroup.