Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DM-16496: add support for XOR #435

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

DM-16496: add support for XOR #435

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions core/modules/ccontrol/testCControl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -950,6 +950,14 @@ static const std::vector<Antlr4CompareQueries> ANTLR4_COMPARE_QUERIES = {
},
"SELECT * FROM Filter WHERE NOT(filterId>1 AND filterId<6)"
),

// tests XOR
Antlr4CompareQueries(
"select ra_PS, objectId from Object where ra_PS > 1 XOR ra_PS < 2;",
"select ra_PS, objectId from Object where (ra_PS > 1 and not ra_PS < 2) OR (not ra_PS > 1 and ra_PS < 2)",
[](query::SelectStmt::Ptr const & selectStatement) {},
"SELECT ra_PS,objectId FROM Object WHERE (ra_PS>1 AND NOT ra_PS<2) OR (NOT ra_PS>1 AND ra_PS<2)"
),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With that nontrivial transforms I'd prefer to see more tests with XOR combinations, e.g.

A XOR NOT B
NOT A XOR NOT B
A XOR B XOR C
A XOR B OR C XOR D
A XOR B AND C XOR D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, will do.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last couple of expressions were about operator precedence but I'm not sure that they cover all cases correctly, try to think about more cases. In mysql XOR is between AND and OR in precedence.

};


Expand Down
109 changes: 65 additions & 44 deletions core/modules/parser/QSMySqlListener.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,11 +536,17 @@ class LogicalOperatorCBH : public BaseCBH {
enum OperatorType {
AND,
OR,
XOR,
};
virtual void handleLogicalOperator(OperatorType operatorType) = 0;

static string OperatorTypeToStr(OperatorType operatorType) {
return operatorType == AND ? "AND" : "OR";
switch (operatorType) {
case AND: return "AND"; break;
case OR: return "OR"; break;
case XOR: return "XOR"; break;
}
return "unrecognized operator type.";
}
};

Expand Down Expand Up @@ -2689,29 +2695,15 @@ class LogicalExpressionAdapter :

void handleLogicalOperator(LogicalOperatorCBH::OperatorType operatorType) {
TRACE_CALLBACK_INFO(LogicalOperatorCBH::OperatorTypeToStr(operatorType));
switch (operatorType) {
default:
ASSERT_EXECUTION_CONDITION(false, "unhandled operator type", _ctx);
break;

case LogicalOperatorCBH::AND:
// We capture the AndTerm into a base class so we can pass by reference into the setter.
_setLogicalOperator(make_shared<query::AndTerm>());
break;

case LogicalOperatorCBH::OR:
// We capture the OrTerm into a base class so we can pass by reference into the setter.
_setLogicalOperator(make_shared<query::OrTerm>());
break;
}
ASSERT_EXECUTION_CONDITION(false == _logicalOperatorIsSet,
"logical operator must be set only once.", _ctx);
_logicalOperatorIsSet = true;
_logicalOperatorType = operatorType;
}

void handleLogicalExpression(shared_ptr<query::LogicalTerm> const & logicalTerm,
antlr4::ParserRuleContext* childCtx) override {
TRACE_CALLBACK_INFO(logicalTerm);
if (_logicalOperator != nullptr && _logicalOperator->merge(*logicalTerm)) {
return;
}
_terms.push_back(logicalTerm);
}

Expand All @@ -2725,47 +2717,74 @@ class LogicalExpressionAdapter :
}

void onExit() override {
ASSERT_EXECUTION_CONDITION(_logicalOperator != nullptr, "logicalOperator is not set.", _ctx);

bool isOr = dynamic_pointer_cast<query::OrTerm>(_logicalOperator) != nullptr;
for (auto term : _terms) {
if (false == _logicalOperator->merge(*term)) {
if (isOr) {
_logicalOperator->addBoolTerm(make_shared<query::AndTerm>(term));
} else {
_logicalOperator->addBoolTerm(term);
ASSERT_EXECUTION_CONDITION(_logicalOperatorIsSet, "logicalOperator is not set.", _ctx);
shared_ptr<query::LogicalTerm> logicalTerm;
switch (_logicalOperatorType) {
case LogicalOperatorCBH::AND: {
logicalTerm = make_shared<query::AndTerm>();
for (auto term : _terms) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

auto const& maybe to avoid copy (and below)

if (false == logicalTerm->merge(*term)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a special ! operator in C++ for this kind of test (also named not) 😉

logicalTerm->addBoolTerm(term);
}
}
break;
}

case LogicalOperatorCBH::OR: {
logicalTerm = make_shared<query::OrTerm>();
for (auto term : _terms) {
if (false == logicalTerm->merge(*term)) {
logicalTerm->addBoolTerm(make_shared<query::AndTerm>(term));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason you have to wrap it into AndTerm here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has something to do with getting it into disjunctive normal form. @fritzm can explain it to us (me, again) in the meeting tomorrow.

}
}
break;
}

case LogicalOperatorCBH::XOR: {
logicalTerm = make_shared<query::OrTerm>();
for (auto addTerm : _terms) {
query::BoolTerm::PtrVector terms;
for (auto otherTerm: _terms) {
if (otherTerm == addTerm) {
terms.push_back(otherTerm);
} else {
auto term = dynamic_pointer_cast<query::BoolFactor>(otherTerm->copySyntax());
ASSERT_EXECUTION_CONDITION(term != nullptr, "XOR currently only works with BoolFactor", _ctx);
term->setHasNot(true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this handle terms that already have NOT, I mean something like:

A XOR NOT B

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hrm. good question! I'll investigate.

terms.push_back(term);
}
}
query::BoolFactorTerm::PtrVector termsWithParenthesis;
termsWithParenthesis.push_back(make_shared<query::PassTerm>("("));
termsWithParenthesis.push_back(make_shared<query::BoolTermFactor>(make_shared<query::AndTerm>(terms)));
termsWithParenthesis.push_back(make_shared<query::PassTerm>(")"));
auto boolFactor = make_shared<query::BoolFactor>(termsWithParenthesis);
auto andTerm = make_shared<query::AndTerm>(boolFactor);
logicalTerm->addBoolTerm(andTerm);
}
break;
}

default:
ASSERT_EXECUTION_CONDITION(false, "unhandled logical operator.", _ctx);
}
lockedParent()->handleLogicalExpression(_logicalOperator, _ctx);
lockedParent()->handleLogicalExpression(logicalTerm, _ctx);
}

string name() const override { return getTypeName(this); }

private:
void _setLogicalOperator(shared_ptr<query::LogicalTerm> const & logicalTerm) {
ASSERT_EXECUTION_CONDITION(nullptr == _logicalOperator,
"logical operator must be set only once.", _ctx);
_logicalOperator = logicalTerm;
}

friend ostream& operator<<(ostream& os, const LogicalExpressionAdapter& logicaAndlExpressionAdapter);

// a qserv restrictor fucntion can be the left side of a predicate (currently it can only be the left
// side; that is to say, it can only be the first term in the WHERE clause. If `handleQservFunctionSpec`
// is called and _leftTerm is null (as well as _rightTerm and _logicalOperator, then _leftHandled is set
// to true to indicate that the left term has been handled. This allows onExit to put only one term into
// the logicalOperator and know that it was ok (the qserv IR accepts an AndTerm with only one factor).
// This mechanism does not fully proect against qserv restrictors that may be the left side of a
// subsequent logical expression. TBD if that's really an issue.
vector<shared_ptr<query::BoolTerm>> _terms;
shared_ptr<query::LogicalTerm> _logicalOperator;
LogicalOperatorCBH::OperatorType _logicalOperatorType;
bool _logicalOperatorIsSet {false};
};


ostream& operator<<(ostream& os, const LogicalExpressionAdapter& logicalExpressionAdapter) {
os << "LogicalExpressionAdapter(";
os << "terms:" << util::printable(logicalExpressionAdapter._terms);
os << util::printable(logicalExpressionAdapter._terms);
return os;
}

Expand Down Expand Up @@ -3277,6 +3296,8 @@ class LogicalOperatorAdapter :
lockedParent()->handleLogicalOperator(LogicalOperatorCBH::AND);
} else if (_ctx->OR() != nullptr || _ctx->getText() == "||") {
lockedParent()->handleLogicalOperator(LogicalOperatorCBH::OR);
} else if (_ctx->XOR() != nullptr) { // just a note; there is no alt operator like e.g. && for AND
lockedParent()->handleLogicalOperator(LogicalOperatorCBH::XOR);
} else {
ASSERT_EXECUTION_CONDITION(false, "unhandled logical operator", _ctx);
}
Expand Down
20 changes: 14 additions & 6 deletions core/modules/query/BoolTerm.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ bool OrTerm::merge(const BoolTerm& other) {
return true;
}
void OrTerm::dbgPrint(std::ostream& os) const {
os << "OrTerm(terms:" << util::printable(_terms) << ")";
os << "OrTerm(" << util::printable(_terms) << ")";
}
bool OrTerm::operator==(const BoolTerm& rhs) const {
auto rhsOrTerm = dynamic_cast<OrTerm const *>(&rhs);
Expand All @@ -332,7 +332,7 @@ bool AndTerm::merge(const BoolTerm& other) {
return true;
}
void AndTerm::dbgPrint(std::ostream& os) const {
os << "AndTerm(terms:" << util::printable(_terms) << ")";
os << "AndTerm(" << util::printable(_terms) << ")";
}
bool AndTerm::operator==(const BoolTerm& rhs) const {
auto rhsAndTerm = dynamic_cast<AndTerm const *>(&rhs);
Expand All @@ -348,7 +348,11 @@ std::shared_ptr<BoolTerm> BoolFactor::copySyntax() const {
return bf;
}
void BoolFactor::dbgPrint(std::ostream& os) const {
os << "BoolFactor(terms:" << util::printable(_terms) << ", hasNot:" << _hasNot << ")";
os << "BoolFactor(" << util::printable(_terms);
if (_hasNot) {
os << ", has NOT";
}
os << ")";
}
void UnknownTerm::dbgPrint(std::ostream& os) const {
os << "UnknownTerm()";
Expand All @@ -362,7 +366,11 @@ BoolFactorTerm::Ptr PassTerm::copySyntax() const {
return BoolFactorTerm::Ptr(p);
}
void PassTerm::dbgPrint(std::ostream& os) const {
os << "PassTerm(text:'" << _text << "')";
os << "PassTerm('";
if ("(" == _text) os << "LHP";
else if (")" == _text) os << "RHP";
else os << _text;
os << "')";
}
bool PassTerm::operator==(const BoolFactorTerm& rhs) const {
auto rhsPassTerm = dynamic_cast<PassTerm const *>(&rhs);
Expand All @@ -377,7 +385,7 @@ BoolFactorTerm::Ptr PassListTerm::copySyntax() const {
return BoolFactorTerm::Ptr(p);
}
void PassListTerm::dbgPrint(std::ostream& os) const {
os << "PassListTerm(terms:" << util::printable(_terms) << ")";
os << "PassListTerm(" << util::printable(_terms) << ")";
}
bool PassListTerm::operator==(const BoolFactorTerm& rhs) const {
auto rhsTerm = dynamic_cast<PassListTerm const *>(&rhs);
Expand All @@ -392,7 +400,7 @@ BoolFactorTerm::Ptr BoolTermFactor::copySyntax() const {
return BoolFactorTerm::Ptr(p);
}
void BoolTermFactor::dbgPrint(std::ostream& os) const {
os << "BoolTermFactor(term:" << _term << ")";
os << "BoolTermFactor(" << _term << ")";
}
bool BoolTermFactor::operator==(const BoolFactorTerm& rhs) const {
auto rhsTerm = dynamic_cast<BoolTermFactor const *>(&rhs);
Expand Down
2 changes: 1 addition & 1 deletion core/modules/query/FuncExpr.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ std::ostream&
operator<<(std::ostream& os, FuncExpr const& fe) {
os << "FuncExpr(";
os << "name:" << fe._name;
os << ", params:" << util::printable(fe.params);
os << ", " << util::printable(fe.params);
os << ")";
return os;
}
Expand Down
2 changes: 1 addition & 1 deletion core/modules/query/GroupByClause.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ bool GroupByTerm::operator==(const GroupByTerm& rhs) const {
// GroupByClause
////////////////////////////////////////////////////////////////////////
std::ostream& operator<<(std::ostream& os, GroupByClause const& c) {
os <<"GroupByClause(terms:" << util::ptrPrintable(c._terms) << ")";
os << "GroupByClause(" << util::ptrPrintable(c._terms) << ")";
return os;
}

Expand Down
2 changes: 1 addition & 1 deletion core/modules/query/OrderByClause.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ bool OrderByTerm::operator==(const OrderByTerm& rhs) const {
////////////////////////////////////////////////////////////////////////
std::ostream&
operator<<(std::ostream& os, OrderByClause const& clause) {
os << "OrderByClause(terms:" << util::ptrPrintable(clause._terms) << ")";
os << "OrderByClause(" << util::ptrPrintable(clause._terms) << ")";
return os;
}

Expand Down
16 changes: 12 additions & 4 deletions core/modules/query/Predicate.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,9 @@ BoolFactorTerm::Ptr InPredicate::clone() const {
void InPredicate::dbgPrint(std::ostream& os) const {
os << "InPredicate(value:" << value;
os << ", cands:" << util::printable(cands);
os << ", hasNot:" << hasNot;
if (hasNot) {
os << ", has NOT";
}
os << ")";
}

Expand All @@ -271,7 +273,9 @@ BoolFactorTerm::Ptr BetweenPredicate::clone() const {

void BetweenPredicate::dbgPrint(std::ostream& os) const {
os << "BetweenPredicate(value:" << value;
os << ", NOT:" << (hasNot ? "true" : "false");
if (hasNot) {
os << " NOT,";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will make two commas

}
os << ", minValue:" << minValue;
os << ", maxValue:" << maxValue;
os << ")";
Expand All @@ -298,7 +302,9 @@ BoolFactorTerm::Ptr LikePredicate::clone() const {

void LikePredicate::dbgPrint(std::ostream& os) const {
os << "LikePredicate(value:" << value;
os << ", NOT:" << hasNot;
if (hasNot) {
os << ", NOT";
}
os << ", charValue:" << charValue;
os << ")";
}
Expand All @@ -322,7 +328,9 @@ BoolFactorTerm::Ptr NullPredicate::clone() const {

void NullPredicate::dbgPrint(std::ostream& os) const {
os << "NullPredicate(value:" << value;
os << ", hasNot:" << hasNot;
if (hasNot) {
os << ", NOT";
}
os << ")";
}

Expand Down
13 changes: 3 additions & 10 deletions core/modules/query/ValueExpr.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ renderList(QueryTemplate& qt, ValueExprPtrVector const& vel) {
////////////////////////////////////////////////////////////////////////

std::ostream& operator<<(std::ostream& os, const ValueExpr::FactorOp& factorOp) {
os << "FactorOp(op:";
os << "FactorOp(";
switch(factorOp.op) {
case ValueExpr::NONE: os << "NONE"; break;
case ValueExpr::UNKNOWN: os << "UNKNOWN"; break;
Expand All @@ -92,7 +92,7 @@ std::ostream& operator<<(std::ostream& os, const ValueExpr::FactorOp& factorOp)
case ValueExpr::BIT_XOR: os << "BIT_XOR"; break;
default: os << "!!unhandled!!"; break;
}
os << ", factor:" << factorOp.factor;
os << ", " << factorOp.factor;
os << ")";
return os;
}
Expand Down Expand Up @@ -270,14 +270,7 @@ std::string ValueExpr::sqlFragment() const {

std::ostream& operator<<(std::ostream& os, ValueExpr const& ve) {
os << "ValueExpr(";
os << "alias:" << ve._alias;
os << ", isColumnRef:" << ve.isColumnRef();
os << ", isFactor:" << ve.isFactor();
os << ", isStar:" << ve.isStar();
bool hasAgg = false;
qana::CheckAggregation ca(hasAgg);
os << ", hasAgg:" << hasAgg;
os << ", factorOps:";
if (not ve._alias.empty()) os << "alias:" << ve._alias << ", ";
os << util::printable(ve._factorOps);
os << ")";
return os;
Expand Down
11 changes: 5 additions & 6 deletions core/modules/query/ValueFactor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,12 +136,11 @@ ValueFactorPtr ValueFactor::clone() const{

std::ostream& operator<<(std::ostream& os, ValueFactor const& ve) {
os << "ValueFactor(";
os << "type:" << ValueFactor::getTypeString(ve._type);
os << ", columnRef:" << ve._columnRef;
os << ", funcExpr:" << ve._funcExpr;
os << ", valueExpr:" << ve._valueExpr;
os << ", alias:" << ve._alias;
os << ", constVal:" << ve._constVal;
if (ve._columnRef != nullptr) os << ve._columnRef;
if (ve._funcExpr != nullptr) os << ve._funcExpr;
if (ve._valueExpr != nullptr) os << ve._valueExpr;
if (not ve._alias.empty()) os << "alias:" << ve._alias;
if (not ve._constVal.empty()) os << "constVal:" << ve._constVal;
os << ")";
return os;
}
Expand Down