Fix fragment consolidation to allow using absolute URIs. #5135
Conversation
shaunrd0
force-pushed
the
smr/sc-49934/relative-consolidation-uris
branch
from
5c5d147
to
7fa89f3
Compare
|
@KiterLuc this is a fix in |
Sorry 2.24.1 has already sailed :( |
shaunrd0
force-pushed
the
smr/sc-49934/relative-consolidation-uris
branch
from
7fa89f3
to
ce20790
Compare
Adding a backport tag in case we go for 2.24.2 |
| @@ -362,7 +362,7 @@ Status FragmentConsolidator::consolidate_fragments( | |||
| NDRange union_non_empty_domains; | |||
| std::unordered_set<std::string> to_consolidate_set; | |||
| for (auto& uri : fragment_uris) { | |||
| to_consolidate_set.emplace(uri); | |||
| to_consolidate_set.emplace(URI(uri).last_path_part()); | |||
There was a problem hiding this comment.
We should be more rigorous here and validate that the removed part is either the array URI for old fragment style or array_uri + "__fragments". Let's add coverage for all.
There was a problem hiding this comment.
Should relative fragment URIs not be allowed? I'm just thinking for that case neither of these checks would pass. The removed part of the URI would be the CWD if we do something like URI("__1719935189448_1719935189448_2eebea0af3ad535fe64ab72eee433790_22").last_path_part() for example.
There was a problem hiding this comment.
The old way of using the API should still work.
shaunrd0
force-pushed
the
smr/sc-49934/relative-consolidation-uris
branch
from
fe1e779
to
a5e55c1
Compare
|
|
||
| // Normalizes path separators for windows paths. | ||
| std::string array_uri = URI(array_name).to_string(); | ||
| // Check for valid URI based on array format version. |
There was a problem hiding this comment.
Use array_for_reads->array_directory().get_fragments_dir(write_version);
There was a problem hiding this comment.
Much better, thanks!
+ Add test for v11 array.
Also supports S3 but not across file systems.
shaunrd0
force-pushed
the
smr/sc-49934/relative-consolidation-uris
branch
from
7ba2202
to
3981759
Compare
| // Check for valid URI based on array format version. | ||
| auto fragments_dir = array_for_reads->array_directory().get_fragments_dir( | ||
| frag_id.array_format_version()); | ||
| if (!fragment_uri.contains(fragments_dir)) { |
There was a problem hiding this comment.
uri != fragments_dir.join_path(uri.last_path_part().c_str())
This fixes fragment consolidation to allow using absolute URIs. I ran into this while adding a TileDB-Go binding for `tiledb_array_consolidate_fragments` in [SC-49723](https://app.shortcut.com/tiledb-inc/story/49723/add-tiledb-go-binding-for-tiledb-array-consolidate-fragments) by passing URIs directly from the [FragmentInfo APIs.](https://github.com/TileDB-Inc/TileDB-Go/pull/322/files#diff-c37e5f4dd452918ab7c467ff243d69310dbd1b238b7b717a98871f80cf0fab70R156) --- TYPE: BUG DESC: Fix fragment consolidation to allow using absolute URIs. (cherry picked from commit a7e2a07)
This fixes fragment consolidation to allow using absolute URIs. I ran into this while adding a TileDB-Go binding for
tiledb_array_consolidate_fragmentsin SC-49723 by passing URIs directly from the FragmentInfo APIs.TYPE: BUG
DESC: Fix fragment consolidation to allow using absolute URIs.