diff --git a/velox/connectors/hive/HiveConnectorUtil.cpp b/velox/connectors/hive/HiveConnectorUtil.cpp index f0dc05c96e8..ba7e9c25940 100644 --- a/velox/connectors/hive/HiveConnectorUtil.cpp +++ b/velox/connectors/hive/HiveConnectorUtil.cpp @@ -890,10 +890,10 @@ core::TypedExprPtr extractFiltersFromRemainingFilter( } common::Filter* oldFilter = nullptr; try { - common::Subfield subfield; - if (auto filter = exec::ExprToSubfieldFilterParser::getInstance() - ->leafCallToSubfieldFilter( - *call, subfield, evaluator, negated)) { + if (auto subfieldAndFilter = + exec::ExprToSubfieldFilterParser::getInstance() + ->leafCallToSubfieldFilter(*call, evaluator, negated)) { + auto& [subfield, filter] = subfieldAndFilter.value(); if (auto it = filters.find(subfield); it != filters.end()) { oldFilter = it->second.get(); filter = filter->mergeWith(oldFilter); diff --git a/velox/dwio/common/MetadataFilter.cpp b/velox/dwio/common/MetadataFilter.cpp index 59181c557fe..88e3a5481e2 100644 --- a/velox/dwio/common/MetadataFilter.cpp +++ b/velox/dwio/common/MetadataFilter.cpp @@ -208,13 +208,14 @@ std::unique_ptr MetadataFilter::Node::fromExpression( return fromExpression(*call->inputs()[0], evaluator, !negated); } try { - Subfield subfield; - auto filter = + auto subfieldAndFilter = exec::ExprToSubfieldFilterParser::getInstance() - ->leafCallToSubfieldFilter(*call, subfield, evaluator, negated); - if (!filter) { + ->leafCallToSubfieldFilter(*call, evaluator, negated); + if (!subfieldAndFilter.has_value()) { return nullptr; } + + auto& [subfield, filter] = subfieldAndFilter.value(); VELOX_CHECK( subfield.valid(), "Invalid subfield from expression: {}", diff --git a/velox/expression/ExprToSubfieldFilter.cpp b/velox/expression/ExprToSubfieldFilter.cpp index 9d3d51eac93..9b3c75e3f7f 100644 --- a/velox/expression/ExprToSubfieldFilter.cpp +++ b/velox/expression/ExprToSubfieldFilter.cpp @@ -471,67 +471,90 @@ std::unique_ptr ExprToSubfieldFilterParser::makeOrFilter( return orFilter(std::move(a), std::move(b)); } -std::unique_ptr +namespace { +std::optional>> +combine(common::Subfield& subfield, std::unique_ptr& filter) { + if (filter != nullptr) { + return std::make_pair(std::move(subfield), std::move(filter)); + } + + return std::nullopt; +} +} // namespace + +std::optional>> PrestoExprToSubfieldFilterParser::leafCallToSubfieldFilter( const core::CallTypedExpr& call, - common::Subfield& subfield, core::ExpressionEvaluator* evaluator, bool negated) { if (call.inputs().empty()) { - return nullptr; + return std::nullopt; } const auto* leftSide = call.inputs()[0].get(); + common::Subfield subfield; if (call.name() == "eq") { if (toSubfield(leftSide, subfield)) { - return negated ? makeNotEqualFilter(call.inputs()[1], evaluator) - : makeEqualFilter(call.inputs()[1], evaluator); + auto filter = negated ? makeNotEqualFilter(call.inputs()[1], evaluator) + : makeEqualFilter(call.inputs()[1], evaluator); + + return combine(subfield, filter); } } else if (call.name() == "neq") { if (toSubfield(leftSide, subfield)) { - return negated ? makeEqualFilter(call.inputs()[1], evaluator) - : makeNotEqualFilter(call.inputs()[1], evaluator); + auto filter = negated ? makeEqualFilter(call.inputs()[1], evaluator) + : makeNotEqualFilter(call.inputs()[1], evaluator); + return combine(subfield, filter); } } else if (call.name() == "lte") { if (toSubfield(leftSide, subfield)) { - return negated ? makeGreaterThanFilter(call.inputs()[1], evaluator) - : makeLessThanOrEqualFilter(call.inputs()[1], evaluator); + auto filter = negated + ? makeGreaterThanFilter(call.inputs()[1], evaluator) + : makeLessThanOrEqualFilter(call.inputs()[1], evaluator); + return combine(subfield, filter); } } else if (call.name() == "lt") { if (toSubfield(leftSide, subfield)) { - return negated ? makeGreaterThanOrEqualFilter(call.inputs()[1], evaluator) - : makeLessThanFilter(call.inputs()[1], evaluator); + auto filter = negated + ? makeGreaterThanOrEqualFilter(call.inputs()[1], evaluator) + : makeLessThanFilter(call.inputs()[1], evaluator); + return combine(subfield, filter); } } else if (call.name() == "gte") { if (toSubfield(leftSide, subfield)) { - return negated + auto filter = negated ? makeLessThanFilter(call.inputs()[1], evaluator) : makeGreaterThanOrEqualFilter(call.inputs()[1], evaluator); + return combine(subfield, filter); } } else if (call.name() == "gt") { if (toSubfield(leftSide, subfield)) { - return negated ? makeLessThanOrEqualFilter(call.inputs()[1], evaluator) - : makeGreaterThanFilter(call.inputs()[1], evaluator); + auto filter = negated + ? makeLessThanOrEqualFilter(call.inputs()[1], evaluator) + : makeGreaterThanFilter(call.inputs()[1], evaluator); + return combine(subfield, filter); } } else if (call.name() == "between") { if (toSubfield(leftSide, subfield)) { - return makeBetweenFilter( + auto filter = makeBetweenFilter( call.inputs()[1], call.inputs()[2], evaluator, negated); + return combine(subfield, filter); } } else if (call.name() == "in") { if (toSubfield(leftSide, subfield)) { - return makeInFilter(call.inputs()[1], evaluator, negated); + auto filter = makeInFilter(call.inputs()[1], evaluator, negated); + return combine(subfield, filter); } } else if (call.name() == "is_null") { if (toSubfield(leftSide, subfield)) { if (negated) { - return isNotNull(); + return std::make_pair(std::move(subfield), isNotNull()); } - return isNull(); + return std::make_pair(std::move(subfield), isNull()); } } - return nullptr; + return std::nullopt; } std::pair> @@ -550,19 +573,18 @@ PrestoExprToSubfieldFilterParser::toSubfieldFilter( makeOrFilter(std::move(left.second), std::move(right.second))}; } - common::Subfield subfield; - std::unique_ptr filter; if (call->name() == "not") { const auto& input = call->inputs()[0]; if (input->isCallKind(); auto* inner = input->asUnchecked()) { - filter = leafCallToSubfieldFilter(*inner, subfield, evaluator, true); + if (auto result = leafCallToSubfieldFilter(*inner, evaluator, true)) { + return std::move(result.value()); + } } } else { - filter = leafCallToSubfieldFilter(*call, subfield, evaluator, false); - } - if (filter) { - return std::make_pair(std::move(subfield), std::move(filter)); + if (auto result = leafCallToSubfieldFilter(*call, evaluator, false)) { + return std::move(result.value()); + } } } VELOX_UNSUPPORTED( diff --git a/velox/expression/ExprToSubfieldFilter.h b/velox/expression/ExprToSubfieldFilter.h index 9257a29aa5f..ed236749d1f 100644 --- a/velox/expression/ExprToSubfieldFilter.h +++ b/velox/expression/ExprToSubfieldFilter.h @@ -443,22 +443,20 @@ class ExprToSubfieldFilterParser { core::ExpressionEvaluator*) = 0; /// Analyzes 'call' expression to determine if it can be expressed as a - /// subfield filter. Returns the filter and sets 'subfield' output argument if - /// so. Otherwise, returns nullptr. If 'negated' is true, considers the - /// negation of 'call' expressions (not(call)). It is possible that 'call' - /// expression can be represented as subfield filter, but its negation cannot. - /// - /// TODO Make this and toSubfieldFilter APIs consistent. Both should not throw - /// and return std::optional pair of filter and subfield. - virtual std::unique_ptr leafCallToSubfieldFilter( + /// subfield filter. Returns the subfield and filter if so. Otherwise, returns + /// std::nullopt. If 'negated' is true, considers the negation of 'call' + /// expressions (not(call)). It is possible that 'call' expression can be + /// represented as subfield filter, but its negation cannot. + virtual std::optional< + std::pair>> + leafCallToSubfieldFilter( const core::CallTypedExpr& call, - common::Subfield& subfield, core::ExpressionEvaluator* evaluator, bool negated = false) = 0; protected: - // Converts an expression into a subfield. Returns false if the expression is - // not a valid field expression. + // Converts an expression into a subfield. Returns false if the expression + // is not a valid field expression. static bool toSubfield( const core::ITypedExpr* field, common::Subfield& subfield); @@ -523,9 +521,9 @@ class PrestoExprToSubfieldFilterParser : public ExprToSubfieldFilterParser { const core::TypedExprPtr& expr, core::ExpressionEvaluator* evaluator) override; - std::unique_ptr leafCallToSubfieldFilter( + std::optional>> + leafCallToSubfieldFilter( const core::CallTypedExpr& call, - common::Subfield& subfield, core::ExpressionEvaluator* evaluator, bool negated = false) override; }; diff --git a/velox/expression/tests/ExprToSubfieldFilterTest.cpp b/velox/expression/tests/ExprToSubfieldFilterTest.cpp index 91c1ee74fe8..880c14b3dfa 100644 --- a/velox/expression/tests/ExprToSubfieldFilterTest.cpp +++ b/velox/expression/tests/ExprToSubfieldFilterTest.cpp @@ -68,11 +68,13 @@ class ExprToSubfieldFilterTest : public testing::Test { std::pair> leafCallToSubfieldFilter(const core::CallTypedExprPtr& call) { - Subfield subfield; - auto filter = - ExprToSubfieldFilterParser::getInstance()->leafCallToSubfieldFilter( - *call, subfield, evaluator()); - return {std::move(subfield), std::move(filter)}; + if (auto result = + ExprToSubfieldFilterParser::getInstance()->leafCallToSubfieldFilter( + *call, evaluator())) { + return std::move(result.value()); + } + + return std::make_pair(common::Subfield(), nullptr); } std::pair> toSubfieldFilter( @@ -319,49 +321,53 @@ class CustomExprToSubfieldFilterParser : public ExprToSubfieldFilterParser { std::move(left.first), makeOrFilter(std::move(left.second), std::move(right.second))}; } - common::Subfield subfield; - std::unique_ptr filter; if (call->name() == "not") { if (auto* inner = call->inputs()[0]->asUnchecked()) { - filter = leafCallToSubfieldFilter(*inner, subfield, evaluator, true); + if (auto result = leafCallToSubfieldFilter(*inner, evaluator, true)) { + return std::move(result.value()); + } } } else { - filter = leafCallToSubfieldFilter(*call, subfield, evaluator, false); - } - if (filter) { - return std::make_pair(std::move(subfield), std::move(filter)); + if (auto result = leafCallToSubfieldFilter(*call, evaluator, false)) { + return std::move(result.value()); + } } } VELOX_UNSUPPORTED( "Unsupported expression for range filter: {}", expr->toString()); } - std::unique_ptr leafCallToSubfieldFilter( + std::optional>> + leafCallToSubfieldFilter( const core::CallTypedExpr& call, - common::Subfield& subfield, core::ExpressionEvaluator* evaluator, bool negated) override { if (call.inputs().empty()) { - return nullptr; + return std::nullopt; } const auto* leftSide = call.inputs()[0].get(); + common::Subfield subfield; if (call.name() == "custom_eq") { if (toSubfield(leftSide, subfield)) { - return negated ? makeNotEqualFilter(call.inputs()[1], evaluator) - : makeEqualFilter(call.inputs()[1], evaluator); + auto filter = negated ? makeNotEqualFilter(call.inputs()[1], evaluator) + : makeEqualFilter(call.inputs()[1], evaluator); + if (filter != nullptr) { + return std::make_pair(std::move(subfield), std::move(filter)); + } + return std::nullopt; } } else if (call.name() == "is_null") { if (toSubfield(call.inputs()[0].get(), subfield)) { if (negated) { - return isNotNull(); + return std::make_pair(std::move(subfield), isNotNull()); } - return isNull(); + return std::make_pair(std::move(subfield), isNull()); } } - return nullptr; + return std::nullopt; } };