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

Pd/experiment/windows segfault 4 #15

Closed
wants to merge 2 commits into from
Closed
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
71 changes: 61 additions & 10 deletions test/src/unit-cppapi-enumerations.cc
Original file line number Diff line number Diff line change
Expand Up @@ -522,7 +522,7 @@ TEST_CASE_METHOD(

TEST_CASE_METHOD(
CPPEnumerationFx,
"CPP: Enumeration Query - Invalid Enumeration Value",
"CPP: Enumeration Query - Invalid Enumeration Value is Always False",
"[enumeration][query][basic]") {
create_array();

Expand All @@ -542,10 +542,64 @@ TEST_CASE_METHOD(
.set_data_buffer("attr1", attr1)
.set_condition(qc);

// Check that the error message is helpful to users.
auto matcher = Catch::Matchers::ContainsSubstring(
"Enumeration value not found for field 'attr1'");
REQUIRE_THROWS_WITH(query.submit(), matcher);
REQUIRE_NOTHROW(query.submit());

std::vector<int> dim_expect = {1, 2, 3, 4, 5};
std::vector<int> attr1_expect = {
INT32_MIN, INT32_MIN, INT32_MIN, INT32_MIN, INT32_MIN};

REQUIRE(dim == dim_expect);
REQUIRE(attr1 == attr1_expect);
}

TEST_CASE_METHOD(
CPPEnumerationFx,
"CPP: Enumeration Query - Invalid Enumeration Value Accepted by EQ",
"[enumeration][query][basic]") {
create_array();

// Attempt to query with an enumeration value that isn't in the Enumeration
QueryCondition qc(ctx_);
qc.init("attr1", "alf", 3, TILEDB_EQ);

// Execute the query condition against the array
std::vector<int> dim(5);
std::vector<int> attr1(5);

auto array = Array(ctx_, uri_, TILEDB_READ);
Query query(ctx_, array);
query.add_range("dim", 1, 5)
.set_layout(TILEDB_ROW_MAJOR)
.set_data_buffer("dim", dim)
.set_data_buffer("attr1", attr1)
.set_condition(qc);

CHECK_NOTHROW(query.submit());
}

TEST_CASE_METHOD(
CPPEnumerationFx,
"CPP: Enumeration Query - Invalid Enumeration Value Accepted by IN",
"[enumeration][query][basic]") {
create_array();

// Attempt to query with an enumeration value that isn't in the Enumeration
std::vector<std::string> vals = {"alf", "fred"};
auto qc = QueryConditionExperimental::create(ctx_, "attr1", vals, TILEDB_IN);

// Execute the query condition against the array
std::vector<int> dim(5);
std::vector<int> attr1(5);

auto array = Array(ctx_, uri_, TILEDB_READ);
Query query(ctx_, array);
query.add_range("dim", 1, 5)
.set_layout(TILEDB_ROW_MAJOR)
.set_data_buffer("dim", dim)
.set_data_buffer("attr1", attr1)
.set_condition(qc);

CHECK_NOTHROW(query.submit());
}

TEST_CASE_METHOD(
Expand All @@ -572,7 +626,7 @@ TEST_CASE_METHOD(
TEST_CASE_METHOD(
CPPEnumerationFx,
"CPP: Enumeration Query - Attempt to query on empty enumeration",
"[enumeration][query][error]") {
"[enumeration][query][empty-results]") {
create_array(true);

// Attempt to query with an enumeration value that isn't in the Enumeration
Expand All @@ -591,10 +645,7 @@ TEST_CASE_METHOD(
.set_data_buffer("attr3", attr3)
.set_condition(qc);

// Check that the error message is helpful to users.
auto matcher = Catch::Matchers::ContainsSubstring(
"Enumeration value not found for field 'attr3'");
REQUIRE_THROWS_WITH(query.submit(), matcher);
REQUIRE_NOTHROW(query.submit());
}

CPPEnumerationFx::CPPEnumerationFx()
Expand Down
20 changes: 16 additions & 4 deletions tiledb/sm/c_api/tiledb.h
Original file line number Diff line number Diff line change
Expand Up @@ -95,10 +95,22 @@ typedef enum {

/** Query condition operator. */
typedef enum {
/** Helper macro for defining query condition operator enums. */
#define TILEDB_QUERY_CONDITION_OP_ENUM(id) TILEDB_##id
#include "tiledb_enum.h"
#undef TILEDB_QUERY_CONDITION_OP_ENUM
/** Less-than operator */
TILEDB_LT = 0,
/** Less-than-or-equal operator */
TILEDB_LE = 1,
/** Greater-than operator */
TILEDB_GT = 2,
/** Greater-than-or-equal operator */
TILEDB_GE = 3,
/** Equal operator */
TILEDB_EQ = 4,
/** Not-equal operator */
TILEDB_NE = 5,
/** IN set membership operator. */
TILEDB_IN = 6,
/** NOT IN set membership operator. */
TILEDB_NOT_IN = 7,
} tiledb_query_condition_op_t;

/** Query condition combination operator. */
Expand Down
7 changes: 7 additions & 0 deletions tiledb/sm/c_api/tiledb_enum.h
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,9 @@
TILEDB_QUERY_STATUS_DETAILS_ENUM(REASON_MEMORY_BUDGET) = 2,
#endif

// This enumeration is special in that if you add enumeration entries here
// you have to manually add the new values in tiledb.h. This is to avoid
// exposing `TILEDB_ALWAYS_TRUE` and `TILEDB_ALWAYS_FALSE` in the public API.
#ifdef TILEDB_QUERY_CONDITION_OP_ENUM
/** Less-than operator */
TILEDB_QUERY_CONDITION_OP_ENUM(LT) = 0,
Expand All @@ -99,6 +102,10 @@
TILEDB_QUERY_CONDITION_OP_ENUM(IN) = 6,
/** NOT IN set membership operator. */
TILEDB_QUERY_CONDITION_OP_ENUM(NOT_IN) = 7,
/** ALWAYS TRUE operator. */
TILEDB_QUERY_CONDITION_OP_ENUM(ALWAYS_TRUE) = 253,
/** ALWAYS TRUE operator. */
TILEDB_QUERY_CONDITION_OP_ENUM(ALWAYS_FALSE) = 254,
#endif

#ifdef TILEDB_QUERY_CONDITION_COMBINATION_OP_ENUM
Expand Down
23 changes: 22 additions & 1 deletion tiledb/sm/enums/query_condition_op.h
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,10 @@ inline const std::string& query_condition_op_str(
return constants::query_condition_op_in_str;
case QueryConditionOp::NOT_IN:
return constants::query_condition_op_not_in_str;
case QueryConditionOp::ALWAYS_TRUE:
return constants::query_condition_op_always_true_str;
case QueryConditionOp::ALWAYS_FALSE:
return constants::query_condition_op_always_false_str;
default:
return constants::empty_str;
}
Expand All @@ -105,6 +109,13 @@ inline Status query_condition_op_enum(
} else if (
query_condition_op_str == constants::query_condition_op_not_in_str) {
*query_condition_op = QueryConditionOp::NOT_IN;
} else if (
query_condition_op_str == constants::query_condition_op_always_true_str) {
*query_condition_op = QueryConditionOp::ALWAYS_TRUE;
} else if (
query_condition_op_str ==
constants::query_condition_op_always_false_str) {
*query_condition_op = QueryConditionOp::ALWAYS_FALSE;
} else {
return Status_Error("Invalid QueryConditionOp " + query_condition_op_str);
}
Expand All @@ -113,7 +124,7 @@ inline Status query_condition_op_enum(

inline void ensure_qc_op_is_valid(QueryConditionOp query_condition_op) {
auto qc_op_enum{::stdx::to_underlying(query_condition_op)};
if (qc_op_enum > 7) {
if (qc_op_enum > 7 && qc_op_enum != 253 && qc_op_enum != 254) {
throw std::runtime_error(
"Invalid Query Condition Op " + std::to_string(qc_op_enum));
}
Expand Down Expand Up @@ -156,6 +167,16 @@ inline QueryConditionOp negate_query_condition_op(const QueryConditionOp op) {
case QueryConditionOp::NOT_IN:
return QueryConditionOp::IN;

// ALWAYS_TRUE and ALWAYS_FALSE are the result of QueryCondition rewriting
// which means they should not be available for negation. This saves us
// from having to have invertible and non-invertible versions of these
// operations.
case QueryConditionOp::ALWAYS_TRUE:
throw std::logic_error("Invalid negation of rewritten query.");

case QueryConditionOp::ALWAYS_FALSE:
throw std::logic_error("Invalid negation of rewritten query.");

default:
throw std::runtime_error("negate_query_condition_op: Invalid op.");
}
Expand Down
6 changes: 6 additions & 0 deletions tiledb/sm/misc/constants.cc
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,12 @@ const std::string query_status_initialized_str = "INITIALIZED";
/** TILEDB_UNINITIALIZED Query String **/
const std::string query_status_uninitialized_str = "UNINITIALIZED";

/** TILEDB_ALWAYS_TRUE Query Condition Op String **/
const std::string query_condition_op_always_true_str = "ALWAYS_TRUE";

/** TILEDB_ALWAYS_FALSE Query Condition Op String **/
const std::string query_condition_op_always_false_str = "ALWAYS_FALSE";

/** TILEDB_LT Query Condition Op String **/
const std::string query_condition_op_lt_str = "LT";

Expand Down
6 changes: 6 additions & 0 deletions tiledb/sm/misc/constants.h
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,12 @@ extern const std::string query_status_initialized_str;
/** TILEDB_UNINITIALIZED Query String **/
extern const std::string query_status_uninitialized_str;

/** TILEDB_ALWAYS_TRUE Query Condition Op String **/
extern const std::string query_condition_op_always_true_str;

/** TILEDB_ALWAYS_FALSE Query Condition Op String **/
extern const std::string query_condition_op_always_false_str;

/** TILEDB_LT Query Condition Op String **/
extern const std::string query_condition_op_lt_str;

Expand Down
31 changes: 20 additions & 11 deletions tiledb/sm/query/ast/query_ast.cc
Original file line number Diff line number Diff line change
Expand Up @@ -199,12 +199,17 @@ void ASTNodeVal::rewrite_enumeration_conditions(
if (op_ != QueryConditionOp::IN && op_ != QueryConditionOp::NOT_IN) {
auto idx = enumeration->index_of(get_value_ptr(), get_value_size());
if (idx == constants::enumeration_missing_value) {
throw std::invalid_argument(
"Enumeration value not found for field '" + attr->name() + "'");
if (op_ == QueryConditionOp::NE) {
op_ = QueryConditionOp::ALWAYS_TRUE;
} else {
op_ = QueryConditionOp::ALWAYS_FALSE;
}
data_ = ByteVecValue(val_size);
utils::safe_integral_cast_to_datatype(0, attr->type(), data_);
} else {
data_ = ByteVecValue(val_size);
utils::safe_integral_cast_to_datatype(idx, attr->type(), data_);
}

data_ = ByteVecValue(val_size);
utils::safe_integral_cast_to_datatype(idx, attr->type(), data_);
} else {
// Buffers and writers for the new data/offsets memory
std::vector<uint8_t> data_buffer(val_size * members_.size());
Expand All @@ -215,25 +220,29 @@ void ASTNodeVal::rewrite_enumeration_conditions(

ByteVecValue curr_data(val_size);
uint64_t curr_offset = 0;
uint64_t num_offsets = 0;

for (auto& member : members_) {
auto idx = enumeration->index_of(member.data(), member.size());
if (idx == constants::enumeration_missing_value) {
throw std::invalid_argument(
"Enumeration value not found for field '" + attr->name() + "'");
continue;
}

utils::safe_integral_cast_to_datatype(idx, attr->type(), curr_data);
data_writer.write(curr_data.data(), curr_data.size());
offsets_writer.write(curr_offset);
curr_offset += val_size;
num_offsets += 1;
}

data_ = ByteVecValue(data_buffer.size());
std::memcpy(data_.data(), data_buffer.data(), data_buffer.size());
auto total_data_size = curr_offset;
auto total_offsets_size = num_offsets * constants::cell_var_offset_size;

data_ = ByteVecValue(total_data_size);
std::memcpy(data_.data(), data_buffer.data(), total_data_size);

offsets_ = ByteVecValue(offsets_buffer.size());
std::memcpy(offsets_.data(), offsets_buffer.data(), offsets_buffer.size());
offsets_ = ByteVecValue(total_offsets_size);
std::memcpy(offsets_.data(), offsets_buffer.data(), total_offsets_size);

generate_members();
}
Expand Down
Loading
Loading