fix CTE deterministic functions#86967
Conversation
clickhouse-gh
bot
added
the
pr-bugfix
…ic in scope of query
devcrafter
left a comment
devcrafter
left a comment
There was a problem hiding this comment.
LGTM from my side, but let's confirm with @novikd
| @@ -0,0 +1,2 @@ | |||
| with (select randConstant()) as a select a = a; | |||
| with (select now() + sleep(1)) as a select a = a; | |||
There was a problem hiding this comment.
Please add test where select has more than 1 parent select, and check that such function is executed once and the calculated value used everywhere in the query
| { | ||
| /// This is a hack to allow a query like `select randConstant(), randConstant(), randConstant()`. | ||
| /// Function randConstant() would return the same value for the same arguments (in scope). | ||
| /// Also we want to use most outer scope for function deterministic in scope of query. |
There was a problem hiding this comment.
So, we store a function value in most outer scope, i.e., globally for the query on a local node. Logically looks as expected behavior, but I'd double-check with somebody who can know better - @novikd @KochetovNicolai
| IdentifierResolveScope * function_scope = &scope; | ||
| if (resolver && resolver->isDeterministicInScopeOfQuery()) | ||
| while (function_scope->parent_scope) | ||
| function_scope = function_scope->parent_scope; |
There was a problem hiding this comment.
If we always use the most outer scope then maybe it'd be better to put the cache into the query context or into QueryAnalyzer.
It doesn't make sense to keep it in each scope if we only fill the most outer one.
There was a problem hiding this comment.
we only do it for isDeterministicInScopeOfQuery() functions, for non-deterministic-in-scope functions we use local scope - I don't think it makes sense to create special cache for this - it's logically consistent that upper scope is also a global scope for the whole query
|
|
||
| String getName() const override { return name; } | ||
| bool isDeterministic() const override { return false; } | ||
| bool isDeterministicInScopeOfQuery() const override { return false; } |
There was a problem hiding this comment.
It disables constant folding for this function, doesn't it?
There was a problem hiding this comment.
not sure about this - is it required for such functions to forfeit constant folding? not obvious to me...
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
robot-clickhouse-ci-1
added
the
pr-synced-to-cloud
yakov-olkhovskiy
added
the
pr-must-backport
robot-ch-test-poll3
added
pr-backports-created-cloud
Cherry pick #86967 to 25.9: fix CTE deterministic functions
Backport #86967 to 25.9: fix CTE deterministic functions
robot-clickhouse-ci-2
added
the
pr-backports-created
Backport #86967 to 25.7: fix CTE deterministic functions
Backport #86967 to 25.8: fix CTE deterministic functions
Backport #86967 to 25.6: fix CTE deterministic functions
7 participants
Changelog category (leave one):
Changelog entry (a [user-readable short description]
Fix result of function calculated in CTE being non-deterministic in the query.
closes #86591