From 07ddbb486dd686f6fd020e4f8c976b31ea3560e8 Mon Sep 17 00:00:00 2001 From: Luc Rancourt Date: Fri, 1 Jul 2022 03:54:17 +0300 Subject: [PATCH] Addressing feedback from @ihnorton, part 2. --- .../sm/enums/query_condition_combination_op.h | 14 +++ tiledb/sm/enums/query_condition_op.h | 25 +++++ tiledb/sm/query/ast/query_ast.cc | 71 ++------------ tiledb/sm/query/ast/query_ast.h | 97 +++++++++++++++++-- tiledb/sm/query/ast/test/unit-query-ast.cc | 21 +++- tiledb/sm/query/query_condition.cc | 2 +- 6 files changed, 156 insertions(+), 74 deletions(-) diff --git a/tiledb/sm/enums/query_condition_combination_op.h b/tiledb/sm/enums/query_condition_combination_op.h index 7049ef6f423..51f1a8488c5 100644 --- a/tiledb/sm/enums/query_condition_combination_op.h +++ b/tiledb/sm/enums/query_condition_combination_op.h @@ -113,6 +113,20 @@ inline void ensure_qc_combo_op_string_is_valid( ensure_qc_combo_op_is_valid(qc_combo_op); } +inline QueryConditionCombinationOp negate_qc_combination_op( + const QueryConditionCombinationOp op) { + switch (op) { + case QueryConditionCombinationOp::AND: + return QueryConditionCombinationOp::OR; + + case QueryConditionCombinationOp::OR: + return QueryConditionCombinationOp::AND; + + default: + throw std::runtime_error("negate_qc_combination_op: Invalid op."); + } +} + } // namespace sm } // namespace tiledb diff --git a/tiledb/sm/enums/query_condition_op.h b/tiledb/sm/enums/query_condition_op.h index 57a282b2f00..343ba2969da 100644 --- a/tiledb/sm/enums/query_condition_op.h +++ b/tiledb/sm/enums/query_condition_op.h @@ -113,6 +113,31 @@ inline void ensure_qc_op_string_is_valid(const std::string& qc_op_str) { ensure_qc_op_is_valid(qc_op); } +inline QueryConditionOp negate_query_condition_op(const QueryConditionOp op) { + switch (op) { + case QueryConditionOp::LT: + return QueryConditionOp::GE; + + case QueryConditionOp::GT: + return QueryConditionOp::LE; + + case QueryConditionOp::GE: + return QueryConditionOp::LT; + + case QueryConditionOp::LE: + return QueryConditionOp::GT; + + case QueryConditionOp::NE: + return QueryConditionOp::EQ; + + case QueryConditionOp::EQ: + return QueryConditionOp::NE; + + default: + throw std::runtime_error("negate_query_condition_op: Invalid op."); + } +} + } // namespace sm } // namespace tiledb diff --git a/tiledb/sm/query/ast/query_ast.cc b/tiledb/sm/query/ast/query_ast.cc index 8411afb7d76..ba64f6b34ed 100644 --- a/tiledb/sm/query/ast/query_ast.cc +++ b/tiledb/sm/query/ast/query_ast.cc @@ -41,45 +41,12 @@ bool ASTNodeVal::is_expr() const { return false; } -tdb_unique_ptr ASTNodeVal::clone(bool negate) const { - auto op = op_; - if (negate) { - switch (op) { - case QueryConditionOp::LT: - op = QueryConditionOp::GE; - break; - - case QueryConditionOp::GT: - op = QueryConditionOp::LE; - break; - - case QueryConditionOp::GE: - op = QueryConditionOp::LT; - break; - - case QueryConditionOp::LE: - op = QueryConditionOp::GT; - break; - - case QueryConditionOp::NE: - op = QueryConditionOp::EQ; - break; - - case QueryConditionOp::EQ: - op = QueryConditionOp::NE; - break; - - default: - throw std::runtime_error("ASTNodeExpr::negate: Invalid op."); - } - } +tdb_unique_ptr ASTNodeVal::clone() const { + return tdb_unique_ptr(tdb_new(ASTNodeVal, *this)); +} - return tdb_unique_ptr(tdb_new( - ASTNodeVal, - field_name_, - condition_value_data_.data(), - condition_value_data_.size(), - op)); +tdb_unique_ptr ASTNodeVal::get_negated_tree() const { + return tdb_unique_ptr(tdb_new(ASTNodeVal, *this, ASTNegation)); } void ASTNodeVal::get_field_names( @@ -220,30 +187,12 @@ bool ASTNodeExpr::is_expr() const { return true; } -tdb_unique_ptr ASTNodeExpr::clone(bool negate) const { - std::vector> nodes_copy; - for (const auto& node : nodes_) { - nodes_copy.push_back(node->clone(negate)); - } - - auto combination_op = combination_op_; - if (negate) { - switch (combination_op) { - case QueryConditionCombinationOp::AND: - combination_op = QueryConditionCombinationOp::OR; - break; - - case QueryConditionCombinationOp::OR: - combination_op = QueryConditionCombinationOp::AND; - break; - - default: - throw std::runtime_error("ASTNodeExpr::negate: Invalid op."); - } - } +tdb_unique_ptr ASTNodeExpr::clone() const { + return tdb_unique_ptr(tdb_new(ASTNodeExpr, *this)); +} - return tdb_unique_ptr( - tdb_new(ASTNodeExpr, std::move(nodes_copy), combination_op)); +tdb_unique_ptr ASTNodeExpr::get_negated_tree() const { + return tdb_unique_ptr(tdb_new(ASTNodeExpr, *this, ASTNegation)); } void ASTNodeExpr::get_field_names( diff --git a/tiledb/sm/query/ast/query_ast.h b/tiledb/sm/query/ast/query_ast.h index 512b33bd3fe..069f607fe33 100644 --- a/tiledb/sm/query/ast/query_ast.h +++ b/tiledb/sm/query/ast/query_ast.h @@ -52,6 +52,9 @@ using namespace tiledb::common; namespace tiledb { namespace sm { +class ASTNegationT {}; +static constexpr ASTNegationT ASTNegation{}; + /** * @brief The ASTNode class is an abstract class that contains virtual * methods used by both the value and expression node implementation @@ -69,12 +72,19 @@ class ASTNode { /** * @brief ASTNode class method used in the QueryCondition copy constructor - * ASTNode::combine, and testing that returns a copy of the caller - * node. + * ASTNode::combine, and testing that returns a copy of the caller node. * * @return tdb_unique_ptr A deep copy of the ASTNode. */ - virtual tdb_unique_ptr clone(bool negate = false) const = 0; + virtual tdb_unique_ptr clone() const = 0; + + /** + * @brief ASTNode class method used in the QueryCondition that returns a copy + * of the caller node, but negated. + * + * @return tdb_unique_ptr A negated deep copy of the ASTNode. + */ + virtual tdb_unique_ptr get_negated_tree() const = 0; /** * @brief Gets the set of field names from all the value nodes in the ASTNode. @@ -205,13 +215,39 @@ class ASTNodeVal : public ASTNode { } }; + /** + * @brief Copy constructor. + */ + ASTNodeVal(const ASTNodeVal& rhs) + : field_name_(rhs.field_name_) + , condition_value_data_(rhs.condition_value_data_) + , condition_value_view_( + (rhs.condition_value_view_.content() != nullptr && + rhs.condition_value_view_.size() == 0 ? + (void*)"" : + condition_value_data_.data()), + condition_value_data_.size()) + , op_(rhs.op_) { + } + + ASTNodeVal(const ASTNodeVal& rhs, ASTNegationT) + : field_name_(rhs.field_name_) + , condition_value_data_(rhs.condition_value_data_) + , condition_value_view_( + (rhs.condition_value_view_.content() != nullptr && + rhs.condition_value_view_.size() == 0 ? + (void*)"" : + condition_value_data_.data()), + condition_value_data_.size()) + , op_(negate_query_condition_op(rhs.op_)) { + } + /** * @brief Default destructor of ASTNodeVal. */ ~ASTNodeVal() { } - DISABLE_COPY(ASTNodeVal); DISABLE_MOVE(ASTNodeVal); DISABLE_COPY_ASSIGN(ASTNodeVal); DISABLE_MOVE_ASSIGN(ASTNodeVal); @@ -226,12 +262,19 @@ class ASTNodeVal : public ASTNode { /** * @brief ASTNode class method used in the QueryCondition copy constructor - * ASTNode::combine, and testing that returns a copy of the caller - * node. + * ASTNode::combine, and testing that returns a copy of the caller node. * * @return tdb_unique_ptr A deep copy of the ASTNode. */ - tdb_unique_ptr clone(bool negate = false) const override; + tdb_unique_ptr clone() const override; + + /** + * @brief ASTNode class method used in the QueryCondition that returns a copy + * of the caller node, but negated. + * + * @return tdb_unique_ptr A negated deep copy of the ASTNode. + */ + tdb_unique_ptr get_negated_tree() const override; /** * @brief Gets the set of field names from all the value nodes in the ASTNode. @@ -355,13 +398,41 @@ class ASTNodeExpr : public ASTNode { , combination_op_(c_op) { } + /** + * @brief Copy constructor. + */ + ASTNodeExpr(const ASTNodeExpr& rhs) + : combination_op_(rhs.combination_op_) { + for (auto& node : rhs.nodes_) { + if (node->is_expr()) { + nodes_.emplace_back( + tdb_new(ASTNodeExpr, *static_cast(node.get()))); + } else { + nodes_.emplace_back( + tdb_new(ASTNodeVal, *static_cast(node.get()))); + } + } + } + + ASTNodeExpr(const ASTNodeExpr& rhs, ASTNegationT) + : combination_op_(negate_qc_combination_op(rhs.combination_op_)) { + for (auto& node : rhs.nodes_) { + if (node->is_expr()) { + nodes_.emplace_back(tdb_new( + ASTNodeExpr, *static_cast(node.get()), ASTNegation)); + } else { + nodes_.emplace_back(tdb_new( + ASTNodeVal, *static_cast(node.get()), ASTNegation)); + } + } + } + /** * @brief Default destructor of ASTNodeExpr. */ ~ASTNodeExpr() { } - DISABLE_COPY(ASTNodeExpr); DISABLE_MOVE(ASTNodeExpr); DISABLE_COPY_ASSIGN(ASTNodeExpr); DISABLE_MOVE_ASSIGN(ASTNodeExpr); @@ -381,7 +452,15 @@ class ASTNodeExpr : public ASTNode { * * @return tdb_unique_ptr A deep copy of the ASTNode. */ - tdb_unique_ptr clone(bool negate = false) const override; + tdb_unique_ptr clone() const override; + + /** + * @brief ASTNode class method used in the QueryCondition that returns a copy + * of the caller node, but negated. + * + * @return tdb_unique_ptr A negated deep copy of the ASTNode. + */ + tdb_unique_ptr get_negated_tree() const override; /** * @brief Gets the set of field names from all the value nodes in the ASTNode. diff --git a/tiledb/sm/query/ast/test/unit-query-ast.cc b/tiledb/sm/query/ast/test/unit-query-ast.cc index 8019280b110..08adf862ce4 100644 --- a/tiledb/sm/query/ast/test/unit-query-ast.cc +++ b/tiledb/sm/query/ast/test/unit-query-ast.cc @@ -59,7 +59,12 @@ tdb_unique_ptr test_value_node( } // Test ASTNode::clone on the constructed node. - auto node_val_clone = node_val->clone(negate); + tdb_unique_ptr node_val_clone; + if (negate) { + node_val_clone = node_val->get_negated_tree(); + } else { + node_val_clone = node_val->clone(); + } CHECK(ast_node_to_str(node_val_clone) == expected_result); return node_val; @@ -82,7 +87,12 @@ tdb_unique_ptr test_string_value_node( } // Test ASTNode::clone on the constructed node. - auto node_val_clone = node_val->clone(negate); + tdb_unique_ptr node_val_clone; + if (negate) { + node_val_clone = node_val->get_negated_tree(); + } else { + node_val_clone = node_val->clone(); + } CHECK(ast_node_to_str(node_val_clone) == expected_result); return node_val; @@ -102,7 +112,12 @@ tdb_unique_ptr test_expression_node( } // Test ASTNode::clone on the constructed node. - auto combined_node_clone = combined_node->clone(negate); + tdb_unique_ptr combined_node_clone; + if (negate) { + combined_node_clone = combined_node->get_negated_tree(); + } else { + combined_node_clone = combined_node->clone(); + } CHECK(ast_node_to_str(combined_node_clone) == expected_result); return combined_node; diff --git a/tiledb/sm/query/query_condition.cc b/tiledb/sm/query/query_condition.cc index cef84b999fb..fa68284ee7e 100644 --- a/tiledb/sm/query/query_condition.cc +++ b/tiledb/sm/query/query_condition.cc @@ -1897,7 +1897,7 @@ Status QueryCondition::apply_sparse( } QueryCondition QueryCondition::negated_condition() { - return QueryCondition(tree_->clone(true)); + return QueryCondition(tree_->get_negated_tree()); } const tdb_unique_ptr& QueryCondition::ast() const {