From c8e32bcdb43a42ac2416c9ca6030fdd08fb4b44d Mon Sep 17 00:00:00 2001 From: Rebekah Date: Fri, 10 Feb 2023 16:47:15 -0500 Subject: [PATCH 1/2] Error out no-op set_config calls: Query --- .../unit-cppapi-partial-attribute-write.cc | 6 +- tiledb/sm/c_api/tiledb.cc | 4 +- tiledb/sm/cpp_api/query.h | 2 +- tiledb/sm/query/query.cc | 49 +++++----- tiledb/sm/query/query.h | 21 +++- tiledb/sm/serialization/query.cc | 4 +- tiledb/sm/subarray/subarray.cc | 95 +++++++++---------- tiledb/sm/subarray/subarray.h | 4 +- 8 files changed, 99 insertions(+), 86 deletions(-) diff --git a/test/src/unit-cppapi-partial-attribute-write.cc b/test/src/unit-cppapi-partial-attribute-write.cc index 7159f058790..b5922757b06 100644 --- a/test/src/unit-cppapi-partial-attribute-write.cc +++ b/test/src/unit-cppapi-partial-attribute-write.cc @@ -5,7 +5,7 @@ * * The MIT License * - * @copyright Copyright (c) 2022 TileDB Inc. + * @copyright Copyright (c) 2023 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 @@ -229,7 +229,9 @@ TEST_CASE_METHOD( Query query(ctx_, array, TILEDB_WRITE); query.set_layout(TILEDB_UNORDERED); query.set_data_buffer("d1", dim1); - CHECK_THROWS_WITH(query.submit(), "Query: Dimension buffer d2 is not set"); + CHECK_THROWS_WITH( + query.submit(), + "Query: [check_buffer_names] Dimension buffer d2 is not set"); array.close(); diff --git a/tiledb/sm/c_api/tiledb.cc b/tiledb/sm/c_api/tiledb.cc index 393aa15212d..bdbed6021f4 100644 --- a/tiledb/sm/c_api/tiledb.cc +++ b/tiledb/sm/c_api/tiledb.cc @@ -1720,7 +1720,7 @@ int32_t tiledb_query_set_config( if (sanity_check(ctx, query) == TILEDB_ERR) return TILEDB_ERR; api::ensure_config_is_valid(config); - throw_if_not_ok(query->query_->set_config(config->config())); + query->query_->set_config(config->config()); return TILEDB_OK; } @@ -2412,7 +2412,7 @@ int32_t tiledb_subarray_set_config( if (sanity_check(ctx, subarray) == TILEDB_ERR) return TILEDB_ERR; api::ensure_config_is_valid(config); - throw_if_not_ok(subarray->subarray_->set_config(config->config())); + subarray->subarray_->set_config(config->config()); return TILEDB_OK; } diff --git a/tiledb/sm/cpp_api/query.h b/tiledb/sm/cpp_api/query.h index 56fdea3f403..0cd77e18643 100644 --- a/tiledb/sm/cpp_api/query.h +++ b/tiledb/sm/cpp_api/query.h @@ -7,7 +7,7 @@ * * The MIT License * - * @copyright Copyright (c) 2017-2022 TileDB, Inc. + * @copyright Copyright (c) 2017-2023 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 diff --git a/tiledb/sm/query/query.cc b/tiledb/sm/query/query.cc index 656b5646d09..019d93202ea 100644 --- a/tiledb/sm/query/query.cc +++ b/tiledb/sm/query/query.cc @@ -5,7 +5,7 @@ * * The MIT License * - * @copyright Copyright (c) 2017-2021 TileDB, Inc. + * @copyright Copyright (c) 2017-2023 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 @@ -129,7 +129,7 @@ Query::Query( config_ = storage_manager->config(); // Set initial subarray configuration - throw_if_not_ok(subarray_.set_config(config_)); + subarray_.set_config(config_); rest_scratch_ = make_shared(HERE()); @@ -973,7 +973,8 @@ Status Query::check_buffer_names() { auto dim = array_schema_->dimension_ptr(d); if (buffers_.count(dim->name()) == 0) { throw QueryStatusException( - "Dimension buffer " + dim->name() + " is not set"); + "[check_buffer_names] Dimension buffer " + dim->name() + + " is not set"); } } } @@ -1215,8 +1216,12 @@ Status Query::check_set_fixed_buffer(const std::string& name) { return Status::Ok(); } -Status Query::set_config(const Config& config) { - config_ = config; +void Query::set_config(const Config& config) { + if (status_ != QueryStatus::UNINITIALIZED) { + throw QueryStatusException( + "[set_config] Cannot set config after initialization."); + } + config_.inherit(config); // Refresh memory budget configuration. if (strategy_ != nullptr) { @@ -1226,9 +1231,7 @@ Status Query::set_config(const Config& config) { // Set subarray's config for backwards compatibility // Users expect the query config to effect the subarray based on existing // behavior before subarray was exposed directly - throw_if_not_ok(subarray_.set_config(config_)); - - return Status::Ok(); + subarray_.set_config(config_); } Status Query::set_coords_buffer(void* buffer, uint64_t* buffer_size) { @@ -1282,8 +1285,7 @@ Status Query::set_data_buffer( if (array_schema_->is_dim_label(name)) { // Check the query type is valid. if (type_ != QueryType::READ && type_ != QueryType::WRITE) { - throw StatusException(Status_SerializationError( - "Cannot set buffer; Unsupported query type.")); + throw QueryStatusException("[set_data_buffer] Unsupported query type."); } // Set dimension label buffer on the appropriate buffer depending if the @@ -1398,21 +1400,22 @@ Status Query::set_offsets_buffer( if (array_schema_->is_dim_label(name)) { // Check the query type is valid. if (type_ != QueryType::READ && type_ != QueryType::WRITE) { - throw QueryStatusException("Cannot set buffer; Unsupported query type."); + throw QueryStatusException( + "[set_offsets_buffer] Unsupported query type."); } // Check the dimension labe is in fact variable length. if (!array_schema_->dimension_label(name).is_var()) { throw QueryStatusException( - std::string("Cannot set buffer; Input dimension label '") + name + + "[set_offsets_buffer] Input dimension label '" + name + "' is fixed-sized"); } // Check the query was not already initialized. if (status_ != QueryStatus::UNINITIALIZED) { throw QueryStatusException( - std::string("Cannot set buffer for new dimension label '") + name + - "' after initialization"); + "[set_offsets_buffer] Cannot set buffer for new dimension label '" + + name + "' after initialization"); } // Set dimension label offsets buffers. @@ -1651,7 +1654,7 @@ void Query::set_subarray(const void* subarray) { case QueryType::MODIFY_EXCLUSIVE: if (!array_schema_->dense()) { throw QueryStatusException( - "Cannot set subarray; Setting a subarray is not supported on " + "[set_subarray] Setting a subarray is not supported on " "sparse writes."); } break; @@ -1659,15 +1662,15 @@ void Query::set_subarray(const void* subarray) { default: throw QueryStatusException( - "Cannot set subarray; Setting a subarray is not supported for query " - "type '" + + "[set_subarray] Setting a subarray is not supported for query type " + "'" + query_type_str(type_) + "'."); } // Check this isn't an already initialized query using dimension labels. if (status_ != QueryStatus::UNINITIALIZED) { throw QueryStatusException( - "Cannot set subarray; Setting a subarray on an already initialized " + "[set_subarray] Setting a subarray on an already initialized " "query is not supported."); } @@ -1694,22 +1697,22 @@ void Query::set_subarray(const tiledb::sm::Subarray& subarray) { case QueryType::MODIFY_EXCLUSIVE: if (!array_schema_->dense()) { throw QueryStatusException( - "Cannot set subarray; Setting a subarray is not supported on " + "[set_subarray] Setting a subarray is not supported on " "sparse writes."); } break; default: throw QueryStatusException( - "Cannot set subarray; Setting a subarray is not supported for query " - "type '" + + "[set_subarray] Setting a subarray is not supported for query type " + "'" + query_type_str(type_) + "'."); } // Check the query has not been initialized. if (status_ != tiledb::sm::QueryStatus::UNINITIALIZED) { throw QueryStatusException( - "Cannot set subarray; Setting a subarray on an already initialized " + "[set_subarray] Setting a subarray on an already initialized " "query is not supported."); } @@ -1810,7 +1813,7 @@ Status Query::submit() { if (fragment_size_ != std::numeric_limits::max() && (layout_ != Layout::GLOBAL_ORDER || type_ != QueryType::WRITE)) { throw QueryStatusException( - "Fragment size is only supported for global order writes."); + "[submit] Fragment size is only supported for global order writes."); } // Check attribute/dimensions buffers completeness before query submits diff --git a/tiledb/sm/query/query.h b/tiledb/sm/query/query.h index add3385db21..d809e361ee3 100644 --- a/tiledb/sm/query/query.h +++ b/tiledb/sm/query/query.h @@ -5,7 +5,7 @@ * * The MIT License * - * @copyright Copyright (c) 2017-2022 TileDB, Inc. + * @copyright Copyright (c) 2017-2023 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 @@ -392,12 +392,27 @@ class Query { * * This function overrides the config for Query-level parameters only. * Semantically, the initial query config is copied from the context - * config upon initialization. Note that The context config is immutable + * config upon construction. Note that The context config is immutable * at the C API level because tiledb_ctx_get_config always returns a copy. * * Config parameters set here will *only* be applied within the Query. + * + * @pre The function must be called before Query::init(). */ - Status set_config(const Config& config); + void set_config(const Config& config); + + /** + * Sets the confif for the Query + * + * @param config + * + * @note This is a potentially unsafe operation. Queries should be + * unsubmitted and unfinalized when the config is set. Until C.41 compilance, + * this is necessary for serialization. + */ + inline void unsafe_set_config(const Config& config) { + config_.inherit(config); + } /** * Sets the (zipped) coordinates buffer (set with TILEDB_COORDS as the diff --git a/tiledb/sm/serialization/query.cc b/tiledb/sm/serialization/query.cc index 6fce24e0ea3..2deb4b677c9 100644 --- a/tiledb/sm/serialization/query.cc +++ b/tiledb/sm/serialization/query.cc @@ -5,7 +5,7 @@ * * The MIT License * - * @copyright Copyright (c) 2017-2022 TileDB, Inc. + * @copyright Copyright (c) 2017-2023 TileDB, Inc. * @copyright Copyright (c) 2016 MIT and Intel Corporation * * Permission is hereby granted, free of charge, to any person obtaining a copy @@ -1452,7 +1452,7 @@ Status query_from_capnp( auto config_reader = query_reader.getConfig(); RETURN_NOT_OK(config_from_capnp(config_reader, &decoded_config)); if (decoded_config != nullptr) { - RETURN_NOT_OK(query->set_config(*decoded_config)); + query->unsafe_set_config(*decoded_config); } } diff --git a/tiledb/sm/subarray/subarray.cc b/tiledb/sm/subarray/subarray.cc index b897dbc1930..b8bc798589c 100644 --- a/tiledb/sm/subarray/subarray.cc +++ b/tiledb/sm/subarray/subarray.cc @@ -5,7 +5,7 @@ * * The MIT License * - * @copyright Copyright (c) 2017-2021 TileDB, Inc. + * @copyright Copyright (c) 2017-2023 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 @@ -185,10 +185,10 @@ void Subarray::add_label_range( // A label range has already been set on this dimension. Do the following: // * Check this label is the same label that rangers were already set. if (dim_label_ref.name() != label_range_subset_[dim_idx].value().name) { - throw StatusException(Status_SubarrayError( - "Cannot add label range; Dimension is already to set to use " + throw SubarrayStatusException( + "[add_label_range] Dimension is already to set to use " "dimension label '" + - label_range_subset_[dim_idx].value().name)); + label_range_subset_[dim_idx].value().name + "'"); } } else { // A label range has not yet been set on this dimension. Do the following: @@ -199,9 +199,9 @@ void Subarray::add_label_range( // value of the entire domain). if (range_subset_[dim_label_ref.dimension_index()] .is_explicitly_initialized()) { - throw StatusException(Status_SubarrayError( - "Cannot add label range; Dimension '" + std::to_string(dim_idx) + - "' already has ranges set to it.")); + throw SubarrayStatusException( + "[add_label_range] Dimension '" + std::to_string(dim_idx) + + "' already has ranges set to it."); } label_range_subset_[dim_idx] = LabelRangeSubset(dim_label_ref, coalesce_ranges_); @@ -218,9 +218,9 @@ void Subarray::add_label_range( label_range_subset_[dim_idx].value().ranges.add_range( range, read_range_oob_error); if (!error_status.ok()) { - throw StatusException(Status_SubarrayError( - "Cannot add label range for dimension label '" + dim_label_ref.name() + - "'; " + error_status.message())); + throw SubarrayStatusException( + "[add_label_range] Cannot add label range for dimension label '" + + dim_label_ref.name() + "'; " + error_status.message()); } if (oob_warning.has_value()) { LOG_WARN( @@ -236,21 +236,19 @@ void Subarray::add_label_range( const void* stride) { // Check input range data is valid data. if (start == nullptr || end == nullptr) { - throw StatusException( - Status_SubarrayError("Cannot add label range; Invalid range")); + throw SubarrayStatusException("[add_label_range] Invalid range"); } if (stride != nullptr) { - throw StatusException( - Status_SubarrayError("Cannot add label range; Setting range stride is " - "currently unsupported")); + throw SubarrayStatusException( + "[add_label_range] Setting range stride is currently unsupported"); } // Get dimension label range and check the label is in fact fixed-sized. const auto& dim_label_ref = array_->array_schema_latest().dimension_label(label_name); if (dim_label_ref.label_cell_val_num() == constants::var_num) { - throw StatusException( - Status_SubarrayError("Cannot add label range; Cannot add a fixed-sized " - "range to a variable sized dimension label")); + throw SubarrayStatusException( + "[add_label_range] Cannot add a fixed-sized range to a variable sized " + "dimension label"); } // Add the label range to this subarray. return this->add_label_range( @@ -268,17 +266,16 @@ void Subarray::add_label_range_var( // Check the input range data is valid. if ((start == nullptr && start_size != 0) || (end == nullptr && end_size != 0)) { - throw StatusException( - Status_SubarrayError("Cannot add range; Invalid range")); + throw SubarrayStatusException("[add_label_range_var] Invalid range"); } // Get the dimension label range and check the label is in fact // variable-sized. const auto& dim_label_ref = array_->array_schema_latest().dimension_label(label_name); if (dim_label_ref.label_cell_val_num() != constants::var_num) { - throw StatusException(Status_SubarrayError( - "Cannot add label range; Cannot add a variable-sized range to a " - "fixed-sized dimension label")); + throw SubarrayStatusException( + "[add_label_range_var] Cannot add a variable-sized range to a " + "fixed-sized dimension label"); } // Add the label range to this subarray. return this->add_label_range( @@ -585,8 +582,7 @@ const std::vector& Subarray::get_attribute_ranges( const std::string& Subarray::get_label_name(const uint32_t dim_index) const { if (!has_label_ranges(dim_index)) { - throw SubarrayStatusException( - "Cannot get label name. No label ranges set."); + throw SubarrayStatusException("[get_label_name] No label ranges set"); } return label_range_subset_[dim_index]->name; } @@ -602,9 +598,9 @@ void Subarray::get_label_range( .dimension_index(); if (!label_range_subset_[dim_idx].has_value() || label_range_subset_[dim_idx].value().name != label_name) { - throw StatusException(Status_SubarrayError( - "Cannot get label range; No ranges set on dimension label '" + - label_name + "'")); + throw SubarrayStatusException( + "[get_label_range] No ranges set on dimension label '" + label_name + + "'"); } const auto& range = label_range_subset_[dim_idx].value().ranges[range_idx]; *start = range.start_fixed(); @@ -633,9 +629,9 @@ void Subarray::get_label_range_var( .dimension_index(); if (!label_range_subset_[dim_idx].has_value() || label_range_subset_[dim_idx].value().name != label_name) { - throw StatusException(Status_SubarrayError( - "Cannot get label range; No ranges set on dimension label '" + - label_name + "'")); + throw SubarrayStatusException( + "[get_label_range_var] No ranges set on dimension label '" + + label_name + "'"); } const auto& range = label_range_subset_[dim_idx].value().ranges[range_idx]; std::memcpy(start, range.start_str().data(), range.start_size()); @@ -652,9 +648,9 @@ void Subarray::get_label_range_var_size( .dimension_index(); if (!label_range_subset_[dim_idx].has_value() || label_range_subset_[dim_idx].value().name != label_name) { - throw StatusException(Status_SubarrayError( - "Cannot get label range size; No ranges set on dimension label '" + - label_name + "'")); + throw SubarrayStatusException( + "[get_label_range_var_size] No ranges set on dimension label '" + + label_name + "'"); } const auto& range = label_range_subset_[dim_idx].value().ranges[range_idx]; *start_size = range.start_size(); @@ -901,8 +897,7 @@ bool Subarray::empty(uint32_t dim_idx) const { QueryType Subarray::get_query_type() const { if (array_ == nullptr) - throw StatusException(Status_SubarrayError( - "Cannot get query type from array; Invalid array")); + throw SubarrayStatusException("[get_query_type] Invalid array"); return array_->get_query_type(); } @@ -1091,8 +1086,8 @@ void Subarray::set_layout(Layout layout) { layout_ = layout; } -Status Subarray::set_config(const Config& config) { - config_ = config; +void Subarray::set_config(const Config& config) { + config_.inherit(config); QueryType array_query_type{array_->get_query_type()}; @@ -1101,13 +1096,11 @@ Status Subarray::set_config(const Config& config) { std::string read_range_oob_str = config.get("sm.read_range_oob", &found); assert(found); if (read_range_oob_str != "error" && read_range_oob_str != "warn") - return LOG_STATUS(Status_SubarrayError( - "Invalid value " + read_range_oob_str + - " for sm.read_range_obb. Acceptable values are 'error' or 'warn'.")); + throw SubarrayStatusException( + "[set_config] Invalid value " + read_range_oob_str + + " for sm.read_range_obb. Acceptable values are 'error' or 'warn'."); err_on_range_oob_ = read_range_oob_str == "error"; } - - return Status::Ok(); }; const Config* Subarray::config() const { @@ -1780,14 +1773,14 @@ NDRange Subarray::ndrange(const std::vector& range_coords) const { void Subarray::set_attribute_ranges( const std::string& attr_name, const std::vector& ranges) { if (!array_->array_schema_latest().is_attr(attr_name)) { - throw StatusException(Status_SubarrayError( - "Cannot add ranges; No attribute named " + attr_name + "'.")); + throw SubarrayStatusException( + "[set_attribute_ranges] No attribute named " + attr_name + "'."); } auto search = attr_range_subset_.find(attr_name); if (search != attr_range_subset_.end()) { - throw StatusException(Status_SubarrayError( - "Cannot add ranges; Ranges are already set for attribute '" + - attr_name + "'.")); + throw SubarrayStatusException( + "[set_attribute_ranges] Ranges are already set for attribute '" + + attr_name + "'."); } attr_range_subset_[attr_name] = ranges; } @@ -1799,9 +1792,9 @@ const std::vector& Subarray::ranges_for_label( .dimension_index(); if (!label_range_subset_[dim_idx].has_value() || label_range_subset_[dim_idx].value().name != label_name) { - throw StatusException(Status_SubarrayError( - "Cannot get label ranges; No ranges set on dimension label '" + - label_name + "'")); + throw SubarrayStatusException( + "[ranges_for_label] No ranges set on dimension label '" + label_name + + "'"); } return label_range_subset_[dim_idx]->get_ranges(); } diff --git a/tiledb/sm/subarray/subarray.h b/tiledb/sm/subarray/subarray.h index ce6d265efe4..65162ff9e55 100644 --- a/tiledb/sm/subarray/subarray.h +++ b/tiledb/sm/subarray/subarray.h @@ -5,7 +5,7 @@ * * The MIT License * - * @copyright Copyright (c) 2017-2021 TileDB, Inc. + * @copyright Copyright (c) 2017-2023 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 @@ -240,7 +240,7 @@ class Subarray { /* ********************************* */ /** Sets config for query-level parameters only. */ - Status set_config(const Config& config); + void set_config(const Config& config); /** * Get the config of the writer From 00d2041eb4b026adaffa129510a8956e03e81d93 Mon Sep 17 00:00:00 2001 From: Rebekah Date: Fri, 10 Feb 2023 18:25:59 -0500 Subject: [PATCH 2/2] Fix spelling mistake --- tiledb/sm/query/query.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tiledb/sm/query/query.h b/tiledb/sm/query/query.h index d809e361ee3..c17f46e0ec7 100644 --- a/tiledb/sm/query/query.h +++ b/tiledb/sm/query/query.h @@ -402,7 +402,7 @@ class Query { void set_config(const Config& config); /** - * Sets the confif for the Query + * Sets the config for the Query * * @param config *