Skip to content

Add support for Carrara URIs#862

Merged
XanthosXanthopoulos merged 16 commits into
mainfrom
xan/VCF-41
Dec 10, 2025
Merged

Add support for Carrara URIs#862
XanthosXanthopoulos merged 16 commits into
mainfrom
xan/VCF-41

Conversation

@XanthosXanthopoulos
Copy link
Copy Markdown
Collaborator

@XanthosXanthopoulos XanthosXanthopoulos commented Dec 4, 2025

This PR adds support for TileDB Carrara URIs.
Due to changes to the TileDB data model for Carrara the vcf_headers array is now registered under the metadata group 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_headers array from the root group and registered it under the metadata group.

Copy link
Copy Markdown
Collaborator

@jp-dark jp-dark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread libtiledbvcf/src/utils/uri.cc Outdated
Comment thread libtiledbvcf/src/dataset/tiledbvcfdataset.cc
Copy link
Copy Markdown

@bkmartinjr bkmartinjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Member

@alancleary alancleary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 use std::string_view instead of std::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?

Comment thread libtiledbvcf/src/utils/uri.h Outdated
Comment thread libtiledbvcf/src/utils/uri.h
Comment thread libtiledbvcf/src/utils/uri.h Outdated
Comment thread libtiledbvcf/src/utils/uri.cc Outdated
Copy link
Copy Markdown
Collaborator

@jp-dark jp-dark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that core 2.30 is merged into main, the detect_tiledb_data_protocol should be replaced with core tiledb::Context.data_protocol method.

@jp-dark jp-dark requested review from bkmartinjr and removed request for bkmartinjr December 9, 2025 17:58
Copy link
Copy Markdown
Collaborator

@jp-dark jp-dark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have one suggested change (the verbage of the deprecation warning), but otherwise this looks ready to me.

Comment thread libtiledbvcf/src/dataset/tiledbvcfdataset.cc Outdated
@jp-dark jp-dark dismissed stale reviews from bkmartinjr and alancleary December 10, 2025 12:05

Not available for re-review

@jp-dark jp-dark removed the request for review from alancleary December 10, 2025 12:09
@XanthosXanthopoulos XanthosXanthopoulos merged commit e9b8cb7 into main Dec 10, 2025
11 checks passed
@XanthosXanthopoulos XanthosXanthopoulos deleted the xan/VCF-41 branch December 10, 2025 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants