Fix HAVING clause with time_bucket_gapfill returning wrong rows#9624
Open
Fix HAVING clause with time_bucket_gapfill returning wrong rows#9624
Conversation
|
@antekresic, @kpan2034: please review this pull request.
|
svenklemm
force-pushed
the
sven/gapfill_having
branch
from
8539f75 to
88783de
Compare
antekresic
reviewed
Apr 23, 2026
svenklemm
force-pushed
the
sven/gapfill_having
branch
2 times, most recently
from
3b66e77 to
0b88bc5
Compare
antekresic
approved these changes
Apr 29, 2026
When a HAVING clause was used with gapfill, groups it filtered out disappeared before gapfill could extend them. Queries that filtered out every group returned zero rows, and queries that filtered out some still got gapfilled rows for the filtered buckets. Move the HAVING quals onto the gapfill node so they run on every tuple it produces, real or gapfilled. Gap rows carry NULL aggregates, so normal SQL semantics apply: count(*) > N drops them and count(*) IS NULL keeps them. This only works when each aggregate in HAVING is a top-level entry of the gapfill output. Queries that wrap aggregates with locf or interpolate keep the old behaviour, which is correct as long as HAVING does not eliminate every real group. Fixes #5202
svenklemm
force-pushed
the
sven/gapfill_having
branch
from
0b88bc5 to
74e17dc
Compare
akuzm
reviewed
Apr 29, 2026
Comment on lines
+482
to
+484
| * disappear before gapfill can extend them (#5202). Skipped when an | ||
| * Aggref is not a top-level pathtarget expression (locf/interpolate | ||
| * wraps it), since setrefs cannot resolve it then. |
Member
There was a problem hiding this comment.
What are the implicatioins of this? HAVING still doesn't work if you use locf/interpolate?
Member
There was a problem hiding this comment.
Should locf/interpolate be at the Agg node at all? Maybe we should remove them?
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
3 participants
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.
HAVING quals were attached to the GroupAggregate below the GapFill
node, so filtered-out groups disappeared before gap rows could be
generated. When HAVING eliminated every group the query returned zero
rows; when it eliminated some, GapFill still synthesized NULL rows for
the filtered buckets.
Lift the quals off the aggregate subpath and attach them to the
CustomScan plan's scan.plan.qual, then evaluate them on every tuple
(real and gap-filled) returned from gapfill_exec. Gap rows have NULL
aggregates, so standard SQL HAVING semantics apply: count() > N drops
them, count() IS NULL keeps them.
Fall back to the old behaviour for queries where HAVING references an
Aggref that is not a top-level expression of the GapFill pathtarget
(e.g. HAVING sum(x) > 4 with locf(sum(x)) in the target list), since
set_customscan_references cannot resolve the bare Aggref against
custom_scan_tlist in that case.
Fixes #5202