DM-55685: Expanded aggregation support by malensek · Pull Request #1094 · lsst/qserv

malensek · 2026-09-03T17:15:41Z

The query analysis AggregatePlugin previously assumed a flat, top-level list of expressions from the parser adapter. As a result, the adapter layer restricted aggregations to simple column references only. This change enables the plugin to handle both a flat list of expressions from the legacy adapter as well as a potentially deeper tree of expressions produced by the Hyrise adapter.

Additionally, this change enables BIT_ aggregations, since they can be split and merged via AccumulateOp like MIN, MAX, SUM.

Here are some of the new possibilities supported by this change (using the qcase02 test dataset):

SELECT ROUND(100.0 * COUNT(yFlags) / COUNT(*), 1) FROM qcase02.Object;
SELECT MAX(ABS(decl_PS)) FROM qcase02.Object;
SELECT MAX(objectId + 1) FROM qcase02.Object;
SELECT MIN(LENGTH(objectId)) FROM qcase02.Object;
SELECT 25 + SUM(uNumObs) + 100 FROM qcase02.Object;
SELECT BIT_OR(uFlags & 255) FROM qcase02.Object;

Summary of Changes

AggregatePlugin recursively traverses ValueExpr and ValueFactor trees
SchemaFactory preserves UNSIGNED_FLAG when creating integer columns
BIT_XOR, BIT_AND, BIT_OR are allowed by the parser adapter and processed in query analysis
HAVING aggregations that can't be resolved are rejected up front
AVG() that contain non-deterministic functions are rejected (distributed AVG has to call the function twice)
New unit and integration tests for this functionality

Related issues (existing behavior, being tracked in DM-56009):

HAVING with non-aggregate expressions are not currently validated against the SELECT list
GROUP BY keys are not validated against the SELECT list
SELECT * does not populate _usedValueExprs, so nothing in ORDER BY / HAVING matches

Copilot

🟡 Changes recommended

Critical aggregate-placement and grouped-merge planning issues remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Expands distributed aggregation to support nested expressions, bitwise aggregates, and unsigned intermediate result types.

Changes:

Recursively detects and rewrites aggregate expression trees.
Adds BIT_AND, BIT_OR, and BIT_XOR support.
Adds unit and integration coverage.

File summaries

File	Description
`src/query/ValueFactor.h`	Declares recursive aggregation detection.
`src/query/ValueFactor.cc`	Implements recursive factor traversal.
`src/query/ValueExpr.cc`	Delegates aggregation detection to factors.
`src/query/AggOp.cc`	Adds bitwise accumulation operations.
`src/qproc/testQueryAnaAggregation.cc`	Tests rewritten plans; critically, a grouped merge expects `chunkId` that the parallel query does not emit.
`src/qana/CheckAggregation.h`	Removes the obsolete shallow checker.
`src/qana/AggregatePlugin.cc`	Recursively rewrites aggregate trees.
`src/mysql/SchemaFactory.cc`	Preserves unsigned integer metadata.
`src/ccontrol/testHyriseGeneratedIR.cc`	Updates adapter aggregation tests.
`src/ccontrol/HyriseAdapter.cc`	Expands aggregate parsing; critically, invalid aggregate placements beyond `WHERE`, including `HAVING`, are not safely rejected or rewritten.
`data/case01/queries/2117_nestedAggregateExpressionGroupBy.sql`	Tests grouped nested aggregation.
`data/case01/queries/2116_nestedAggregateExpression.sql`	Tests nested aggregate arithmetic.
`data/case01/queries/2115_bitwiseAggregatesEmptyInput.sql`	Tests empty-input identities.
`data/case01/queries/2114_bitwiseAggregateWideValue.sql`	Tests unsigned 64-bit results.
`data/case01/queries/2113_bitwiseAggregateExpression.sql`	Tests expression arguments.
`data/case01/queries/2112_bitwiseAggregatesGroupBy.sql`	Tests grouped bitwise aggregation.
`data/case01/queries/2111_bitwiseAggregatesEmptyChunks.sql`	Tests empty worker chunks.
`data/case01/queries/2110_bitwiseAggregates.sql`	Tests basic bitwise aggregates.

Review details

Files reviewed: 18/18 changed files
Comments generated: 2
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

🟢 Approval recommended

The recursive rewrite, placement validation, schema handling, and edge cases are covered consistently by focused unit and integration tests.

Review details

Files reviewed: 21/21 changed files
Comments generated: 0 new
Review effort level: Balanced

Previously this method checked all factors for AGGFUNC types. This adds a new ValueFactor::hasAggregation() method to also check for scalar functions or expressions that contain an aggregation, and is called by the original ValueExpr::hasAggregation().

These methods make it possible to check if any of the expressions under a ValueExpr / ValueFactor contain a function that produces non-deterministic output (e.g., RAND()). This is useful in determining if the Factor can be used in a distributed AVG operation.

Previously signed/unsigned information was not preserved for integer types when creating merge tables, so unsigned results from the worker were converted to signed columns on the czar. In most cases this would not be an issue (no conversion error), but bitwise aggregations make it easier to produce values that would not convert properly (e.g., a number with all bits set).

Much like MIN, MAX, SUM, AccumulateOp can be used for bitwise aggregations because the same action can be applied in both the parallel and merging phases. This adds BIT_AND, BIT_OR, and BIT_XOR to the map of valid aggregations and associates them with AccumulateOp.

The previous parser adapter produced a flat, top-level list of expressions that made merge query rewriting straightforward. However, it also meant aggregations were limited to simple column references. This change enables `qana` to handle both a flat list of expressions from the legacy adapter as well as a tree of expressions. Consequently, we can now support admissible nested operations like SELECT MAX(LENGTH(x)).

* Updates the parser adapter to allow these query types * Adds validation to TablePlugin to ensure aggregations in `HAVING` are present in the `SELECT` list * Adds new unit and integration tests to verify their functionality.

Copilot

Copilot review overview

🟡 Changes recommended

Bitwise aggregates remain unsupported by the legacy parser, and case-sensitive HAVING matching rejects valid SQL.

Review effort: Balanced
Findings: 1 · 1

Open (2)

Guard or implement bitwise aggregates for the legacy parser · New
Make aggregate function matching case-insensitive · New

malensek · 2026-10-02T22:41:16Z

        {"avg", true},
+        {"bit_and", true},
+        {"bit_or", true},
+        {"bit_xor", true},


Hmm, must've missed a guard on the new tests. Thanks!

malensek · 2026-10-02T22:43:54Z

+void validateHavingMatch(query::ValueExprPtr const& valueExpr, query::ValueExprPtr const& valueExprMatch) {
+    if (valueExpr->hasAggregation() && valueExprMatch == nullptr) {
+        throw AnalysisError(
+                "HAVING aggregate expressions must match an entry or alias in the SELECT list: \"" +
+                valueExpr->sqlFragment(query::QueryTemplate::NO_ALIAS) + "\"");


Hmm, yes, actually I was planning to make that case-insensitive but I'll need to check the other callers first.

malensek requested a balanced review from Copilot September 3, 2026 17:15

Copilot started reviewing on behalf of malensek September 3, 2026 17:16 View session

Copilot AI reviewed Sep 3, 2026

View reviewed changes

Comment thread src/ccontrol/HyriseAdapter.cc

Comment thread src/qproc/testQueryAnaAggregation.cc Outdated

malensek force-pushed the tickets/DM-55685 branch 3 times, most recently from 5bd2755 to 33f082c Compare September 9, 2026 22:21

malensek requested a balanced review from Copilot September 9, 2026 22:27

Copilot started reviewing on behalf of malensek September 9, 2026 22:28 View session

Copilot AI reviewed Sep 9, 2026

View reviewed changes

malensek marked this pull request as ready for review September 9, 2026 22:31

malensek and others added 6 commits October 2, 2026 15:27

Allow queries with nested and bitwise aggregations

bc50b92

* Updates the parser adapter to allow these query types * Adds validation to TablePlugin to ensure aggregations in `HAVING` are present in the `SELECT` list * Adds new unit and integration tests to verify their functionality.

malensek force-pushed the tickets/DM-55685 branch from 33f082c to bc50b92 Compare October 2, 2026 22:29

malensek requested a balanced review from Copilot October 2, 2026 22:30

Copilot started reviewing on behalf of malensek October 2, 2026 22:30 View session

Copilot AI reviewed Oct 2, 2026

View reviewed changes

Navigation Menu

DM-55685: Expanded aggregation support - #1094

DM-55685: Expanded aggregation support#1094
malensek wants to merge 6 commits into
mainfrom
tickets/DM-55685

malensek commented Sep 3, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

malensek Oct 2, 2026

Uh oh!

malensek Oct 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

malensek commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Related issues (existing behavior, being tracked in DM-56009):

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

malensek Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

malensek Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

malensek commented Sep 3, 2026 •

edited

Loading