Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent consolidation with fragment list in dense arrays if it could result in data loss. #5251

Merged

Conversation

DimitrisStaratzis
Copy link
Contributor

@DimitrisStaratzis DimitrisStaratzis commented Aug 22, 2024

When using fragment list consolidation with dense arrays, there is a possibility that consolidating some fragments would lead to data loss (see https://docs.tiledb.com/main/background/internal-mechanics/consolidation#algorithm for more details). This PR fixes this issue.


TYPE: IMPROVEMENT
DESC: Prevent consolidation with fragment list in dense arrays if it could result in data loss.

@DimitrisStaratzis DimitrisStaratzis force-pushed the dstara/sc-50064/disallow-dense-fragment-list-consolidation branch 2 times, most recently from 5e182e4 to 8d49155 Compare August 22, 2024 10:54
@DimitrisStaratzis DimitrisStaratzis marked this pull request as ready for review August 23, 2024 00:23
test/src/unit-cppapi-consolidation.cc Outdated Show resolved Hide resolved
test/src/unit-cppapi-consolidation.cc Show resolved Hide resolved
test/src/unit-cppapi-consolidation.cc Show resolved Hide resolved
test/src/unit-cppapi-consolidation.cc Outdated Show resolved Hide resolved
tiledb/sm/consolidator/fragment_consolidator.cc Outdated Show resolved Hide resolved
tiledb/sm/consolidator/fragment_consolidator.cc Outdated Show resolved Hide resolved
tiledb/sm/consolidator/fragment_consolidator.cc Outdated Show resolved Hide resolved
@DimitrisStaratzis DimitrisStaratzis changed the title Prevent consolidation with fragment lists in Dense Arrays if it could result in data loss Prevent consolidation with fragment list in dense arrays if it could result in data loss Aug 29, 2024
@DimitrisStaratzis DimitrisStaratzis force-pushed the dstara/sc-50064/disallow-dense-fragment-list-consolidation branch from efce830 to e0a4714 Compare August 29, 2024 12:21
Copy link
Contributor

@KiterLuc KiterLuc left a comment

Choose a reason for hiding this comment

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

Easy fixes and this is good to go.

tiledb/sm/consolidator/fragment_consolidator.cc Outdated Show resolved Hide resolved
tiledb/sm/consolidator/fragment_consolidator.cc Outdated Show resolved Hide resolved
// cheaper. We compare the current fragment's start timestamp against the
// upper bound we calculated previously.
auto timestamp_range{frag_info.timestamp_range()};
bool timestamp_before = !(timestamp_range.first > max_timestamp);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bool timestamp_before = !(timestamp_range.first > max_timestamp);
bool timestamps_overlap = !(timestamp_range.first > max_timestamp);

@KiterLuc KiterLuc changed the title Prevent consolidation with fragment list in dense arrays if it could result in data loss Prevent consolidation with fragment list in dense arrays if it could result in data loss. Sep 9, 2024
@KiterLuc KiterLuc merged commit 119c46f into dev Sep 9, 2024
65 checks passed
@KiterLuc KiterLuc deleted the dstara/sc-50064/disallow-dense-fragment-list-consolidation branch September 9, 2024 14:12
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.

2 participants