Bug fix: update range end when merging overlapping subarray ranges. (#…

…5268) Bug fix: update range end when merging overlapping subarray ranges. Previously, when merging overlapping ranges, the new range end would _always_ be updated to that of the merged range. This was a _clear_ issue if one range was fully encompassed within another. For example, if range `{3, 4}` were merged into `{1, 10}`, the old behavior would produce a new range of `{1, 4}`. This PR fixes the bug simply, taking the larger of the two range ends on merge. [sc-53970] --- TYPE: BUG DESC: Update range end when merging overlapping subarray ranges. --------- Co-authored-by: KiterLuc <[email protected]> Co-authored-by: Luc Rancourt <[email protected]>
TileDB-Inc · Sep 3, 2024 · 7fd6328 · 7fd6328
1 parent 84ff80c
commit 7fd6328
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 4 deletions.
diff --git a/test/regression/targets/sc-53970.cc b/test/regression/targets/sc-53970.cc
@@ -41,7 +41,7 @@ static void write_array(const std::string& array_uri);
 
 TEST_CASE(
     "Subarray range expansion bug",
-    "[bug][sc53970][subarray-range-expansion][!shouldfail]") {
+    "[bug][sc53970][subarray-range-expansion]") {
   std::string array_uri = "test_array_schema_dump";
 
   // Test setup
@@ -78,7 +78,7 @@ TEST_CASE(
 
   // The expected result are a single matching cell of (0, 1507468.6)
   REQUIRE(dim[0] == 0);
-  REQUIRE(abs(attr[0] - 1507468.6) < 0.00000005);
+  REQUIRE(abs(attr[0] - 1507468.6f) < 0.00000005);
 
   // Check we didn't get any extra results
   REQUIRE(dim[1] == -1);

diff --git a/tiledb/sm/subarray/range_subset.h b/tiledb/sm/subarray/range_subset.h
@@ -5,7 +5,7 @@
  *
  * The MIT License
  *
- * @copyright Copyright (c) 2017-2023 TileDB, Inc.
+ * @copyright Copyright (c) 2017-2024 TileDB, Inc.
  *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to deal
@@ -169,7 +169,10 @@ struct MergeStrategy<
           tail->start_as<T>() <= head_end;
 
       if (can_merge) {
-        head->set_end_fixed(tail->end_fixed());
+        // Only update the end if the merging end is greater.
+        if (tail->end_as<T>() > head->end_as<T>()) {
+          head->set_end_fixed(tail->end_fixed());
+        }
         merged_cells++;
       } else {
         head++;

diff --git a/tiledb/sm/subarray/test/unit_range_subset.cc b/tiledb/sm/subarray/test/unit_range_subset.cc
@@ -528,6 +528,36 @@ TEMPLATE_TEST_CASE_SIG(
       check_subset_range_values(range_subset, 3, data3[0], data3[1]);
     }
   }
+
+  SECTION("Overlapping, encompassing ranges") {
+    // Note: this test is intended to duplicate regression test sc-53970,
+    // validating that a range which is fully encompassed within another
+    // will merge as expected.
+
+    // Add ranges.
+    T data1[2] = {3, 3};
+    T data2[2] = {1, 10};
+    std::vector<Range> ranges = {
+        Range(data1, 2 * sizeof(T)), Range(data2, 2 * sizeof(T))};
+    for (auto range : ranges) {
+      range_subset.add_range(range);
+    }
+    CHECK(range_subset.num_ranges() == 2);
+
+    // Sort and merge ranges.
+    ThreadPool pool{2};
+    range_subset.sort_and_merge_ranges(&pool, merge);
+
+    // Check range results.
+    if (merge) {
+      CHECK(range_subset.num_ranges() == 1);
+      check_subset_range_values(range_subset, 0, data2[0], data2[1]);
+    } else {
+      CHECK(range_subset.num_ranges() == 2);
+      check_subset_range_values(range_subset, 0, data2[0], data2[1]);
+      check_subset_range_values(range_subset, 1, data1[0], data1[1]);
+    }
+  }
 }
 
 TEST_CASE(