Skip to content

Commit abef3a4

Browse files
committed
bugfix: Fix dense fragment domains during global order write with maximum fragment size (#5655)
In CORE-290 a customer reported issues with corrupt arrays after running consolidation. The symptom was memory allocation errors when opening an array. The root cause turned out to be the consolidation itself was writing new fragments where the fragment domain did not match the number of tiles in the fragment. The fragment metadata domain is a bounding rectangle. This means that the global order writer must split the tiles of its input into fragments at tile boundaries which bisect the bounding rectangle into two smaller bounding rectangles. To do so, we add a first pass identify_fragment_tile_boundaries which returns a list of tile offsets where new fragments will begin. Upon finishing a fragment, we use that tile offset to determine which rectangle within the target subarray the fragment actually represents, and update the fragment metadata accordingly. We use new functions is_rectangular_domain to determine whether a (start_tile, num_tiles) pair identifies a rectangle, and domain_tile_offset to compute that rectangle. Much of the complexity comes from the usage of the global order writer which does happen in consolidation: multi-part writes. A user (or a consolidation operation) can set a domain D which it intends to write into, and then actually fill in all of the cells over multiple submit calls which stream in the cells to write. It is not required for these cells to be tile aligned. Because of that, and the need to write rectangle fragments, a single submit cannot always determine whether a tail of tiles belongs to its current fragment or must be deferred to the next. To get around this we keep those tiles in memory in the global_write_state_ and prepend them to the user input in the next submit.
1 parent 8fcdd4d commit abef3a4

29 files changed

+4435
-535
lines changed

test/src/unit-cppapi-consolidation.cc

Lines changed: 425 additions & 0 deletions
Large diffs are not rendered by default.

test/src/unit-cppapi-max-fragment-size.cc

Lines changed: 1160 additions & 0 deletions
Large diffs are not rendered by default.

test/src/unit-sparse-global-order-reader.cc

Lines changed: 2 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -3598,19 +3598,7 @@ void CSparseGlobalOrderFx::run_execute(Instance& instance) {
35983598
ASSERTER(cursor_cells + num_cells <= expect.size());
35993599

36003600
// accumulate
3601-
std::apply(
3602-
[&](auto&... field) {
3603-
std::apply(
3604-
[&](auto&... field_cursor) {
3605-
std::apply(
3606-
[&](const auto&... field_size) {
3607-
(field.accumulate_cursor(field_cursor, field_size), ...);
3608-
},
3609-
field_sizes);
3610-
},
3611-
outcursor);
3612-
},
3613-
std::tuple_cat(outdims, outatts));
3601+
templates::query::accumulate_cursor(out, outcursor, field_sizes);
36143602

36153603
if (status == TILEDB_COMPLETED) {
36163604
break;
@@ -3620,15 +3608,7 @@ void CSparseGlobalOrderFx::run_execute(Instance& instance) {
36203608
// Clean up.
36213609
tiledb_query_free(&query);
36223610

3623-
std::apply(
3624-
[outcursor](auto&... outfield) {
3625-
std::apply(
3626-
[&](const auto&... field_cursor) {
3627-
(outfield.finish_multipart_read(field_cursor), ...);
3628-
},
3629-
outcursor);
3630-
},
3631-
std::tuple_cat(outdims, outatts));
3611+
templates::query::resize(out, outcursor);
36323612

36333613
ASSERTER(expect.dimensions() == outdims);
36343614

test/support/CMakeLists.txt

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,8 @@ list(APPEND TILEDB_CORE_INCLUDE_DIR "${CMAKE_SOURCE_DIR}/tiledb/sm/c_api")
3636

3737
# Gather the test source files
3838
set(TILEDB_TEST_SUPPORT_SOURCES
39-
rapidcheck/show.cc
39+
rapidcheck/show/array_schema_templates.cc
40+
rapidcheck/show/query_ast.cc
4041
src/array_helpers.cc
4142
src/array_schema_helpers.cc
4243
src/ast_helpers.h
Lines changed: 198 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,198 @@
1+
/**
2+
* @file test/support/rapidcheck/array_schema_templates.h
3+
*
4+
* @section LICENSE
5+
*
6+
* The MIT License
7+
*
8+
* @copyright Copyright (c) 2025 TileDB, Inc.
9+
*
10+
* Permission is hereby granted, free of charge, to any person obtaining a copy
11+
* of this software and associated documentation files (the "Software"), to deal
12+
* in the Software without restriction, including without limitation the rights
13+
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
14+
* copies of the Software, and to permit persons to whom the Software is
15+
* furnished to do so, subject to the following conditions:
16+
*
17+
* The above copyright notice and this permission notice shall be included in
18+
* all copies or substantial portions of the Software.
19+
*
20+
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
21+
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
22+
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
23+
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
24+
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
25+
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
26+
* THE SOFTWARE.
27+
*
28+
* @section DESCRIPTION
29+
*
30+
* This file defines rapidcheck generators for the structures
31+
* defined in test/support/src/array_schema_templates.h.
32+
*/
33+
34+
#ifndef TILEDB_RAPIDCHECK_ARRAY_SCHEMA_H
35+
#define TILEDB_RAPIDCHECK_ARRAY_SCHEMA_H
36+
37+
#include <test/support/src/array_schema_templates.h>
38+
#include <test/support/stdx/tuple.h>
39+
#include <test/support/tdb_rapidcheck.h>
40+
41+
#include "tiledb/common/arithmetic.h"
42+
43+
namespace rc {
44+
45+
using namespace tiledb::test;
46+
using namespace tiledb::test::templates;
47+
48+
template <DimensionType D>
49+
Gen<templates::Domain<D>> make_domain(std::optional<D> bound = std::nullopt) {
50+
auto bounds = gen::mapcat(gen::arbitrary<D>(), [bound](D lb) {
51+
const D ub_limit =
52+
(bound.has_value() ?
53+
tiledb::common::checked_arithmetic<D>::add(lb, bound.value())
54+
.value_or(std::numeric_limits<D>::max()) :
55+
std::numeric_limits<D>::max());
56+
if constexpr (std::is_same_v<D, int64_t> || std::is_same_v<D, uint64_t>) {
57+
return gen::pair(gen::just(lb), gen::inRange(lb, ub_limit));
58+
} else {
59+
// NB: `gen::inRange` is exclusive at the upper end but tiledb domain is
60+
// inclusive. So we have to use `int64_t` to avoid overflow.
61+
return gen::pair(
62+
gen::just(lb),
63+
gen::cast<D>(gen::inRange(int64_t(lb), int64_t(ub_limit) + 1)));
64+
}
65+
});
66+
67+
return gen::map(bounds, [](std::pair<D, D> bounds) {
68+
return templates::Domain<D>(bounds.first, bounds.second);
69+
});
70+
}
71+
72+
template <DimensionType D>
73+
struct Arbitrary<templates::Domain<D>> {
74+
static Gen<templates::Domain<D>> arbitrary() {
75+
return make_domain<D>();
76+
}
77+
};
78+
79+
template <DimensionType D>
80+
Gen<D> make_extent(
81+
const templates::Domain<D>& domain, std::optional<D> bound = std::nullopt) {
82+
// upper bound on all possible extents to avoid unreasonably
83+
// huge tile sizes
84+
static constexpr D extent_limit = static_cast<D>(
85+
std::is_signed<D>::value ?
86+
std::min(
87+
static_cast<int64_t>(std::numeric_limits<D>::max()),
88+
static_cast<int64_t>(1024 * 16)) :
89+
std::min(
90+
static_cast<uint64_t>(std::numeric_limits<D>::max()),
91+
static_cast<uint64_t>(1024 * 16)));
92+
93+
const D extent_bound =
94+
(bound.has_value() ? std::min(bound.value(), extent_limit) :
95+
extent_limit);
96+
97+
// NB: `gen::inRange` is exclusive at the upper end but tiledb domain is
98+
// inclusive. So we have to be careful to avoid overflow.
99+
100+
D extent_lower_bound = 1;
101+
D extent_upper_bound;
102+
103+
const auto bound_distance = tiledb::common::checked_arithmetic<D>::sub(
104+
domain.upper_bound, domain.lower_bound);
105+
if (bound_distance.has_value()) {
106+
extent_upper_bound =
107+
(bound_distance.value() < extent_bound ? bound_distance.value() + 1 :
108+
extent_bound);
109+
} else {
110+
extent_upper_bound = extent_bound;
111+
}
112+
113+
return gen::inRange(extent_lower_bound, extent_upper_bound + 1);
114+
}
115+
116+
template <tiledb::sm::Datatype D>
117+
Gen<templates::Dimension<D>> make_dimension(
118+
std::optional<typename templates::Dimension<D>::value_type> extent_bound =
119+
std::nullopt,
120+
std::optional<typename templates::Dimension<D>::value_type> domain_bound =
121+
std::nullopt) {
122+
using CoordType = templates::Dimension<D>::value_type;
123+
auto tup = gen::mapcat(
124+
make_domain<CoordType>(domain_bound),
125+
[extent_bound](Domain<CoordType> domain) {
126+
return gen::pair(gen::just(domain), make_extent(domain, extent_bound));
127+
});
128+
129+
return gen::map(tup, [](std::pair<Domain<CoordType>, CoordType> tup) {
130+
return templates::Dimension<D>(tup.first, tup.second);
131+
});
132+
}
133+
134+
template <tiledb::sm::Datatype D>
135+
struct Arbitrary<templates::Dimension<D>> {
136+
static Gen<templates::Dimension<D>> arbitrary() {
137+
return make_dimension<D>();
138+
}
139+
};
140+
141+
template <DimensionType D>
142+
Gen<D> make_coordinate(const templates::Domain<D>& domain) {
143+
// `gen::inRange` does an exclusive upper bound,
144+
// whereas the domain upper bound is inclusive.
145+
// As a result some contortion is required to deal
146+
// with numeric_limits.
147+
if constexpr (std::is_same_v<D, StringDimensionCoordType>) {
148+
// NB: poor performance with small domains for sure
149+
return gen::suchThat(
150+
gen::map(
151+
gen::string<std::string>(),
152+
[](std::string s) {
153+
StringDimensionCoordType v(s.begin(), s.end());
154+
return v;
155+
}),
156+
[domain](const StringDimensionCoordType& s) {
157+
return domain.lower_bound <= s && s <= domain.upper_bound;
158+
});
159+
} else if constexpr (std::is_signed<D>::value) {
160+
if (int64_t(domain.upper_bound) < std::numeric_limits<int64_t>::max()) {
161+
return gen::cast<D>(gen::inRange(
162+
int64_t(domain.lower_bound), int64_t(domain.upper_bound + 1)));
163+
} else {
164+
return gen::inRange(domain.lower_bound, domain.upper_bound);
165+
}
166+
} else {
167+
if (uint64_t(domain.upper_bound) < std::numeric_limits<uint64_t>::max()) {
168+
return gen::cast<D>(gen::inRange(
169+
uint64_t(domain.lower_bound), uint64_t(domain.upper_bound + 1)));
170+
} else {
171+
return gen::inRange(domain.lower_bound, domain.upper_bound);
172+
}
173+
}
174+
}
175+
176+
template <DimensionType D>
177+
Gen<templates::Domain<D>> make_range(const templates::Domain<D>& domain) {
178+
return gen::apply(
179+
[](D p1, D p2) { return templates::Domain<D>(p1, p2); },
180+
make_coordinate<D>(domain),
181+
make_coordinate<D>(domain));
182+
}
183+
184+
template <>
185+
void show<Domain<int>>(const templates::Domain<int>& domain, std::ostream& os);
186+
187+
template <>
188+
void show<Domain<uint64_t>>(
189+
const templates::Domain<uint64_t>& domain, std::ostream& os);
190+
191+
template <>
192+
void show<Dimension<tiledb::sm::Datatype::UINT64>>(
193+
const templates::Dimension<tiledb::sm::Datatype::UINT64>& dimension,
194+
std::ostream& os);
195+
196+
} // namespace rc
197+
198+
#endif

0 commit comments

Comments
 (0)