From f339978a9d6ce32feea36d570ae9e67a99a19f67 Mon Sep 17 00:00:00 2001 From: Anton Alkin Date: Wed, 16 Apr 2025 11:04:23 +0200 Subject: [PATCH 1/2] DPL Analysis: modernize expression parsing code --- .../Core/include/Framework/Expressions.h | 32 +++--- Framework/Core/src/Expressions.cxx | 100 ++++++++---------- 2 files changed, 66 insertions(+), 66 deletions(-) diff --git a/Framework/Core/include/Framework/Expressions.h b/Framework/Core/include/Framework/Expressions.h index af89e56f85835..1fa9afe48e136 100644 --- a/Framework/Core/include/Framework/Expressions.h +++ b/Framework/Core/include/Framework/Expressions.h @@ -69,6 +69,7 @@ struct ExpressionInfo { namespace o2::framework::expressions { +void unknownParameterUsed(const char* name); const char* stringType(atype::type t); template @@ -147,7 +148,7 @@ struct PlaceholderNode : LiteralNode { if constexpr (variant_trait_v::type> != VariantType::Unknown) { retrieve = [](InitContext& context, char const* name) { return LiteralNode::var_t{context.options().get(name)}; }; } else { - runtime_error("Unknown parameter used in expression."); + unknownParameterUsed(name.c_str()); } } @@ -188,6 +189,20 @@ struct ParameterNode : LiteralNode { struct ConditionalNode { }; + +/// concepts +template +concept is_literal_like = std::same_as || std::same_as || std::same_as; + +template +concept is_binding = std::same_as; + +template +concept is_operation = std::same_as; + +template +concept is_conditional = std::same_as; + /// A generic tree node struct Node { Node(LiteralNode&& v) : self{std::forward(v)}, left{nullptr}, right{nullptr}, condition{nullptr} @@ -267,7 +282,7 @@ struct NodeRecord { /// Tree-walker helper template -void walk(Node* head, L const& pred) +void walk(Node* head, L&& pred) { std::stack path; path.emplace(head, 0); @@ -512,16 +527,15 @@ inline Node binned(std::vector const& binning, std::vector const& paramete } template -Node updateParameters(Node const& pexp, int bins, std::vector const& parameters, int bin) +inline Node updateParameters(Node const& pexp, int bins, std::vector const& parameters, int bin) { Node result{pexp}; - auto updateParameter = [&bins, ¶meters, &bin](Node* node) { + walk(&result, [&bins, ¶meters, &bin](Node* node) { if (node->self.index() == 5) { auto* n = std::get_if<5>(&node->self); n->reset(parameters[n->index * bins + bin]); } - }; - walk(&result, updateParameter); + }); return result; } @@ -594,12 +608,6 @@ gandiva::ExpressionPtr makeExpression(gandiva::NodePtr node, gandiva::FieldPtr r /// Update placeholder nodes from context void updatePlaceholders(Filter& filter, InitContext& context); -template -std::vector makeProjectors(framework::pack) -{ - return {C::Projector()...}; -} - std::shared_ptr createProjectorHelper(size_t nColumns, expressions::Projector* projectors, std::shared_ptr schema, std::vector> const& fields); diff --git a/Framework/Core/src/Expressions.cxx b/Framework/Core/src/Expressions.cxx index 6f646515b7837..184365b517654 100644 --- a/Framework/Core/src/Expressions.cxx +++ b/Framework/Core/src/Expressions.cxx @@ -24,6 +24,10 @@ using namespace o2::framework; namespace o2::framework::expressions { +void unknownParameterUsed(const char* name) +{ + runtime_error_f("Unknown parameter used in expression: %s", name); +} /// a map between BasicOp and gandiva node definitions /// note that logical 'and' and 'or' are created separately @@ -89,43 +93,41 @@ size_t Filter::designateSubtrees(Node* node, size_t index) return index; } -namespace +template +constexpr inline auto makeDatum(T const&) { -struct LiteralNodeHelper { - DatumSpec operator()(LiteralNode const& node) const - { - return DatumSpec{node.value, node.type}; - } -}; + return DatumSpec{}; +} -struct BindingNodeHelper { - DatumSpec operator()(BindingNode const& node) const - { - return DatumSpec{node.name, node.hash, node.type}; - } -}; +template +constexpr inline auto makeDatum(T const& node) +{ + return DatumSpec{node.value, node.type}; +} -struct OpNodeHelper { - ColumnOperationSpec operator()(OpNode const& node) const - { - return ColumnOperationSpec{node.op}; - } -}; +template +constexpr inline auto makeDatum(T const& node) +{ + return DatumSpec{node.name, node.hash, node.type}; +} -struct PlaceholderNodeHelper { - DatumSpec operator()(PlaceholderNode const& node) const - { - return DatumSpec{node.value, node.type}; - } -}; +template +constexpr inline auto makeOp(T const&, size_t const&) +{ + return ColumnOperationSpec{}; +} -struct ParameterNodeHelper { - DatumSpec operator()(ParameterNode const& node) const - { - return DatumSpec{node.value, node.type}; - } -}; -} // namespace +template +constexpr inline auto makeOp(T const& node, size_t const& index) +{ + return ColumnOperationSpec{node.op, index}; +} + +template +constexpr inline auto makeOp(T const&, size_t const& index) +{ + return ColumnOperationSpec{BasicOp::Conditional, index}; +} std::shared_ptr concreteArrowType(atype::type type) { @@ -169,7 +171,7 @@ std::string upcastTo(atype::type f) case atype::DOUBLE: return "castFLOAT8"; default: - throw runtime_error_f("Do not know how to cast to %d", f); + throw runtime_error_f("Do not know how to cast to %s", stringType(f)); } } @@ -196,13 +198,11 @@ std::ostream& operator<<(std::ostream& os, DatumSpec const& spec) void updatePlaceholders(Filter& filter, InitContext& context) { - auto updateNode = [&](Node* node) { + expressions::walk(filter.node.get(), [&](Node* node) { if (node->self.index() == 3) { std::get_if<3>(&node->self)->reset(context); } - }; - - expressions::walk(filter.node.get(), updateNode); + }); } const char* stringType(atype::type t) @@ -246,12 +246,7 @@ Operations createOperations(Filter const& expression) auto processLeaf = [](Node const* const node) { return std::visit( - overloaded{ - [lh = LiteralNodeHelper{}](LiteralNode const& node) { return lh(node); }, - [bh = BindingNodeHelper{}](BindingNode const& node) { return bh(node); }, - [ph = PlaceholderNodeHelper{}](PlaceholderNode const& node) { return ph(node); }, - [pr = ParameterNodeHelper{}](ParameterNode const& node) { return pr(node); }, - [](auto&&) { return DatumSpec{}; }}, + [](auto const& n){return makeDatum(n);}, node->self); }; @@ -266,10 +261,7 @@ Operations createOperations(Filter const& expression) // create operation spec, pop the node and add its children auto operationSpec = std::visit( - overloaded{ - [&](OpNode node) { return ColumnOperationSpec{node.op, top.node_ptr->index}; }, - [&](ConditionalNode) { return ColumnOperationSpec{BasicOp::Conditional, top.node_ptr->index}; }, - [](auto&&) { return ColumnOperationSpec{}; }}, + [&](auto const& n){ return makeOp(n, top.node_ptr->index); }, top.node_ptr->self); operationSpec.result = DatumSpec{top.index, operationSpec.type}; @@ -623,15 +615,15 @@ gandiva::NodePtr createExpressionTree(Operations const& opSpecs, auto rightNode = datumNode(it->right); auto condNode = datumNode(it->condition); - auto insertUpcastNode = [&](gandiva::NodePtr node, atype::type t) { - if (t != it->type) { - auto upcast = gandiva::TreeExprBuilder::MakeFunction(upcastTo(it->type), {node}, concreteArrowType(it->type)); + auto insertUpcastNode = [](gandiva::NodePtr node, atype::type t0, atype::type t) { + if (t != t0) { + auto upcast = gandiva::TreeExprBuilder::MakeFunction(upcastTo(t0), {node}, concreteArrowType(t0)); node = upcast; } return node; }; - auto insertEqualizeUpcastNode = [&](gandiva::NodePtr& node1, gandiva::NodePtr& node2, atype::type t1, atype::type t2) { + auto insertEqualizeUpcastNode = [](gandiva::NodePtr& node1, gandiva::NodePtr& node2, atype::type t1, atype::type t2) { if (t2 > t1) { auto upcast = gandiva::TreeExprBuilder::MakeFunction(upcastTo(t2), {node1}, concreteArrowType(t2)); node1 = upcast; @@ -656,14 +648,14 @@ gandiva::NodePtr createExpressionTree(Operations const& opSpecs, default: if (it->op < BasicOp::Sqrt) { if (it->type != atype::BOOL) { - leftNode = insertUpcastNode(leftNode, it->left.type); - rightNode = insertUpcastNode(rightNode, it->right.type); + leftNode = insertUpcastNode(leftNode, it->type, it->left.type); + rightNode = insertUpcastNode(rightNode, it->type, it->right.type); } else if (it->op == BasicOp::Equal || it->op == BasicOp::NotEqual) { insertEqualizeUpcastNode(leftNode, rightNode, it->left.type, it->right.type); } temp_node = gandiva::TreeExprBuilder::MakeFunction(basicOperationsMap[it->op], {leftNode, rightNode}, concreteArrowType(it->type)); } else { - leftNode = insertUpcastNode(leftNode, it->left.type); + leftNode = insertUpcastNode(leftNode, it->type, it->left.type); temp_node = gandiva::TreeExprBuilder::MakeFunction(basicOperationsMap[it->op], {leftNode}, concreteArrowType(it->type)); } break; From 607d745991cb6da6648c68abb7d197ad425d0a6c Mon Sep 17 00:00:00 2001 From: ALICE Action Bot Date: Wed, 16 Apr 2025 09:05:39 +0000 Subject: [PATCH 2/2] Please consider the following formatting changes --- Framework/Core/include/Framework/Expressions.h | 1 - Framework/Core/src/Expressions.cxx | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/Framework/Core/include/Framework/Expressions.h b/Framework/Core/include/Framework/Expressions.h index 1fa9afe48e136..9e00388ee5df8 100644 --- a/Framework/Core/include/Framework/Expressions.h +++ b/Framework/Core/include/Framework/Expressions.h @@ -189,7 +189,6 @@ struct ParameterNode : LiteralNode { struct ConditionalNode { }; - /// concepts template concept is_literal_like = std::same_as || std::same_as || std::same_as; diff --git a/Framework/Core/src/Expressions.cxx b/Framework/Core/src/Expressions.cxx index 184365b517654..94649f8639a0a 100644 --- a/Framework/Core/src/Expressions.cxx +++ b/Framework/Core/src/Expressions.cxx @@ -246,7 +246,7 @@ Operations createOperations(Filter const& expression) auto processLeaf = [](Node const* const node) { return std::visit( - [](auto const& n){return makeDatum(n);}, + [](auto const& n) { return makeDatum(n); }, node->self); }; @@ -261,7 +261,7 @@ Operations createOperations(Filter const& expression) // create operation spec, pop the node and add its children auto operationSpec = std::visit( - [&](auto const& n){ return makeOp(n, top.node_ptr->index); }, + [&](auto const& n) { return makeOp(n, top.node_ptr->index); }, top.node_ptr->self); operationSpec.result = DatumSpec{top.index, operationSpec.type};