Skip to content

fix(fumadb): mirror aggregate review hardening#62

Merged
aryasaatvik merged 1 commit into
devfrom
fix/dev-fumadb-aggregate-review-fixes
Jun 26, 2026
Merged

fix(fumadb): mirror aggregate review hardening#62
aryasaatvik merged 1 commit into
devfrom
fix/dev-fumadb-aggregate-review-fixes

Conversation

@aryasaatvik

Copy link
Copy Markdown
Owner

Summary

Mirrors the review-hardening deltas from upstream RhysSullivan/executor#1119 onto dev.

dev already contains the broader FumaDB aggregate and keyset query feature, so this PR only carries the remaining drift from the upstream review loop.

Changes

  • Preserve memory and Drizzle parity for empty composite filters by compiling empty JSON or filters to a constant false SQL predicate.
  • Add regression coverage for empty or and empty and filters across the aggregate harness.
  • Replace nested plugin-storage operator selection with an explicit typed JSON compare-operator map.
  • Document SQLite percentile behavior on the public FumaDB and plugin-storage stats inputs.

Intentional Differences From Upstream RhysSullivan#1119

  • This PR does not re-add the full aggregate and keyset implementation because dev already has that feature surface.
  • This PR does not touch the OpenAPI storage facade mock because dev already has the mock shape needed by the expanded collection facade.
  • This PR has no changeset because it only mirrors fixes to an existing dev feature surface.

Tests

  • bun run bootstrap
  • bun run --cwd packages/core/fumadb test -- src/query/aggregate.test.ts src/query/table-policy.test.ts
  • bun run --cwd packages/core/sdk test -- src/plugin-storage-aggregate.test.ts src/plugin-storage.test.ts
  • bun run --cwd packages/core/fumadb typecheck
  • bun run --cwd packages/core/sdk typecheck
  • bun run typecheck
  • ./node_modules/.bin/oxfmt --check packages/core/fumadb/src/adapters/drizzle/query.ts packages/core/fumadb/src/query/aggregate.test.ts packages/core/fumadb/src/query/aggregate.ts packages/core/sdk/src/executor.ts packages/core/sdk/src/plugin-storage.ts

@greptile-apps

greptile-apps Bot commented Jun 24, 2026

Copy link
Copy Markdown

Greptile Summary

This PR ports four targeted review-hardening fixes from upstream onto dev's existing FumaDB aggregate surface: the main logic fix corrects empty or filter semantics in the Drizzle adapter, the operator-mapping refactor in the plugin-storage path removes a subtle default-to-"=" fallback for unrecognised operators, and the remaining changes add regression tests and documentation.

  • Empty or Drizzle fix (query.ts): Empty { kind: "or", items: [] } now returns Drizzle.sql\1 = 0`instead ofundefined, matching the in-memory [].some() === falsesemantic. The emptyandcase already worked correctly viaDrizzle.and()returningundefined` (treated as no-constraint = vacuously true). Both paths are exercised by the new test against both the memory and SQLite harnesses.
  • Typed compare-operator map (executor.ts): Replaces a deeply-nested ternary with a satisfies-constrained map and a guard that continues on any operator not in the map, rather than silently falling through to "=". Since only the five known scalar operators can reach that branch in practice, the guard is a safety net rather than a behaviour change for valid inputs.
  • SQLite percentile documentation (aggregate.ts, plugin-storage.ts): Adds a JSDoc note clarifying that SQLite computes percentiles via post-projection in application code, not as a native SQL aggregate.

Confidence Score: 5/5

Safe to merge. The Drizzle adapter fix is a single-line guard that corrects empty or to produce 1 = 0; the empty and path already behaved correctly without an explicit check. The operator-map refactor in the plugin-storage path is a strict improvement with no behaviour change for valid inputs.

Both changed logic paths have direct test coverage across memory and SQLite harnesses. The or fix mirrors what [].some() already did in the in-memory evaluator. The executor refactor replaces error-prone ternary fallthrough with a typed map and a safe continue guard. Documentation changes are accurate and scoped to the affected interfaces.

No files require special attention. query.ts and executor.ts carry the only logic changes, both of which are minimal and well-tested.

Important Files Changed

Filename Overview
packages/core/fumadb/src/adapters/drizzle/query.ts Adds an explicit early return of 1 = 0 for empty or filters, correctly mirroring the in-memory [].some()false semantic that was already correct in aggregate-eval.ts.
packages/core/fumadb/src/query/aggregate.test.ts Adds a regression test for empty composite filter semantics (empty or → 0 rows, empty and → 4 rows) executed against both the memory and SQLite adapters via the shared runSuite harness.
packages/core/fumadb/src/query/aggregate.ts Documentation-only change: adds SQLite percentile behavior note to JsonStatsAdapterOptions.percentiles and JsonStatsOptions.percentiles.
packages/core/sdk/src/executor.ts Replaces nested ternary operator-mapping with a typed pluginStorageJsonCompareOperators map and adds a guard that skips unknown operators instead of silently mapping them to "=". Import of JsonCompareOperator added to support the satisfies constraint.
packages/core/sdk/src/plugin-storage.ts Documentation-only change: adds SQLite percentile behavior note to PluginStorageStatsInput.percentiles.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[buildJsonFilter called] --> B{filter.kind?}
    B -- and --> C[Drizzle.and items.map ...]
    C --> D{"items empty?"}
    D -- yes --> E[Drizzle.and returns undefined]
    D -- no --> F[SQL AND expression]
    E --> G[buildScopedConditions: if compiled skip push]
    G --> H[no JSON constraint = vacuously true]
    F --> I[pushed to parts]
    B -- or --> J{"items.length === 0?"}
    J -- yes --> K["return sql 1=0 NEW FIX"]
    K --> L[pushed to parts always false]
    J -- no --> M[Drizzle.or items.map ...]
    M --> N[SQL OR expression]
    B -- compare / array --> O[leaf SQL predicate]
    subgraph executor.ts pluginStorageWhereToJsonFilter
      P[iterate condition entries] --> Q{"operator === in?"}
      Q -- yes --> R[push array filter, continue]
      Q -- no --> S{"operator in pluginStorageJsonCompareOperators?"}
      S -- no --> T[continue skip NEW GUARD]
      S -- yes --> U[look up operator from map]
      U --> V[push compare filter]
    end
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[buildJsonFilter called] --> B{filter.kind?}
    B -- and --> C[Drizzle.and items.map ...]
    C --> D{"items empty?"}
    D -- yes --> E[Drizzle.and returns undefined]
    D -- no --> F[SQL AND expression]
    E --> G[buildScopedConditions: if compiled skip push]
    G --> H[no JSON constraint = vacuously true]
    F --> I[pushed to parts]
    B -- or --> J{"items.length === 0?"}
    J -- yes --> K["return sql 1=0 NEW FIX"]
    K --> L[pushed to parts always false]
    J -- no --> M[Drizzle.or items.map ...]
    M --> N[SQL OR expression]
    B -- compare / array --> O[leaf SQL predicate]
    subgraph executor.ts pluginStorageWhereToJsonFilter
      P[iterate condition entries] --> Q{"operator === in?"}
      Q -- yes --> R[push array filter, continue]
      Q -- no --> S{"operator in pluginStorageJsonCompareOperators?"}
      S -- no --> T[continue skip NEW GUARD]
      S -- yes --> U[look up operator from map]
      U --> V[push compare filter]
    end
Loading

Reviews (1): Last reviewed commit: "fix(fumadb): mirror aggregate review har..." | Re-trigger Greptile

@aryasaatvik aryasaatvik merged commit fe84352 into dev Jun 26, 2026
8 checks passed
@aryasaatvik aryasaatvik deleted the fix/dev-fumadb-aggregate-review-fixes branch June 26, 2026 13:04
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.

1 participant