Skip to content

Commit 9d8943a

Browse files
shaunrd0shaunrd0
committed
Feedback from review
1 parent 843bdbb commit 9d8943a

File tree

8 files changed

+107
-108
lines changed

8 files changed

+107
-108
lines changed

‎tiledb/api/c_api/datatype/datatype_api.cc‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -45,11 +45,7 @@ capi_return_t tiledb_datatype_to_str(
4545

4646
capi_return_t tiledb_datatype_from_str(
4747
const char* str, tiledb_datatype_t* datatype) {
48-
tiledb::sm::Datatype val = tiledb::sm::Datatype::UINT8;
49-
if (!tiledb::sm::datatype_enum(str, &val).ok()) {
50-
return TILEDB_ERR;
51-
}
52-
*datatype = (tiledb_datatype_t)val;
48+
*datatype = (tiledb_datatype_t)tiledb::sm::datatype_enum(str);
5349
return TILEDB_OK;
5450
}
5551

‎tiledb/sm/enums/datatype.h‎

Lines changed: 48 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -235,100 +235,99 @@ inline const std::string& datatype_str(Datatype type) {
235235
}
236236

237237
/** Returns the datatype given a string representation. */
238-
inline Status datatype_enum(
239-
const std::string& datatype_str, Datatype* datatype) {
238+
inline Datatype datatype_enum(const std::string& datatype_str) {
240239
if (datatype_str == constants::int32_str)
241-
*datatype = Datatype::INT32;
240+
return Datatype::INT32;
242241
else if (datatype_str == constants::int64_str)
243-
*datatype = Datatype::INT64;
242+
return Datatype::INT64;
244243
else if (datatype_str == constants::float32_str)
245-
*datatype = Datatype::FLOAT32;
244+
return Datatype::FLOAT32;
246245
else if (datatype_str == constants::float64_str)
247-
*datatype = Datatype::FLOAT64;
246+
return Datatype::FLOAT64;
248247
else if (datatype_str == constants::char_str)
249-
*datatype = Datatype::CHAR;
248+
return Datatype::CHAR;
250249
else if (datatype_str == constants::blob_str)
251-
*datatype = Datatype::BLOB;
250+
return Datatype::BLOB;
252251
else if (datatype_str == constants::geom_wkb_str)
253-
*datatype = Datatype::GEOM_WKB;
252+
return Datatype::GEOM_WKB;
254253
else if (datatype_str == constants::geom_wkt_str)
255-
*datatype = Datatype::GEOM_WKT;
254+
return Datatype::GEOM_WKT;
256255
else if (datatype_str == constants::bool_str)
257-
*datatype = Datatype::BOOL;
256+
return Datatype::BOOL;
258257
else if (datatype_str == constants::int8_str)
259-
*datatype = Datatype::INT8;
258+
return Datatype::INT8;
260259
else if (datatype_str == constants::uint8_str)
261-
*datatype = Datatype::UINT8;
260+
return Datatype::UINT8;
262261
else if (datatype_str == constants::int16_str)
263-
*datatype = Datatype::INT16;
262+
return Datatype::INT16;
264263
else if (datatype_str == constants::uint16_str)
265-
*datatype = Datatype::UINT16;
264+
return Datatype::UINT16;
266265
else if (datatype_str == constants::uint32_str)
267-
*datatype = Datatype::UINT32;
266+
return Datatype::UINT32;
268267
else if (datatype_str == constants::uint64_str)
269-
*datatype = Datatype::UINT64;
268+
return Datatype::UINT64;
270269
else if (datatype_str == constants::string_ascii_str)
271-
*datatype = Datatype::STRING_ASCII;
270+
return Datatype::STRING_ASCII;
272271
else if (datatype_str == constants::string_utf8_str)
273-
*datatype = Datatype::STRING_UTF8;
272+
return Datatype::STRING_UTF8;
274273
else if (datatype_str == constants::string_utf16_str)
275-
*datatype = Datatype::STRING_UTF16;
274+
return Datatype::STRING_UTF16;
276275
else if (datatype_str == constants::string_utf32_str)
277-
*datatype = Datatype::STRING_UTF32;
276+
return Datatype::STRING_UTF32;
278277
else if (datatype_str == constants::string_ucs2_str)
279-
*datatype = Datatype::STRING_UCS2;
278+
return Datatype::STRING_UCS2;
280279
else if (datatype_str == constants::string_ucs4_str)
281-
*datatype = Datatype::STRING_UCS4;
280+
return Datatype::STRING_UCS4;
282281
else if (datatype_str == constants::any_str)
283-
*datatype = Datatype::ANY;
282+
return Datatype::ANY;
284283
else if (datatype_str == constants::datetime_year_str)
285-
*datatype = Datatype::DATETIME_YEAR;
284+
return Datatype::DATETIME_YEAR;
286285
else if (datatype_str == constants::datetime_month_str)
287-
*datatype = Datatype::DATETIME_MONTH;
286+
return Datatype::DATETIME_MONTH;
288287
else if (datatype_str == constants::datetime_week_str)
289-
*datatype = Datatype::DATETIME_WEEK;
288+
return Datatype::DATETIME_WEEK;
290289
else if (datatype_str == constants::datetime_day_str)
291-
*datatype = Datatype::DATETIME_DAY;
290+
return Datatype::DATETIME_DAY;
292291
else if (datatype_str == constants::datetime_hr_str)
293-
*datatype = Datatype::DATETIME_HR;
292+
return Datatype::DATETIME_HR;
294293
else if (datatype_str == constants::datetime_min_str)
295-
*datatype = Datatype::DATETIME_MIN;
294+
return Datatype::DATETIME_MIN;
296295
else if (datatype_str == constants::datetime_sec_str)
297-
*datatype = Datatype::DATETIME_SEC;
296+
return Datatype::DATETIME_SEC;
298297
else if (datatype_str == constants::datetime_ms_str)
299-
*datatype = Datatype::DATETIME_MS;
298+
return Datatype::DATETIME_MS;
300299
else if (datatype_str == constants::datetime_us_str)
301-
*datatype = Datatype::DATETIME_US;
300+
return Datatype::DATETIME_US;
302301
else if (datatype_str == constants::datetime_ns_str)
303-
*datatype = Datatype::DATETIME_NS;
302+
return Datatype::DATETIME_NS;
304303
else if (datatype_str == constants::datetime_ps_str)
305-
*datatype = Datatype::DATETIME_PS;
304+
return Datatype::DATETIME_PS;
306305
else if (datatype_str == constants::datetime_fs_str)
307-
*datatype = Datatype::DATETIME_FS;
306+
return Datatype::DATETIME_FS;
308307
else if (datatype_str == constants::datetime_as_str)
309-
*datatype = Datatype::DATETIME_AS;
308+
return Datatype::DATETIME_AS;
310309
else if (datatype_str == constants::time_hr_str)
311-
*datatype = Datatype::TIME_HR;
310+
return Datatype::TIME_HR;
312311
else if (datatype_str == constants::time_min_str)
313-
*datatype = Datatype::TIME_MIN;
312+
return Datatype::TIME_MIN;
314313
else if (datatype_str == constants::time_sec_str)
315-
*datatype = Datatype::TIME_SEC;
314+
return Datatype::TIME_SEC;
316315
else if (datatype_str == constants::time_ms_str)
317-
*datatype = Datatype::TIME_MS;
316+
return Datatype::TIME_MS;
318317
else if (datatype_str == constants::time_us_str)
319-
*datatype = Datatype::TIME_US;
318+
return Datatype::TIME_US;
320319
else if (datatype_str == constants::time_ns_str)
321-
*datatype = Datatype::TIME_NS;
320+
return Datatype::TIME_NS;
322321
else if (datatype_str == constants::time_ps_str)
323-
*datatype = Datatype::TIME_PS;
322+
return Datatype::TIME_PS;
324323
else if (datatype_str == constants::time_fs_str)
325-
*datatype = Datatype::TIME_FS;
324+
return Datatype::TIME_FS;
326325
else if (datatype_str == constants::time_as_str)
327-
*datatype = Datatype::TIME_AS;
326+
return Datatype::TIME_AS;
328327
else {
329-
return Status_Error("Invalid Datatype " + datatype_str);
328+
throw std::runtime_error(
329+
"Invalid Datatype string (\"" + datatype_str + "\")");
330330
}
331-
return Status::Ok();
332331
}
333332

334333
/** Returns true if the input datatype is a string type. */
@@ -440,12 +439,7 @@ inline void ensure_datatype_is_valid(Datatype type) {
440439
* the datatype string's enum is not between 0 and 43.
441440
**/
442441
inline void ensure_datatype_is_valid(const std::string& datatype_str) {
443-
Datatype datatype_type;
444-
Status st{datatype_enum(datatype_str, &datatype_type)};
445-
if (!st.ok()) {
446-
throw std::runtime_error(
447-
"Invalid Datatype string (\"" + datatype_str + "\")");
448-
}
442+
Datatype datatype_type = datatype_enum(datatype_str);
449443
ensure_datatype_is_valid(datatype_type);
450444
}
451445

‎tiledb/sm/serialization/array.cc‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -102,8 +102,7 @@ Status metadata_from_capnp(
102102
auto entry_reader = entries_reader[i];
103103
auto key = std::string{std::string_view{
104104
entry_reader.getKey().cStr(), entry_reader.getKey().size()}};
105-
Datatype type = Datatype::UINT8;
106-
RETURN_NOT_OK(datatype_enum(entry_reader.getType(), &type));
105+
Datatype type = datatype_enum(entry_reader.getType());
107106
uint32_t value_num = entry_reader.getValueNum();
108107

109108
auto value_ptr = entry_reader.getValue();

‎tiledb/sm/serialization/array_schema.cc‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -400,8 +400,7 @@ void attribute_to_capnp(
400400
shared_ptr<Attribute> attribute_from_capnp(
401401
const capnp::Attribute::Reader& attribute_reader) {
402402
// Get datatype
403-
Datatype datatype = Datatype::ANY;
404-
throw_if_not_ok(datatype_enum(attribute_reader.getType(), &datatype));
403+
Datatype datatype = datatype_enum(attribute_reader.getType());
405404

406405
// Set nullable
407406
const bool nullable = attribute_reader.getNullable();
@@ -614,8 +613,7 @@ shared_ptr<Dimension> dimension_from_capnp(
614613
Status st;
615614

616615
// Deserialize datatype
617-
Datatype dim_type;
618-
st = datatype_enum(dimension_reader.getType().cStr(), &dim_type);
616+
Datatype dim_type = datatype_enum(dimension_reader.getType().cStr());
619617
if (!st.ok()) {
620618
throw std::runtime_error(
621619
"[Deserialization::dimension_from_capnp] " +
@@ -782,8 +780,7 @@ void dimension_label_to_capnp(
782780
shared_ptr<DimensionLabel> dimension_label_from_capnp(
783781
const capnp::DimensionLabel::Reader& dim_label_reader) {
784782
// Get datatype
785-
Datatype datatype = Datatype::ANY;
786-
throw_if_not_ok(datatype_enum(dim_label_reader.getType(), &datatype));
783+
Datatype datatype = datatype_enum(dim_label_reader.getType());
787784

788785
shared_ptr<ArraySchema> schema{nullptr};
789786
if (dim_label_reader.hasSchema()) {

‎tiledb/sm/serialization/enumeration.cc‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -74,8 +74,7 @@ shared_ptr<const Enumeration> enumeration_from_capnp(
7474
const capnp::Enumeration::Reader& reader) {
7575
auto name = reader.getName();
7676
auto path_name = reader.getPathName();
77-
Datatype datatype = Datatype::ANY;
78-
throw_if_not_ok(datatype_enum(reader.getType(), &datatype));
77+
Datatype datatype = datatype_enum(reader.getType());
7978

8079
const void* data = nullptr;
8180
uint64_t data_size = 0;

‎tiledb/sm/serialization/query.cc‎

Lines changed: 20 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -271,33 +271,35 @@ Subarray subarray_from_capnp(
271271
auto ranges_reader = reader.getRanges();
272272

273273
uint32_t dim_num = ranges_reader.size();
274-
std::vector<RangeSetAndSuperset> range_subset(dim_num);
274+
std::vector<RangeSetAndSuperset> range_subset;
275+
range_subset.reserve(dim_num);
275276
std::vector<bool> is_default(dim_num, false);
276277
for (uint32_t i = 0; i < dim_num; i++) {
277278
auto range_reader = ranges_reader[i];
278-
Datatype type = Datatype::UINT8;
279-
throw_if_not_ok(datatype_enum(range_reader.getType(), &type));
279+
Datatype type = datatype_enum(range_reader.getType());
280280
auto dim = array->array_schema_latest().dimension_ptr(i);
281281

282-
bool implicitly_initialized = range_reader.getHasDefaultRange();
283-
range_subset[i] = RangeSetAndSuperset(
284-
dim->type(), dim->domain(), implicitly_initialized, coalesce_ranges);
285-
is_default[i] = implicitly_initialized;
286-
if (range_reader.hasBufferSizes()) {
287-
auto ranges = range_buffers_from_capnp(range_reader);
282+
is_default[i] = range_reader.getHasDefaultRange();
283+
if (is_default[i]) {
288284
// If the range is implicitly initialized, the RangeSetAndSuperset
289285
// constructor will initialize the ranges to the domain.
290-
if (!implicitly_initialized) {
286+
range_subset.emplace_back(
287+
type, dim->domain(), is_default[i], coalesce_ranges);
288+
} else {
289+
std::vector<Range> ranges;
290+
if (range_reader.hasBufferSizes()) {
291+
ranges = range_buffers_from_capnp(range_reader);
291292
// Add custom ranges, clearing any implicit ranges previously set.
292-
range_subset[i] = RangeSetAndSuperset(
293-
dim->type(), dim->domain(), ranges, coalesce_ranges);
293+
range_subset.emplace_back(
294+
type, dim->domain(), std::move(ranges), coalesce_ranges);
295+
} else {
296+
// Handle 1.7 style ranges where there is a single range with no sizes
297+
auto data = range_reader.getBuffer();
298+
auto data_ptr = data.asBytes();
299+
ranges.emplace_back(data_ptr.begin(), data.size());
300+
range_subset.emplace_back(
301+
type, dim->domain(), std::move(ranges), coalesce_ranges);
294302
}
295-
} else {
296-
// Handle 1.7 style ranges where there is a single range with no sizes
297-
auto data = range_reader.getBuffer();
298-
auto data_ptr = data.asBytes();
299-
Range range(data_ptr.begin(), data.size());
300-
throw_if_not_ok(range_subset[i].add_range_unrestricted(range));
301303
}
302304
}
303305

‎tiledb/sm/subarray/range_subset.cc‎

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -135,10 +135,11 @@ RangeSetAndSuperset::RangeSetAndSuperset(
135135
RangeSetAndSuperset::RangeSetAndSuperset(
136136
Datatype datatype,
137137
const Range& superset,
138-
std::vector<Range> subset,
138+
std::vector<Range>&& subset,
139139
bool coalesce_ranges)
140-
: RangeSetAndSuperset(datatype, superset, false, coalesce_ranges) {
141-
ranges_ = std::move(subset);
140+
: impl_(range_subset_internals(datatype, superset, coalesce_ranges))
141+
, is_implicitly_initialized_(false)
142+
, ranges_(subset) {
142143
}
143144

144145
void RangeSetAndSuperset::sort_and_merge_ranges(
@@ -171,6 +172,13 @@ tuple<Status, optional<std::string>> RangeSetAndSuperset::add_range(
171172
}
172173
}
173174

175+
void RangeSetAndSuperset::check_oob() {
176+
for (auto& range : ranges_) {
177+
impl_->check_range_is_valid(range);
178+
throw_if_not_ok(impl_->check_range_is_subset(range));
179+
}
180+
}
181+
174182
Status RangeSetAndSuperset::add_range_unrestricted(const Range& range) {
175183
if (is_implicitly_initialized_) {
176184
ranges_.clear();
@@ -179,11 +187,4 @@ Status RangeSetAndSuperset::add_range_unrestricted(const Range& range) {
179187
return impl_->add_range(ranges_, range);
180188
}
181189

182-
void RangeSetAndSuperset::check_oob() {
183-
for (auto& range : ranges_) {
184-
impl_->check_range_is_valid(range);
185-
throw_if_not_ok(impl_->check_range_is_subset(range));
186-
}
187-
}
188-
189190
} // namespace tiledb::sm

‎tiledb/sm/subarray/range_subset.h‎

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -419,6 +419,9 @@ class TypedRangeSetAndFullsetImpl<std::string, CoalesceAdds>
419419
*/
420420
class RangeSetAndSuperset {
421421
public:
422+
/** Friend Subarray for calling `add_range_unrestricted` */
423+
friend class Subarray;
424+
422425
/* ********************************* */
423426
/* CONSTRUCTORS & DESTRUCTORS */
424427
/* ********************************* */
@@ -453,7 +456,7 @@ class RangeSetAndSuperset {
453456
RangeSetAndSuperset(
454457
Datatype datatype,
455458
const Range& superset,
456-
std::vector<Range> subset,
459+
std::vector<Range>&& subset,
457460
bool coalesce_ranges);
458461

459462
/** Destructor. */
@@ -484,16 +487,6 @@ class RangeSetAndSuperset {
484487
tuple<Status, optional<std::string>> add_range(
485488
Range& range, const bool read_range_oob_error = true);
486489

487-
/**
488-
* Adds a range to the range manager without performing any checkes.
489-
*
490-
* If the ranges are currently implicitly initialized, then they will be
491-
* cleared before the new range is added.
492-
*
493-
* @param range The range to add.
494-
*/
495-
Status add_range_unrestricted(const Range& range);
496-
497490
/**
498491
* Removes all ranges.
499492
*
@@ -559,6 +552,10 @@ class RangeSetAndSuperset {
559552
void sort_and_merge_ranges(ThreadPool* const compute_tp, bool merge = false);
560553

561554
private:
555+
/* ********************************* */
556+
/* PRIVATE ATTRIBUTES */
557+
/* ********************************* */
558+
562559
/** Pointer to typed implementation details. */
563560
shared_ptr<detail::RangeSetAndSupersetImpl> impl_ = nullptr;
564561

@@ -571,6 +568,20 @@ class RangeSetAndSuperset {
571568

572569
/** Stored ranges. */
573570
std::vector<Range> ranges_{};
571+
572+
/* ********************************* */
573+
/* PRIVATE METHODS */
574+
/* ********************************* */
575+
576+
/**
577+
* Adds a range to the range manager without performing any checkes.
578+
*
579+
* If the ranges are currently implicitly initialized, then they will be
580+
* cleared before the new range is added.
581+
*
582+
* @param range The range to add.
583+
*/
584+
Status add_range_unrestricted(const Range& range);
574585
};
575586

576587
} // namespace tiledb::sm

0 commit comments

Comments
 (0)