Conversation
A `HAVING` over a timeseries aggregate was silently discarded: the statement answered every group the `WHERE` clause left, including the ones the predicate excludes, with no error. `TimeseriesRules::plan_aggregate` never read `params.having`, and `SqlPlan::TimeseriesScan` carries no `having` slot, so the predicate built at `engine_rules/params.rs:146` was dropped while the other five engine rules forward it. A second route lost it earlier still: `has_aggregation` returned false when `GROUP BY` was empty and the projection named no aggregate, so `SELECT grp FROM t HAVING COUNT(*) > 1` took the non-aggregate path and no rule saw the clause at all. The clause now fails loudly. `plan_aggregate` refuses a non-empty `having` with a typed error naming the collection and the clause, and `has_aggregation` treats a present `HAVING` as an aggregation signal so the clause always reaches a rule that can judge it. Supporting it would need a new plan field crossing a node boundary and executor-side post-group filtering across every consumer of the scan. The tests walk the whole space rather than the one reported shape: seven collection shapes across six engines plus the default, crossed with four clause shapes, each engine stating its own DDL because they do not agree on what a collection needs. A join aggregate that answers input rows under an aggregate heading is a separate defect, recorded rather than asserted away.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A
HAVINGover a timeseries aggregate now fails loudly instead of returning rows thepredicate excludes. It also reaches the planner by a second route that previously lost it.
Part of #333. That issue lists four ways the timeseries engine drops what the native scan
cannot express; this closes one of them plus a second route to the same defect.
The defect
A
HAVINGover a timeseries aggregate was discarded and the statement answered every groupthe
WHEREclause left — rows the user had explicitly filtered out, with no error.Two causes:
TimeseriesRules::plan_aggregate(nodedb-sql/src/engine_rules/timeseries.rs:104) neverread
params.having, andSqlPlan::TimeseriesScan(types/plan/variants.rs:285) has nohavingslot. The predicate is built atengine_rules/params.rs:146and forwarded by theother five engine rules (
columnar.rs:148,document_schemaless.rs:153,document_strict.rs:153,kv.rs:128,spatial.rs:140) — this one discarded it.has_aggregation(planner/select/select_stmt.rs:382) returnedfalsewhenGROUP BYwas empty and the projection named no aggregate, so
SELECT grp FROM t HAVING COUNT(*) > 1took the non-aggregate path and no rule saw theclause at all.
The fix
nodedb-sql/src/engine_rules/timeseries.rsplan_aggregaterefuses a non-emptyhavingwith a typed error naming the collection and the clausenodedb-sql/src/planner/select/select_stmt.rshas_aggregationtreats a presentHAVINGas an aggregation signalThis refuses; it does not support. Supporting the clause needs a new plan field crossing a
node boundary and executor-side post-group filtering across every consumer of the scan. The
refusal turns a silently wrong answer into an error, which is the honest half of the pair and
what the issue allows. If you would rather the clause worked, say so and I will file the
support as a follow-up and keep the refusal here.
Tests
nodedb/tests/wire/cases/having_matrix_all_engines.rswalks the whole space rather than theone reported shape: seven collection shapes — six engines plus the default a bare
CREATE COLLECTIONselects — crossed with four clause shapes (GROUP BY+HAVING,bare
HAVING,HAVINGover a join, and aWHEREcontrol).Each engine states its whole DDL, because they do not agree on what a collection needs:
kvrequires
PRIMARY KEY,spatialaccepts onlyCOLUMNS (...)with aGEOMETRYcolumn, andtimeseriesneeds aTIME_KEY.nodedb/tests/wire/cases/timeseries_having_predicate.rspins the reported shape and the twocontrols that make the refusal meaningful — the same aggregate without
HAVINGstill answers,and
WHEREstill filters.To test locally:
Evidence
101, the excluded group returned0202, so the assertions read the code0Out of scope
ROLLUP/CUBE,GROUP BY <expression>, and thegeneric row source missing flushed partitions. Each lives in a different planner branch and
needs its own reproduction.
COUNT(*). Found by this matrix whilewriting it: a join whose projection carries an aggregate does not aggregate, on all seven
shapes, with no
HAVINGinvolved. Separate defect class, filed separately with itsreproduction. The matrix records it rather than asserting it away.
On CI status
As of opening this PR, every check that has run is green: Static gates, Analyze Rust, the six
ASan libFuzzer jobs, the 32-bit wasm build, CodeQL and the secret scan.
The workspace
Test Suite / Lint & Checkjob has not run, because it is opt-in:.github/workflows/ci.ymldispatches the reusable
Testworkflow only when the PR carries therun-cilabel. It is not onthis PR. If you want it run before reviewing, add the label or tell me and I will.
That job runs
cargo clippy --workspace --all-targets --all-features --profile ci -- -D warnings,and it currently fails on
mainfor two reasons that are not this diff:1. A clippy lint in a file this branch does not touch.
Reproduced on the base commit, unchanged:
PR #385 is the open fix.
cargo clippy -p nodedb-sql --lib --tests -- -D warnings— the cratethis branch changes — is clean.
2. A dependency advisory, which no branch can lint its way out of.
That is
cargo-denyreadingCargo.lock, and it blocks PR #385 as well — its only failing checkis this one, not the lint it exists to fix. Bumping wasmtime is its own change.
The pre-push hook runs the workspace preflight and therefore blocks on the same clippy lint. This
push used its documented one-time bypass (
PREFLIGHT_SKIP=1), with this section as the statedreason.