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

Error out no-op set_config calls: Query #3884

Merged
merged 3 commits into from
Apr 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions test/src/unit-cppapi-partial-attribute-write.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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();

Expand Down
4 changes: 2 additions & 2 deletions tiledb/sm/c_api/tiledb.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1394,7 +1394,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;
}

Expand Down Expand Up @@ -2086,7 +2086,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;
}

Expand Down
2 changes: 1 addition & 1 deletion tiledb/sm/cpp_api/query.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
49 changes: 26 additions & 23 deletions tiledb/sm/query/query.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -130,7 +130,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<Buffer>(HERE());

Expand Down Expand Up @@ -984,7 +984,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");
}
}
}
Expand Down Expand Up @@ -1231,8 +1232,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) {
Expand All @@ -1242,9 +1247,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) {
Expand Down Expand Up @@ -1299,8 +1302,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
Expand Down Expand Up @@ -1417,21 +1419,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.
Expand Down Expand Up @@ -1686,23 +1689,23 @@ 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;

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.");
}

Expand All @@ -1729,22 +1732,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.");
}

Expand Down Expand Up @@ -1845,7 +1848,7 @@ Status Query::submit() {
if (fragment_size_ != std::numeric_limits<uint64_t>::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
Expand Down
21 changes: 18 additions & 3 deletions tiledb/sm/query/query.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -402,12 +402,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().
*/
void set_config(const Config& config);

/**
* Sets the config 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.
*/
Status set_config(const Config& config);
inline void unsafe_set_config(const Config& config) {
config_.inherit(config);
}

/**
* Sets the (zipped) coordinates buffer (set with TILEDB_COORDS as the
Expand Down
4 changes: 2 additions & 2 deletions tiledb/sm/serialization/query.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -1486,7 +1486,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);
}
}

Expand Down
Loading