Skip to content

Conversation

Tmonster
Copy link
Contributor

fixes https://github.com/duckdblabs/duckdb-internal/issues/5141

#17425 introduced an optimization to replace constant_or_null(true, X) with an is_not_null(X) filter. This was not tested well and assumes that X is just a BOUND_COLUMN_REF. If X is a function (for example coalesce(X, 'value')) then this optimization does not produce the same results.

In the coalesce example, the column X may only contain NULLS, but since it is coalesced with a constant value, the IS NOT NULL filter will produce a different result.

I thought about adding an IsNotNull() filter on top of the ExpressionFilter when propagating the statistics (@propagate_get.cpp:104). This turns the optimization into a constant_or_null into Is not NULL (Coalesce('X', 'value')). The problem there is that constant_or_null can have multiple children, so it would turn into a conjunction and with all of the children.

There is definitely still room to optimize the constant_or_null function, but since this is a bug fix, I tried to keep the diff small.

@duckdb-draftbot duckdb-draftbot marked this pull request as draft June 24, 2025 10:48
@Tmonster Tmonster marked this pull request as ready for review June 25, 2025 12:59
@Mytherin Mytherin merged commit 2b17cee into duckdb:v1.3-ossivalis Jun 26, 2025
51 checks passed
@Mytherin
Copy link
Collaborator

Thanks!

github-actions bot pushed a commit to duckdb/duckdb-r that referenced this pull request Jun 28, 2025
constant or null can be replaced when argument is a bound column reg (duckdb/duckdb#18018)
bump spatial (v1.3) (duckdb/duckdb#18059)
github-actions bot added a commit to duckdb/duckdb-r that referenced this pull request Jun 28, 2025
constant or null can be replaced when argument is a bound column reg (duckdb/duckdb#18018)
bump spatial (v1.3) (duckdb/duckdb#18059)

Co-authored-by: krlmlr <krlmlr@users.noreply.github.com>
@TheoristCoder
Copy link

I have checked this b887779 commit also fixed the bug #17561 I proposed, thanks for your fixing!

@TheoristCoder TheoristCoder mentioned this pull request Jul 5, 2025
2 tasks
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.

3 participants