Repository navigation
Conversation
Replace correlated frequency queries with grouped MODE aggregates. Preserve null handling and tie-breaking, and cover empty ranges and leading/trailing gaps with regression tests. BREAKING CHANGE: MODE with gapFill=true now includes empty buckets as null-valued results across the inclusive query range. Results may contain more rows and exceed the configured datapoint query limit. Use gapFill=false to retain populated-bucket-only results.
Normalize JSON nulls before casting and remove the fallback that replaced unavailable differences with absolute readings. Document carry-forward semantics and add regression coverage. BREAKING CHANGE: With gap filling enabled, the first reading after a leading empty bucket now yields null instead of its absolute value as the difference.
richturner
left a comment
There was a problem hiding this comment.
🤖 AI-assisted review: this review was prepared with the help of Claude Code and checked by me before posting.
Overall this looks good. I'd approve after a couple of small changes. The fix closes the cast failures from #3403 and adopts the null semantics the issue asked for.
What works well
- The casts no longer fail.
nullif(value, 'null'::jsonb)is applied before every numeric and boolean cast, across all four query types (All, Nearest, LTTB and Interval). - Booleans no longer turn null into false. The
CASE … WHEN true THEN 1 WHEN false THEN 0 ELSE NULL ENDconversion means an average over[true, null]gives 1.0, not 0.5. - Boolean MODE and MEDIAN now return values. The
::integeron these is needed:AbstractDatapointServiceconverts each result toDoublewithgetValueCoerced, and a boolean result came back null there. - DIFFERENCE behaves as the issue asked. Removing the
COALESCE(..., numeric_value)fallback stops a reading after a gap from showing up as a fake jump.locfkeeps its default, so empty buckets still carry the previous value but an explicit null breaks the chain. The test expectations match the0, null, null, null, 10example from the issue. - MODE is simpler and faster. PostgreSQL's built-in
MODE() WITHIN GROUPskips nulls, and the ordering (ASCfor numbers,DESCfor booleans) keeps the existing tie-breaking. - COUNT is unchanged, as the issue requested.
- Test coverage is thorough. It covers historical and predicted storage, NUMBER, BIG_NUMBER and BOOLEAN, both gap-fill settings, all-null and empty ranges, range boundaries, and LTTB point selection compared against a series stored without the nulls.
Should change
AssetDatapointNullQueryTestchanges the JVM time zone and never restores it.setup()callsTimeZone.setDefault(TimeZone.getTimeZone("UTC")). Integration specs share one JVM, so every spec that runs afterwards also runs in UTC. That can hide time-zone bugs elsewhere, or make results depend on test order. At minimum, save the old zone and restore it incleanupSpec. Ideally, find out why this spec needs UTC whenAssetDatapointQueryTestdoesn't. If buckets come out wrong outside UTC, that is a real bug that deserves its own issue rather than being hidden by the test.- The MODE gap-fill change is a behaviour change bundled into a bug fix. It is well documented under "Compatibility / migration" and was clearly needed, because the old CTE was quietly ignoring
gapFill. But it changes results for existing callers and can now hit the default 100,000-point limit on sparse, fine-grained ranges (as the PR's own benchmark shows). I'd like a maintainer to explicitly accept it in this PR, or split it out, and it needs a release note either way.
Nits
- Repeated SQL fragment.
nullif(value, 'null'::jsonb)and the booleanCASEappear about 12 times across four classes. One protected constant inAssetDatapointQuerywould keep the next change consistent. - Undocumented tie-breaking. PostgreSQL's docs say
mode()picks arbitrarily among equally frequent values. In practice it picks the first in sort order, and the tests pin that down. A one-line comment saying the query relies on this would help. - DIFFERENCE now returns null after a leading empty bucket. With
gapFill=true, the first reading after an empty leading bucket now givesnullinstead of its absolute value. I think that's an improvement, because the old result was a fake spike. It is listed in the PR, but UI charts with DIFFERENCE and gap fill on (e.g. energy meters) will now show a gap where they used to show a spike. - Null values are omitted from the JSON. A null
yis left out entirely, so a datapoint serialises as{"x":…}. Gap-filled aggregates already produced this, so the chart code should cope, but a quick check in the UI would be worthwhile.
Points 1 and 2 are the only ones I'd block on.
|
Restore MODE's existing populated-bucket-only behavior for both gapFill settings, retaining JSON-null handling and numeric/boolean tie-breaking. Restore the corresponding expectations and document the MODE limitation. The gap-fill behavior change and its regression coverage will be carried by bugfix/mode-query-gap-fill as a separate follow-up.
|



Stored JSON null values caused numeric and boolean datapoint queries to fail during SQL
casting. Normalize JSON null to SQL NULL before conversion, and preserve unavailable
values during boolean conversion instead of interpreting them as false.
This applies to historical and predicted datapoints:
available values return null. COUNT still counts stored rows, including null-valued rows.
empty buckets still carry the previous bucket value when gap filling is enabled.
When the range starts with empty buckets, the first available reading now produces an unavailable delta instead of its absolute value. Charts may therefore show a gap where they previously showed an artificial spike.
markers; an all-null range returns no points.
MODE now uses a grouped native aggregate, enabling Timescale gap filling. Numeric ties
still select the smaller value and boolean ties select true. Boolean MEDIAN retains its
threshold of at least 0.5 for a true/1 result.
Compatibility / migration
including leading/trailing gaps and entirely empty ranges. A five-minute query from
08:30 through 09:00 with observations only through 08:55 now returns an additional
(09:00, unavailable) bucket. Callers must handle added rows and unavailable values.
Use gapFill=false if only populated buckets are wanted. Fine-grained sparse ranges
can now exceed the configured result limit (default 100000 points).
available reading, rather than its absolute value. Callers must not assume a zero
baseline when the previous bucket is unavailable.
service's numeric format. They now return 1 or 0, so existing callers may receive real
values where they previously saw unavailable results.
Wire format is unchanged: datapoints use x for timestamp and y for value. The configured
JSON mapper omits null properties. A retained unavailable datapoint therefore appears as
{"x":1767225600000}, not necessarily with an explicit "y":null. Clients must handle an
absent y. Both forms deserialize to a null value. LTTB omits the observation entirely.
No database migration or rewrite of stored datapoints is required.
Validation
The core query suites passed 136 tests (132 new null-query cases plus four existing query
features), with none skipped and UTC explicitly configured on the test JVM. Coverage spans
historical/predicted data, NUMBER/BIG_NUMBER/BOOLEAN, both gap-fill settings, null-only and
empty ranges, ties, recovery after unavailable differences, query bounds and real LTTB
point selection. Each implementation step was checked against failing regressions.
The serialization probe also verified omitted y, explicit-null deserialization, and
preservation of zero and false. All 18 adjacent storage/export/deserialization tests
also passed, with none skipped.
MODE performance
Compared the previous null-safe custom MODE query against the grouped native query using
288 captured EXPLAIN ANALYZE plans on PostgreSQL 17.9 / TimescaleDB 2.26.4, with matching
indexes and seven-day chunks. Three measured warm-cache runs followed one warmup, with
old/new execution order alternated. All tested equal-output data queries improved.
For 20000 numeric observations across 4000 buckets, median execution fell from 3123 ms to
9.2 ms: the old plan scanned the 20000-row CTE 4000 times. Large numeric and boolean
buckets also improved despite sort spills. These are synthetic database execution times,
not end-to-end production guarantees.
Sparse minute-level gap filling across a year now generates 525600 rows instead of 365;
the count query increased from 0.55 ms to 56.8 ms and the default limit rejects that range.
All benchmark data and objects were rolled back.