Do not let an alias give randConstant a second value in one query - #115384
Conversation
`randConstant` is documented to hold one value for a whole query, and its argument is documented as the way to ask for more than one. Yet an alias produced a second value: ``` SELECT randConstant() = randConstant() -- 1 SELECT a = b FROM (SELECT randConstant() AS a, randConstant() AS b) -- 0 ``` `RandomConstantOverloadResolver::buildImpl` draws a new value on every call, so identical calls have to share one built `FunctionBase` to fold to one constant. `resolveFunction` arranges that with `functions_cache`, keyed by `getTreeHash`. That hash covers the alias, so the two aliased calls missed each other in the cache, each folded to its own literal, and no later common subexpression elimination could bring them back together - by then they were two different literals. The key now ignores aliases. An alias renames an expression and never changes the value the `FunctionBase` captures, so aliased calls share what bare calls share. What separates two calls is still their arguments, which the hash covers: `randConstant(1)` and `randConstant(2)` keep their own values. The change is narrower than it looks. Only a function that is folded through its own `FunctionBase` can be split this way; anything evaluated per row, such as `rand` or `generateUUIDv4`, is deduplicated later by ordinary common subexpression elimination, which already ignored aliases. A sweep of all 57 zero-argument non-deterministic functions confirms it: comparing every one of them aliased and bare, before and after, `randConstant` is the only function whose result changes, from 0 to 1, and no bare result moves at all. The `04327` and `03047` regression checks were also run, since they read `randConstant` across shards and under mutations, along with `03611`, `01418` and `00912`. Test `05023` fails on exactly its four aliased rows without this change; its five controls - the bare comparison, the distinct-argument row, the across-rows row, and the `now` and `rand` rows - pass either way.
|
Workflow [PR], commit [6259e38] Summary: ✅
AI ReviewSummaryThis PR fixes the analyzer-specific Findings
Final Verdict❌ Changes requested. The alias-insensitive cache still crosses security/context boundaries for LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 4/4 (100.00%) · Uncovered code |
clickhouse-gh
Bot
added
the
pr-bugfix
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
Build profile diff (arm_release)Comparing ✅ No significant changes. Binary sizes
Only the stripped binary is compared: the official master build keeps debug symbols while PR builds strip them, so the other binaries differ by construction. Compile time of recompiled translation units7 translation units recompiled, 15 s compile time in total, 7 of them have a recent master baseline. |
The stateful row of `05023` printed the two `blockNumber` values and expected `0 0`. Where the counter starts is not a property of the fix: it counts how many blocks the server has already passed through that function object, so a warm server reads `6 6`, which is how the row failed in `Fast test`. The invariant the row is about is that the two scalar subqueries do not share one `FunctionBase`: separate instances count separately and read the same number, a shared one keeps counting and hands the second subquery a larger number. So compare the two numbers and print only the comparison.
`master` gained two more `05023_` tests in the meantime.
Yes, we should do this. |
…eterminism The alias-insensitive cache is global across the whole query, so it must only hand out a shared `FunctionBase` for functions whose value is stable inside a query. The name-based exceptions for `getSetting` and `rowNumberInAllBlocks` missed `getSettingOrDefault`, which reads the `SETTINGS` of its own scope just like `getSetting`: two scalar subqueries differing only by alias could reuse one instance and read the first subquery's setting value. Replace the name list with `isDeterministicInScopeOfQuery`, which covers all of them, and add a regression for both `getSetting` and `getSettingOrDefault`.
| /// the hash still covers: `randConstant(1)` and `randConstant(2)` keep their own values, and that | ||
| /// is the documented way to ask for two different constants in one query. | ||
| if (function && !function->isDeterministic() && !function->isStateful() | ||
| && function->isDeterministicInScopeOfQuery()) |
There was a problem hiding this comment.
isDeterministicInScopeOfQuery() is still too weak for this cache key. hasColumnInTable inherits that default, so it now passes the new gate, but the cached FunctionBase still closes over the scope Context via FunctionToFunctionBaseAdaptor. Its executeImpl later does getContext()->checkAccess(...) and DatabaseCatalog::getTable(..., getContext()) in src/Functions/hasColumnInTable.cpp (see lines 103 and 108), so reusing the first scope's instance changes which user and settings the second scope runs under.
That becomes observable now that aliases are ignored: view expansion already analyzes inner queries under getSQLSecurityOverriddenContext in src/Storages/StorageInMemoryMetadata.cpp:146, so two aliased scalar subqueries from different security scopes can now collide in functions_cache and run the second SHOW_COLUMNS check under the first scope's context. That is a privilege/correctness bug, not just a missed optimization guard. I think this cache needs a stronger condition than "value is stable inside a query" here: either mark hasColumnInTable (and similar context-bound constant-folded functions) as not deterministic in query scope, or make the cache require that the built FunctionBase is scope-independent as well as value-stable.
There was a problem hiding this comment.
This is out of scope.
robot-ch-test-poll3
added
the
pr-synced-to-cloud
ReproducerSELECT randConstant() = randConstant(); -- 1
SELECT a = b FROM (SELECT randConstant() AS a, randConstant() AS b); -- 0, expected 1
-- analyzer-only: with enable_analyzer = 0 both read 1Results:
Backport the fix to CC component owner: @KochetovNicolai @novikd Analysis metadata
Introducing change: not identified
|
3 participants
Related: #112938
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fixed
randConstantreturning two different values in one query when its calls carry different aliases.randConstantis documented to hold a single value for the whole query, andSELECT randConstant() = randConstant()already returned1, but adding aliases as inSELECT randConstant() AS a, randConstant() AS bproduced two unrelated values.Description
randConstantis documented to return "a single random value that remains constant across all rows in the current query execution", and its argument is documented as existing only "to prevent common subexpression elimination when the same function call is used multiple times in a query". So two bare calls must agree, and asking for a second value is spelledrandConstant(1), randConstant(2).RandomConstantOverloadResolver::buildImpldraws a new value on every call, so identical calls have to share one builtFunctionBaseto fold to one constant.resolveFunctionarranges that withfunctions_cache, keyed bygetTreeHash. That hash covers the alias, so the two aliased calls missed each other in the cache, each folded to its own literal, and no later common subexpression elimination could bring them back together — by then they were two different literals. Withenable_analyzer = 0the same query returns1, so this is analyzer-only.The key now ignores aliases. An alias renames an expression and never changes the value the
FunctionBasecaptures, so aliased calls share what bare calls share. What separates two calls is still their arguments, which the hash covers.Dropping the alias from the key widens what the cache can match, and stateful functions must not be matched: the cache is global across query scopes, so two scalar subqueries that differ only by an alias would have ended up sharing one
blockNumbercounter and reading two different numbers. Stateful functions are therefore excluded from the cache alongsidegetSettingandrowNumberInAllBlocks, and the last row of the test covers it by comparing the two counters (their absolute values are not a property of the fix - they count the blocks the server has already passed through the function).The change is narrower than the cache's reach suggests. Only a function folded through its own
FunctionBasecan be split this way; anything evaluated per row, such asrandorgenerateUUIDv4, is deduplicated later by ordinary common subexpression elimination, which already ignored aliases —SELECT rand() AS a, rand() AS b, a = breads1onmastertoday. To bound it rather than argue it, I swept all 57 zero-argument non-deterministic functions, comparing each one aliased and bare, before and after:randConstantis the only function whose result changes, from0to1, and no bare result moves at all.Test
05028fails on exactly its four aliased rows without the change. Its five controls — the bare comparison, the distinct-argument row (four arguments rather than two, so it cannot flake on a value collision), the across-rows row, and thenowandrandrows — pass either way.Verified by building and running against the branch build:
05028plus the five existing tests that readrandConstant(03611,01418,00912,04327across shards,03047under mutations) all pass. The 382-testanalyzerfamily was run on both arms of an exact A/B on the same tree — the one-line change reverted, rebuilt, rerun — and the two arms are identical: 352 pass and the same 18 fail on both, all of them for missing fixtures in my local server rather than for this change.Workflow [PR]
Sync PR [sync-upstream/pr/115384]