From 35df03d3ab6ca991d9d60d2a46caa182ddbd9bff Mon Sep 17 00:00:00 2001 From: "Paul J. Davis" Date: Thu, 28 Sep 2023 20:10:43 -0500 Subject: [PATCH] Fixup query condition set membership creation API (#4390) 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. --- test/src/unit-cppapi-query-condition-sets.cc | 32 ++++++++++++------- .../sm/cpp_api/query_condition_experimental.h | 6 +++- 2 files changed, 26 insertions(+), 12 deletions(-) diff --git a/test/src/unit-cppapi-query-condition-sets.cc b/test/src/unit-cppapi-query-condition-sets.cc index 4d6407cdc85..7962c10d681 100644 --- a/test/src/unit-cppapi-query-condition-sets.cc +++ b/test/src/unit-cppapi-query-condition-sets.cc @@ -169,7 +169,8 @@ TEST_CASE_METHOD( create_array(type, serialize); std::vector 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"); @@ -186,7 +187,8 @@ TEST_CASE_METHOD( create_array(type, serialize); std::vector 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"); @@ -218,7 +220,8 @@ TEST_CASE_METHOD( create_array(type, serialize); std::vector 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); }); } @@ -233,7 +236,8 @@ TEST_CASE_METHOD( create_array(type, serialize); std::vector 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"); @@ -348,7 +352,8 @@ TEST_CASE_METHOD( create_array(type, serialize); std::vector 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"); }); @@ -383,7 +388,8 @@ TEST_CASE_METHOD( create_array(type, serialize); std::vector 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); @@ -402,7 +408,8 @@ TEST_CASE_METHOD( create_array(type, serialize); std::vector 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); @@ -420,11 +427,13 @@ TEST_CASE_METHOD( create_array(type, serialize); std::vector 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 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" @@ -442,7 +451,7 @@ TEST_CASE_METHOD( create_array(type, serialize); std::vector 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."); @@ -459,7 +468,8 @@ TEST_CASE_METHOD( create_array(type, serialize); std::vector 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."); diff --git a/tiledb/sm/cpp_api/query_condition_experimental.h b/tiledb/sm/cpp_api/query_condition_experimental.h index 04dd107b403..f68d2364f8f 100644 --- a/tiledb/sm/cpp_api/query_condition_experimental.h +++ b/tiledb/sm/cpp_api/query_condition_experimental.h @@ -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 * = nullptr> static QueryCondition create( @@ -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 * = nullptr> static QueryCondition create( const Context& ctx, const std::string& field_name, const std::vector>& values, - tiledb_query_condition_op_t op = TILEDB_IN) { + tiledb_query_condition_op_t op) { std::vector data; std::vector offsets;