Skip to content

Commit

Permalink
Fixup query condition set membership creation API (#4390)
Browse files Browse the repository at this point in the history
During review there was some confusion over having a default of
`TILEDB_IN` when creating set membership query conditions. The confusion
also prompted me to realize that we may have other types of query
conditions in the future that take a list of values that are not set
membership related which makes having the `TILEDB_IN` default even less
well designed.

Unfortunately, I forgot to remove the default from the std::string
version of that function. @eddelbuettel also kindly pointed out that I
forgot to document the `op` paramter which I've also added.
  • Loading branch information
davisp authored Sep 29, 2023
1 parent 9d79edd commit 35df03d
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 12 deletions.
32 changes: 21 additions & 11 deletions test/src/unit-cppapi-query-condition-sets.cc
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,8 @@ TEST_CASE_METHOD(
create_array(type, serialize);

std::vector<std::string> values = {"barney", "wilma"};
auto qc = QueryConditionExperimental::create(ctx_, "attr2", values);
auto qc =
QueryConditionExperimental::create(ctx_, "attr2", values, TILEDB_IN);

check_read(qc, [](const QCSetsCell& c) {
return (c.a2 == "barney" || c.a2 == "wilma");
Expand All @@ -186,7 +187,8 @@ TEST_CASE_METHOD(
create_array(type, serialize);

std::vector<std::string> values = {"hack", "pack"};
auto qc = QueryConditionExperimental::create(ctx_, "attr5", values);
auto qc =
QueryConditionExperimental::create(ctx_, "attr5", values, TILEDB_IN);

check_read(qc, [](const QCSetsCell& c) {
return (c.a5 == "hack" || c.a5 == "pack");
Expand Down Expand Up @@ -218,7 +220,8 @@ TEST_CASE_METHOD(
create_array(type, serialize);

std::vector<std::string> values = {"wilma", "betty"};
auto qc = QueryConditionExperimental::create(ctx_, "attr6", values);
auto qc =
QueryConditionExperimental::create(ctx_, "attr6", values, TILEDB_IN);

check_read(qc, [](const QCSetsCell& c) { return (c.a6 == 1 || c.a6 == 3); });
}
Expand All @@ -233,7 +236,8 @@ TEST_CASE_METHOD(
create_array(type, serialize);

std::vector<std::string> values = {"blue", "umber"};
auto qc = QueryConditionExperimental::create(ctx_, "attr3", values);
auto qc =
QueryConditionExperimental::create(ctx_, "attr3", values, TILEDB_IN);

check_read(qc, [](const QCSetsCell& c) {
return (c.a3 == "blue" || c.a3 == "umber");
Expand Down Expand Up @@ -348,7 +352,8 @@ TEST_CASE_METHOD(
create_array(type, serialize);

std::vector<std::string> values = {"wilma"};
auto qc1 = QueryConditionExperimental::create(ctx_, "attr2", values);
auto qc1 =
QueryConditionExperimental::create(ctx_, "attr2", values, TILEDB_IN);
auto qc2 = qc1.negate();

check_read(qc2, [](const QCSetsCell& c) { return !(c.a2 == "wilma"); });
Expand Down Expand Up @@ -383,7 +388,8 @@ TEST_CASE_METHOD(
create_array(type, serialize);

std::vector<std::string> values = {"wilma", "betty"};
auto qc1 = QueryConditionExperimental::create(ctx_, "attr2", values);
auto qc1 =
QueryConditionExperimental::create(ctx_, "attr2", values, TILEDB_IN);
auto qc2 = QueryCondition::create(ctx_, "attr1", 2.0f, TILEDB_GT);
auto qc3 = qc1.combine(qc2, TILEDB_AND);

Expand All @@ -402,7 +408,8 @@ TEST_CASE_METHOD(
create_array(type, serialize);

std::vector<std::string> values = {"wilma", "betty"};
auto qc1 = QueryConditionExperimental::create(ctx_, "attr2", values);
auto qc1 =
QueryConditionExperimental::create(ctx_, "attr2", values, TILEDB_IN);
auto qc2 = QueryCondition::create(ctx_, "attr1", 3.0f, TILEDB_EQ);
auto qc3 = qc1.combine(qc2, TILEDB_OR);

Expand All @@ -420,11 +427,13 @@ TEST_CASE_METHOD(
create_array(type, serialize);

std::vector<std::string> del_values = {"wilma"};
auto del_qc = QueryConditionExperimental::create(ctx_, "attr2", del_values);
auto del_qc =
QueryConditionExperimental::create(ctx_, "attr2", del_values, TILEDB_IN);
write_delete(del_qc);

std::vector<std::string> values = {"wilma", "betty"};
auto qc = QueryConditionExperimental::create(ctx_, "attr2", values);
auto qc =
QueryConditionExperimental::create(ctx_, "attr2", values, TILEDB_IN);

check_read(qc, [](const QCSetsCell& c) {
// Every instance of "wilma" was deleted so we only expect "betty"
Expand All @@ -442,7 +451,7 @@ TEST_CASE_METHOD(
create_array(type, serialize);

std::vector<std::string> values = {"", "foo"};
auto qc = QueryConditionExperimental::create(ctx_, "dim", values);
auto qc = QueryConditionExperimental::create(ctx_, "dim", values, TILEDB_IN);

REQUIRE_THROWS(check_read(qc, [](const QCSetsCell&) -> bool {
throw std::logic_error("Shouldn't get here.");
Expand All @@ -459,7 +468,8 @@ TEST_CASE_METHOD(
create_array(type, serialize);

std::vector<std::string> values = {"oh", "hi"};
auto qc = QueryConditionExperimental::create(ctx_, "attr5", values);
auto qc =
QueryConditionExperimental::create(ctx_, "attr5", values, TILEDB_IN);

REQUIRE_THROWS(check_read(qc, [](const QCSetsCell&) -> bool {
throw std::logic_error("Shouldn't get here.");
Expand Down
6 changes: 5 additions & 1 deletion tiledb/sm/cpp_api/query_condition_experimental.h
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ class QueryConditionExperimental {
* @param ctx The TileDB context.
* @param field_name The field name.
* @param values The set membership values to use.
* @param op The query condition operator to use. Currently limited to
* TILEDB_IN and TILEDB_NOT_IN.
*/
template <typename T, impl::enable_trivial<T>* = nullptr>
static QueryCondition create(
Expand Down Expand Up @@ -95,13 +97,15 @@ class QueryConditionExperimental {
* @param ctx The TileDB context.
* @param field_name The field name.
* @param values The set membership values to use.
* @param op The query condition operator to use. Currently limited to
* TILEDB_IN and TILEDB_NOT_IN.
*/
template <typename T, impl::enable_trivial<T>* = nullptr>
static QueryCondition create(
const Context& ctx,
const std::string& field_name,
const std::vector<std::basic_string<T>>& values,
tiledb_query_condition_op_t op = TILEDB_IN) {
tiledb_query_condition_op_t op) {
std::vector<uint8_t> data;
std::vector<uint64_t> offsets;

Expand Down

0 comments on commit 35df03d

Please sign in to comment.