Skip to content

fix(sql): add missing DuckDB aggregate names, guard against future drift - #1143

Merged
domoritz merged 3 commits into
mainfrom
sql-fix/F-FN-01
Aug 17, 2026
Merged

fix(sql): add missing DuckDB aggregate names, guard against future drift#1143
domoritz merged 3 commits into
mainfrom
sql-fix/F-FN-01

Conversation

@domoritz

Copy link
Copy Markdown
Member

Fixes #1142.

Note: This fix was created by Claude (an agent team ran a binder-error audit of the SQL layer; the fix went through automated implementation, simplification, and adversarial review passes). It is a draft until @domoritz has reviewed it and marks it ready.

What changed

Commit 1 — the fix. Adds the 8 missing names to aggregateNames (alphabetical placement): arg_max_nulls_last, arg_min_nulls_last, argmax, argmin, count_if, group_concat, listagg, sem. Regression tests in test/visitors.test.ts ("Verbatim aggregate detection") build a markQuery-shaped query per name and assert via toBeValidQuery that no GROUP BY is emitted over the aggregate alias and the result binds in DuckDB.

Commit 2 — the guard. New test/aggregate-names.test.ts diffs aggregateNames in both directions against duckdb_functions() WHERE function_type = 'aggregate' on the live test-fixture DuckDB, so this drift class (#1051, and now #1142) is caught mechanically whenever the DuckDB dependency is bumped instead of by user bug reports. Two commented exclusion sets carry the intent: window-only functions that DuckDB reports as aggregates (usable only with OVER, which isAggregateExpression handles separately) plus internal sum_no_overflow, and macro-backed names (geomean, geometric_mean, histogram_values, weighted_avg) that still expand to aggregates. Failure messages print the exact ready-to-paste arrays. A minimal queryFixture helper was added to test/util/validate.ts.

Deliberately not added: the window-only pseudo-aggregates and sum_no_overflow (see exclusion rationale in the test).

Verification

  • pnpm --filter @uwdata/mosaic-sql test: 274/274 pass
  • pnpm --filter @uwdata/mosaic-core test: 63/63 pass (aggregate detection feeds core preaggregation)
  • typecheck + lint clean
  • Revert checks: with src/ast/aggregate.ts reverted, all 8 new detection tests fail with the binder error above, and the drift test fails printing exactly the 8 missing names; with a bogus name injected, the drift test fails in the remove direction.

(“F-FN-01” in the commit messages is the internal finding ID from the audit.)

🤖 Generated with Claude Code

domoritz and others added 2 commits August 11, 2026 22:57
…Fixes F-FN-01.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…N-01.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread packages/mosaic/sql/test/aggregate-names.test.ts
@domoritz
domoritz marked this pull request as ready for review August 13, 2026 20:25
@domoritz
domoritz requested a review from jheer as a code owner August 13, 2026 20:25
@derekperkins

Copy link
Copy Markdown
Collaborator

When I was just working on the function allowlist, duckdb_functions() wasn't quite enough for a full inventory, as several functions are classified as macros. I ended up creating a dedicated AGENTS.md to assist with future updates.

That being said, this is probably sufficient here. I mentioned in my original allowlist PR, it might be useful to put function information in json format elsewhere so any package can look at the function introspection. Right now every core and core extension function is enumerated in the go server, split into compute-only and elevated permissions groups.

domoritz added a commit that referenced this pull request Aug 17, 2026
Fixes #1144.

> **Note:** This fix was created by Claude (an agent team ran a
binder-error audit of the SQL layer; the fix went through automated
implementation, simplification, and adversarial review passes, plus
review notes from @domoritz). It is a draft until @domoritz has reviewed
it and marks it ready.

## What changed

The literal `s.includes(') over ')` check in `isAggregateExpression` is
replaced by a small regex:

```ts
// regexp to match window function calls with inline or named definitions
const windowRegExp = /\)\s*over(\s*\(|\s+[\w"])/;
```

It tolerates arbitrary whitespace (including none) around `over` and
also matches the named-window form (`... over win` / `... over "win"`).
Requiring `(` or whitespace-plus-identifier after `over` avoids false
positives on `OVERLAPS` / `overlay(...)` — which the old literal check
also didn't match.

Regression tests in `test/visitors.test.ts`: the three whitespace
variants plus a single-space control, each asserting
`isAggregateExpression(...) === 0` and binder-validating the generated
markQuery-shaped statement via `toBeValidQuery` (no `GROUP BY` emitted),
plus a named-window case using a `WINDOW` clause.

Out of scope (unchanged behavior): a correctly-classified window
expression combined with a real aggregate in the same mark still hits
vgplot's `markQuery` limitation tracked in #259.

## Verification

- `pnpm --filter @uwdata/mosaic-sql test`: 267/267 pass
- `pnpm --filter @uwdata/mosaic-core test`: 63/63 pass
(`isAggregateExpression` feeds core preaggregation)
- typecheck + lint clean
- Revert check: with `src/visit/visitors.ts` reverted, the
whitespace-variant tests fail with the binder error above.

Heads-up for merging: this PR and #1143 both add tests to
`visitors.test.ts` and will conflict textually — whichever lands second
should keep this branch's module-level `markStyleQuery` test helper and
dedupe.

(“F-FN-02” in the commit message is the internal finding ID from the
audit.)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@domoritz

Copy link
Copy Markdown
Member Author

@derekperkins good points. Let's merge this for now and when we have a good inventory of functions, we can refer to that from a test similar to how we use duckdb_functions().

# Conflicts:
#	packages/mosaic/sql/test/visitors.test.ts
@domoritz
domoritz merged commit a68bf5a into main Aug 17, 2026
5 checks passed
@domoritz
domoritz deleted the sql-fix/F-FN-01 branch August 17, 2026 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

aggregateNames is missing 8 aggregate functions present in current DuckDB

4 participants