Skip to content

Commit

Permalink
Fixup query condition set membership creation API
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 committed Sep 28, 2023
1 parent 9d79edd commit 9ff5ad0
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 9ff5ad0

Please sign in to comment.