From e1894134672e7adb3bbf3e404357dfb9eb9fc2d2 Mon Sep 17 00:00:00 2001 From: Darek Date: Sat, 17 Oct 2026 11:53:35 +0200 Subject: [PATCH] fix(binder): give a new aggregate its own name when an earlier clause has it UNWIND [1, 1, 2] AS x WITH x, count(x) AS c RETURN c, count(x) returned 1|1 twice instead of 2|1 and 1|1; sum(x) and count(DISTINCT x) did the same, and so did count(x) + x after WITH x, count(x) + x AS c. An aggregate is named after its function and arguments, so the RETURN's count(x) had the name of the expression carried in c, or of a part of it, and took that column. An aggregate in a projection list whose name an expression in scope already has, at any depth, now gets a fresh name, reused for each occurrence in the list. In WHERE and ORDER BY an aggregate keeps its structural name, so WITH x, count(x) AS c WHERE count(x) > 1 still filters on c, and so does an aggregate inside a lambda body, which the projection does not compute. Co-Authored-By: Claude Opus 5.5 --- src/binder/bind/bind_projection_clause.cpp | 6 ++++ .../bind_function_expression.cpp | 23 +++++++++++++ .../bind_lambda_expression.cpp | 7 ++++ src/include/binder/expression_binder.h | 5 +++ test/test_files/agg/multi_query_part.test | 32 +++++++++++++++++++ 5 files changed, 73 insertions(+) diff --git a/src/binder/bind/bind_projection_clause.cpp b/src/binder/bind/bind_projection_clause.cpp index fedb198d4b..eb8c96daf1 100644 --- a/src/binder/bind/bind_projection_clause.cpp +++ b/src/binder/bind/bind_projection_clause.cpp @@ -1,5 +1,6 @@ #include #include +#include #include "binder/binder.h" #include "binder/expression/expression_util.h" @@ -124,6 +125,9 @@ static expression_vector getAggregateExpressions(const std::shared_ptr aliases; std::vector parsedItems; @@ -162,6 +166,8 @@ BoundProjectionList Binder::bindProjectionList(const ProjectionBody& projectionB parsedItems.push_back(parsedExpr.get()); } } + config.bindingProjectionList = outerBindingProjectionList; + config.projectionAggregateNames = std::move(outerAggregateNames); return {std::move(projectionExprs), std::move(aliases), std::move(parsedItems)}; } diff --git a/src/binder/bind_expression/bind_function_expression.cpp b/src/binder/bind_expression/bind_function_expression.cpp index fe9636fb93..ede96f7705 100644 --- a/src/binder/bind_expression/bind_function_expression.cpp +++ b/src/binder/bind_expression/bind_function_expression.cpp @@ -1,7 +1,10 @@ +#include + #include "binder/binder.h" #include "binder/expression/aggregate_function_expression.h" #include "binder/expression/scalar_function_expression.h" #include "binder/expression_binder.h" +#include "binder/expression_visitor.h" #include "catalog/catalog.h" #include "common/exception/binder.h" #include "function/arithmetic/vector_arithmetic_functions.h" @@ -23,6 +26,14 @@ using namespace lbug::catalog; namespace lbug { namespace binder { +static bool containsExpressionNamed(const Expression& expression, const std::string& uniqueName) { + if (expression.getUniqueName() == uniqueName) { + return true; + } + return std::ranges::any_of(ExpressionChildrenCollector::collectChildren(expression), + [&](const auto& child) { return containsExpressionNamed(*child, uniqueName); }); +} + std::shared_ptr ExpressionBinder::bindFunctionExpression(const ParsedExpression& expr) { auto funcExpr = expr.constPtrCast(); auto functionName = funcExpr->getNormalizedFunctionName(); @@ -180,6 +191,18 @@ std::shared_ptr ExpressionBinder::bindAggregateFunctionExpression( AggregateFunctionExpression::getUniqueName(function.name, children, function.isDistinct); if (children.empty()) { uniqueExpressionName = binder->getUniqueExpressionName(uniqueExpressionName); + } else if (config.bindingProjectionList && + std::ranges::any_of(binder->scope.getExpressions(), [&](const auto& expr) { + return containsExpressionNamed(*expr, uniqueExpressionName); + })) { + // The name is structural, so this aggregate would share it, and its column, with the + // earlier clause's result, e.g. the group key c in WITH x, count(x) AS c RETURN c, + // count(x), or count(x) inside c in WITH x, count(x) + x AS c RETURN c, count(x) + x. + auto [entry, isNew] = config.projectionAggregateNames.try_emplace(uniqueExpressionName); + if (isNew) { + entry->second = binder->getUniqueExpressionName(uniqueExpressionName); + } + uniqueExpressionName = entry->second; } std::unique_ptr bindData; if (function.bindFunc) { diff --git a/src/binder/bind_expression/bind_lambda_expression.cpp b/src/binder/bind_expression/bind_lambda_expression.cpp index 79ff810608..9da197e844 100644 --- a/src/binder/bind_expression/bind_lambda_expression.cpp +++ b/src/binder/bind_expression/bind_lambda_expression.cpp @@ -1,3 +1,5 @@ +#include + #include "binder/binder.h" #include "binder/expression/expression_util.h" #include "binder/expression/lambda_expression.h" @@ -20,8 +22,13 @@ void ExpressionBinder::bindLambdaExpression(const Expression& lambdaInput, for (auto& varName : parsedLambdaExpr.getVarNames()) { binder->createVariable(varName, listChildType); } + // The projection does not compute aggregates inside a lambda body, so they keep their + // structural names and read the earlier clause's column, as before. + auto& config = binder->getExpressionBinder()->config; + auto bindingProjectionList = std::exchange(config.bindingProjectionList, false); auto funcExpr = binder->getExpressionBinder()->bindExpression(*parsedLambdaExpr.getFunctionExpr()); + config.bindingProjectionList = bindingProjectionList; binder->restoreScope(std::move(prevScope)); boundLambdaExpr.cast(funcExpr->getDataType().copy()); boundLambdaExpr.setFunctionExpr(std::move(funcExpr)); diff --git a/src/include/binder/expression_binder.h b/src/include/binder/expression_binder.h index d5520e4ac3..25aa42b518 100644 --- a/src/include/binder/expression_binder.h +++ b/src/include/binder/expression_binder.h @@ -31,6 +31,11 @@ struct ExpressionBinderConfig { // may lead to empty result. // e.g. [* (r, n | WHERE label(n)='dummy') ] bool disableLabelFunctionLiteralRewrite = false; + // Set while a projection list is bound: an aggregate there is computed by that projection, so + // one whose structural name an earlier clause's expression has gets a fresh name, the same one + // for each occurrence in the list. In WHERE or ORDER BY it keeps the structural name. + bool bindingProjectionList = false; + std::unordered_map projectionAggregateNames; }; class ExpressionBinder { diff --git a/test/test_files/agg/multi_query_part.test b/test/test_files/agg/multi_query_part.test index e89336b5fd..56614fdece 100644 --- a/test/test_files/agg/multi_query_part.test +++ b/test/test_files/agg/multi_query_part.test @@ -96,3 +96,35 @@ Bob|Dan|3 Dan|Alice|3 Dan|Bob|3 Dan|Carol|3 + +-CASE RepeatedAggregateAfterWithGroupKey +-STATEMENT UNWIND [1, 1, 2] AS x WITH x, count(x) AS c RETURN c, count(x) AS n +---- 2 +2|1 +1|1 +-STATEMENT UNWIND [1, 1, 2] AS x WITH x, sum(x) AS s RETURN s, sum(x) AS t +---- 1 +2|3 +-STATEMENT UNWIND [1, 1, 2] AS x WITH x, count(DISTINCT x) AS c RETURN c, count(DISTINCT x) AS n +---- 1 +1|2 +-STATEMENT UNWIND [1, 1, 2] AS x WITH x, count(x) AS c RETURN c, count(x) AS n, count(x) + 1 AS m +---- 2 +2|1|2 +1|1|2 +-STATEMENT UNWIND [1, 1, 2] AS x WITH x, count(x) + x AS c RETURN x, c, count(x) + x AS d +---- 2 +1|3|2 +2|3|3 +-STATEMENT UNWIND [1, 1, 2] AS x WITH x, count(x) AS c WHERE count(x) > 1 RETURN x, c +---- 1 +1|2 +-STATEMENT UNWIND [1, 1, 2] AS x WITH x, count(x) AS c RETURN c ORDER BY count(x) +-CHECK_ORDER +---- 2 +1 +2 +-STATEMENT UNWIND [1, 1, 2] AS x WITH x, count(x) AS c RETURN c, all(i IN [1] WHERE i < count(x)) AS a +---- 2 +1|False +2|True