Skip to content

Commit

Permalink
Addressing feedback from @ihnorton, part 2.
Browse files Browse the repository at this point in the history
  • Loading branch information
@KiterLuc
KiterLuc committed Jul 1, 2022
1 parent 1f17f32 commit ac475d1
Show file tree
Hide file tree
Showing 6 changed files with 147 additions and 74 deletions.
14 changes: 14 additions & 0 deletions tiledb/sm/enums/query_condition_combination_op.h
View file
Expand Up @@ -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

Expand Down
25 changes: 25 additions & 0 deletions tiledb/sm/enums/query_condition_op.h
View file
Expand Up @@ -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

Expand Down
71 changes: 10 additions & 61 deletions tiledb/sm/query/ast/query_ast.cc
View file
Expand Up @@ -41,45 +41,12 @@ bool ASTNodeVal::is_expr() const {
return false;
}

tdb_unique_ptr<ASTNode> 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<ASTNode> ASTNodeVal::clone() const {
return tdb_unique_ptr<ASTNode>(tdb_new(ASTNodeVal, *this));
}

return tdb_unique_ptr<ASTNode>(tdb_new(
ASTNodeVal,
field_name_,
condition_value_data_.data(),
condition_value_data_.size(),
op));
tdb_unique_ptr<ASTNode> ASTNodeVal::get_negated_tree() const {
return tdb_unique_ptr<ASTNode>(tdb_new(ASTNodeVal, *this, ASTNegation));
}

void ASTNodeVal::get_field_names(
Expand Down Expand Up @@ -215,30 +182,12 @@ bool ASTNodeExpr::is_expr() const {
return true;
}

tdb_unique_ptr<ASTNode> ASTNodeExpr::clone(bool negate) const {
std::vector<tdb_unique_ptr<ASTNode>> 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<ASTNode> ASTNodeExpr::clone() const {
return tdb_unique_ptr<ASTNode>(tdb_new(ASTNodeExpr, *this));
}

return tdb_unique_ptr<ASTNode>(
tdb_new(ASTNodeExpr, std::move(nodes_copy), combination_op));
tdb_unique_ptr<ASTNode> ASTNodeExpr::get_negated_tree() const {
return tdb_unique_ptr<ASTNode>(tdb_new(ASTNodeExpr, *this, ASTNegation));
}

void ASTNodeExpr::get_field_names(
Expand Down
88 changes: 79 additions & 9 deletions tiledb/sm/query/ast/query_ast.h
View file
Expand Up @@ -53,6 +53,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
Expand All @@ -70,12 +73,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<ASTNode> A deep copy of the ASTNode.
*/
virtual tdb_unique_ptr<ASTNode> clone(bool negate = false) const = 0;
virtual tdb_unique_ptr<ASTNode> 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<ASTNode> A negated deep copy of the ASTNode.
*/
virtual tdb_unique_ptr<ASTNode> get_negated_tree() const = 0;

/**
* @brief Gets the set of field names from all the value nodes in the ASTNode.
Expand Down Expand Up @@ -206,13 +216,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);
Expand All @@ -227,12 +263,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<ASTNode> A deep copy of the ASTNode.
*/
tdb_unique_ptr<ASTNode> clone(bool negate = false) const override;
tdb_unique_ptr<ASTNode> 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<ASTNode> A negated deep copy of the ASTNode.
*/
tdb_unique_ptr<ASTNode> get_negated_tree() const override;

/**
* @brief Gets the set of field names from all the value nodes in the ASTNode.
Expand Down Expand Up @@ -356,13 +399,32 @@ 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_) {
nodes_.push_back(node->clone());
}
}

/**
* @brief Copy constructor, negated.
*/
ASTNodeExpr(const ASTNodeExpr& rhs, ASTNegationT)
: combination_op_(negate_qc_combination_op(rhs.combination_op_)) {
for (auto& node : rhs.nodes_) {
nodes_.push_back(node->get_negated_tree());
}
}

/**
* @brief Default destructor of ASTNodeExpr.
*/
~ASTNodeExpr() {
}

DISABLE_COPY(ASTNodeExpr);
DISABLE_MOVE(ASTNodeExpr);
DISABLE_COPY_ASSIGN(ASTNodeExpr);
DISABLE_MOVE_ASSIGN(ASTNodeExpr);
Expand All @@ -382,7 +444,15 @@ class ASTNodeExpr : public ASTNode {
*
* @return tdb_unique_ptr<ASTNode> A deep copy of the ASTNode.
*/
tdb_unique_ptr<ASTNode> clone(bool negate = false) const override;
tdb_unique_ptr<ASTNode> 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<ASTNode> A negated deep copy of the ASTNode.
*/
tdb_unique_ptr<ASTNode> get_negated_tree() const override;

/**
* @brief Gets the set of field names from all the value nodes in the ASTNode.
Expand Down
21 changes: 18 additions & 3 deletions tiledb/sm/query/ast/test/unit-query-ast.cc
View file
Expand Up @@ -59,7 +59,12 @@ tdb_unique_ptr<ASTNode> test_value_node(
}

// Test ASTNode::clone on the constructed node.
auto node_val_clone = node_val->clone(negate);
tdb_unique_ptr<ASTNode> 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;
Expand All @@ -82,7 +87,12 @@ tdb_unique_ptr<ASTNode> test_string_value_node(
}

// Test ASTNode::clone on the constructed node.
auto node_val_clone = node_val->clone(negate);
tdb_unique_ptr<ASTNode> 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;
Expand All @@ -102,7 +112,12 @@ tdb_unique_ptr<ASTNode> test_expression_node(
}

// Test ASTNode::clone on the constructed node.
auto combined_node_clone = combined_node->clone(negate);
tdb_unique_ptr<ASTNode> 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;
Expand Down
2 changes: 1 addition & 1 deletion tiledb/sm/query/query_condition.cc
View file
Expand Up @@ -1893,7 +1893,7 @@ Status QueryCondition::apply_sparse(
}

QueryCondition QueryCondition::negated_condition() {
return QueryCondition(tree_->clone(true));
return QueryCondition(tree_->get_negated_tree());
}

const tdb_unique_ptr<ASTNode>& QueryCondition::ast() const {
Expand Down

0 comments on commit ac475d1

Please sign in to comment.