Add table function obfuscate#42701
Conversation
evillique
force-pushed
the
obfuscate-function
branch
from
758a2d5 to
b5a47fa
Compare
|
Maybe another day ... |
|
We need this. |
|
Continued in #101027 |
|
The mentioned PR is about a function that obfuscates an SQL query. This PR implements a table function that obfuscates the result of a certain query. |
|
Workflow [PR], commit [58cc809] Summary: ✅
AI ReviewSummaryThis PR adds the Missing context / blind spots
Findings
Tests
Final VerdictStatus: Minimum required actions:
|
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
|
Yes, that's true. Didn't notice that. The feature is still needed! |
|
The flaky check failure is fixed in #102148, let's update the branch. |
evillique
force-pushed
the
obfuscate-function
branch
from
f1eaf7b to
53ed445
Compare
# Conflicts: # programs/obfuscator/Obfuscator.cpp # src/Parsers/ExpressionListParsers.cpp
…work # Conflicts: # programs/obfuscator/Obfuscator.cpp
The Style check `test_numbers_check` failed with `Gap (4339, 5017) > 100`: the test `05017_obfuscate_inner_settings_limit_stable` sat far above the dense test-number range. Renumber it to `04401` so the numbering stays gap-free. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`DateTimeModel` preserved the date component using `DateLUT::serverTimezoneInstance`,
but a `DateTime` column can carry its own timezone. On a server in one timezone, a
`DateTime('Asia/Tokyo')` value near local midnight could move to a neighbouring
displayed date, breaking the obfuscator contract that the date component is preserved.
Thread the column timezone (`DataTypeDateTime::getTimeZone`) from `ModelFactory`
into `DateTimeModel` and use it for both `toDate` calls. For a `DateTime` without an
explicit timezone `getTimeZone` falls back to the session/server timezone, so the
common case is unchanged. The model is shared with the `clickhouse obfuscator` tool,
so the fix applies to both and keeps them in sync.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`SETTINGS x = DEFAULT` is parsed into `ASTSetQuery::default_settings`, not into `changes`. Two places ignored it when normalizing a query into a cache key: `ASTSetQuery::updateTreeHashImpl` hashed only `changes`, and `removeQueryResultCacheSettings` dropped the whole `SETTINGS` clause once `changes` became empty. As a result an inner `SETTINGS obfuscate_seed = DEFAULT` (which resets the seed to the empty, non-deterministic value) hashed identically to no override at all. With `query_cache_nondeterministic_function_handling = 'save'` the random-seeded query could write an entry that the deterministic session-seeded query then read. Hash `default_settings` in `updateTreeHashImpl` and keep the `SETTINGS` clause while `default_settings` is non-empty. The `obfuscate` table function is the visible trigger, but the fix is general to any `SETTINGS x = DEFAULT` override. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The obfuscator model code is shared with the `clickhouse obfuscator` tool and does not yet support `LowCardinality`. Instead of an exception of the "not a column" kind or a logical error, it rejects such columns with a clean `NOT_IMPLEMENTED`. Pin this behavior; real `LowCardinality` support is tracked as a follow-up so the table function and the CLI tool stay in sync. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Merged Addressed every item in Minimum required actions:
The two |
| /// `SETTINGS obfuscate_seed = DEFAULT` is stored separately and resets the seed | ||
| /// to its default, which is the empty (non-deterministic) one. | ||
| for (const auto & name : settings_ast->default_settings) | ||
| if (name == "obfuscate_seed") |
There was a problem hiding this comment.
This treats SETTINGS obfuscate_seed = DEFAULT as the effective empty seed, but the analyzer path that actually resolves table functions does not apply ASTSetQuery::default_settings to subquery contexts. QueryTreeBuilder only applies set_query.changes, and QueryAnalyzer::resolveTableFunction switches to a subquery context only when QueryNode::hasSettingsChanges is true. For a subquery that only has SETTINGS obfuscate_seed = DEFAULT, ObfuscateStep still sees the outer/session obfuscate_seed, while this cache check marks the same call as non-deterministic.
A deterministic repro shape is to set an invalid session value such as SET obfuscate_markov_order = 0, then run a nested SELECT ... FROM obfuscate(...) ... SETTINGS obfuscate_markov_order = DEFAULT with the analyzer. If the default were applied, it would use order 5; today the table function still sees 0 and throws from ObfuscateStep.
Please make QueryTreeBuilder apply and carry default_settings for SELECT scopes, or materialize these defaults into the settings changes used by the analyzer/table-function execution context before relying on them in the cache logic.
There was a problem hiding this comment.
Addressed in commit 1bdf2cf: QueryTreeBuilder::buildSelectExpression now applies ASTSetQuery::default_settings to the subquery scope via resetSettingsToDefaultValue, so SETTINGS obfuscate_seed = DEFAULT (and any other = DEFAULT) actually takes effect for the nested table function, matching what the cache logic assumes. Verified locally and covered by new test 04407_obfuscate_inner_settings_default_reset.
There was a problem hiding this comment.
This is still present in the current head for the case where the inner scope contains only = DEFAULT settings.
QueryTreeBuilder now resets the copied QueryNode context in src/Analyzer/QueryTreeBuilder.cpp:318, but it still leaves settings_changes empty. resolveTableFunction only switches execution_context to the nearest subquery scope when query_node->hasSettingsChanges() is true at src/Analyzer/Resolve/QueryAnalyzer.cpp:4786, so a subquery with only SETTINGS obfuscate_markov_order = DEFAULT still executes obfuscate with the outer/session setting.
The new 04407_obfuscate_inner_settings_default_reset test misses this path because it also sets obfuscate_seed = 'stable', which makes hasSettingsChanges() true. Please make the table-function context handoff account for default_settings as well, and add a regression where the inner SETTINGS clause contains only the DEFAULT reset.
There was a problem hiding this comment.
The local analyzer path is fixed now, but the "carry default_settings" part of the original issue is still missing.
QueryTreeBuilder resets the copied subquery context at src/Analyzer/QueryTreeBuilder.cpp:318-319, and resolveTableFunction now notices default-only inner settings. But QueryNode still stores only settings_changes, and QueryNode::toAST still serializes only those changes at src/Analyzer/QueryNode.cpp:565-570.
That means any path that rebuilds SQL from the query tree still loses an inner SETTINGS x = DEFAULT. One concrete case is src/Interpreters/ClusterProxy/executeQuery.cpp:1135-1141, which does query_tree->toAST() before shipping the query. A shape like outer/session obfuscate_markov_order = 0 plus inner SETTINGS obfuscate_markov_order = DEFAULT now succeeds locally, but the serialized AST sent to the remote side still drops the reset and falls back to 0.
So the issue is still present until default_settings are persisted through QueryNode state, hashing, equality, and toAST, not only applied to the local context.
There was a problem hiding this comment.
Confirmed — I traced the paths. QueryNode stores only settings_changes (QueryNode.h:715) and QueryNode::toAST rebuilds the SETTINGS clause purely from those (QueryNode.cpp:565-570), so an inner SETTINGS x = DEFAULT (parsed into ASTSetQuery::default_settings, not changes) is indeed dropped when the query tree is re-serialized to AST. The fix in 1bdf2cff849 and the resolveTableFunction handoff apply the reset to the local execution_context only, reading it back from the original AST; they intentionally do not persist it on the QueryNode (as the comment at QueryAnalyzer.cpp:4788 notes). So a toAST-based shipping path — e.g. the ClusterProxy INSERT ... SELECT at executeQuery.cpp:1136 — would ship the query without the inner reset.
Persisting default_settings through QueryNode state, hashing, equality and toAST is a broad, cross-cutting analyzer change: it touches the query-tree hash/equality contract that is used far beyond obfuscate. That is exactly why this PR keeps the reset in the local execution context rather than widening a feature PR into core analyzer plumbing. I'll keep this thread open to track it as a dedicated follow-up, together with coverage for the distributed re-serialization path — the same way real LowCardinality support is tracked separately.
For the record, this only affects the narrow combination of a distributed query, an inner-only = DEFAULT reset, and an outer/session value that differs from the default; the common local obfuscate(SELECT ...) path is correct and covered (04407_obfuscate_inner_settings_default_reset).
`04402_obfuscate_query_cache_default_seed_key` issues `SYSTEM DROP QUERY CACHE TAG` (for repeatable cleanup across re-runs), which the style check `various_checks.sh` requires to carry a `no-parallel` tag, because any `SYSTEM DROP` touches server-wide shared state. The previous comment claimed the unique `query_cache_tag` made `no-parallel` unnecessary; that is true for functional isolation but does not satisfy the mechanical style rule, so the `Style check` -> `various` job failed with: `Test with SYSTEM DROP should have no-parallel tag`. Add the tag and correct the comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`04403_obfuscate_datetime_timezone` selected `DISTINCT toDate(x, 'Asia/Tokyo')` over an `obfuscate(...)` subquery with no `LIMIT`. Because `obfuscate` is an effectively infinite, repeating source, the outer `SELECT DISTINCT` kept reading the stream until it hit `max_rows_to_read`, so the `Fast test` job failed with `Limit for rows or bytes to read exceeded ... While executing ObfuscateSource. (TOO_MANY_ROWS)` even though the produced value (`2024-01-01`) was already correct. Push a `LIMIT 1000` inside the subquery (next to the obfuscate settings), matching the documented idiom used by `04103_obfuscate_table_function_settings`. One full pass over the 1000 input rows is enough to observe that every obfuscated value keeps the Tokyo date, and the reference output is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The integer obfuscation models permute values inside their log2 bucket using 64-bit arithmetic (`getUInt`/`getInt` and a `UInt64`/`Int64` `Field`), but the `ModelFactory` selected them for every `isInteger` type, including the wide ones. A query such as `obfuscate(SELECT toUInt128(1))` was accepted and then failed with an internal `BAD_GET` while inserting a `UInt64` `Field` into a `ColumnVector` of a wide type. Reject `Int128`/`UInt128`/`Int256`/`UInt256` up front with a clean `NOT_IMPLEMENTED` exception instead. The generic signed path also broke the "keep sign" invariant for narrow minimum values: only `Int64`'s minimum was special-cased, so `Int8`'s `-128` (and the other narrow minimums) could be permuted to a magnitude in `128..255` and narrowed back into a positive value. Pass the source type's minimum into `SignedIntegerModel` and keep it unchanged, as is already done for `Int64`. Both the `obfuscate` table function and the `clickhouse obfuscator` CLI tool share this `ModelFactory`, so the fix applies to both. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…eTime `DateTimeModel` keeps the source date and applies a pseudorandom permutation to the time of day. It computed the obfuscated time as a raw "seconds since midnight" offset inside the generated value's local day and added it to the source date's midnight. On a DST transition day the local day is not 24 hours long, so that offset could exceed the source day's length and the result would spill onto a neighbouring displayed date, breaking the invariant that the obfuscator preserves the date as displayed in the column's timezone. Rebuild the result from the source date plus the obfuscated local wall-clock time in the column's timezone via `DateLUTImpl::makeDateTime`, which always lands inside the source local day. On days without an offset change the result is identical to before. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`SETTINGS x = DEFAULT` is parsed into `ASTSetQuery::default_settings`, not `changes`. When building the query tree for a `SELECT`, `QueryTreeBuilder` applied `changes` to the subquery scope but ignored `default_settings`, so a setting reset to its default inside a nested `SELECT` had no effect on that subquery. For the `obfuscate` table function this meant `SELECT ... FROM obfuscate(...) SETTINGS obfuscate_seed = DEFAULT` still ran with the outer/session value, while the query result cache logic already treated such a reset as effective, leaving the two inconsistent. Reset those settings to their default values in the subquery scope as well, matching how `changes` are applied here and how top-level query settings are handled by `InterpreterSetQuery`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…test `04402_obfuscate_query_cache_default_seed_key` consumed `obfuscate(...)` with an outer `count()` and no `LIMIT`. Because `ObfuscateSource` is an effectively infinite, repeating source, the aggregate kept reading the stream until it hit a read limit, so the test timed out (the `Fast test` `Timeout` failure on this test). Push a `LIMIT 4` inside each inner `obfuscate` subquery, matching the other obfuscate tests; the two queries still differ only by the inner `SETTINGS obfuscate_seed = DEFAULT`, so they still occupy two distinct query-cache entries. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
After merging `master`, the development version is `26.7`, but the seven `obfuscate_*` settings introduced by this PR were still recorded under the `26.6` block (where they were placed when `26.6` was the development version). New settings must be registered under the current version, so move them into the `26.7` block. The entries are otherwise unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Merged CI fixes
AI Review — minimum required actions (all three addressed)
Verification
The |
The AST fuzzer found that a huge `obfuscate_markov_order` (e.g.
`9223372036854775806`) makes the `obfuscate` table function throw an
internal `std::length_error` (reported as a logical error): the Markov
model preallocates a `std::vector` of `order` code points in
`MarkovModel`'s constructor, and such a request exceeds the vector's
`max_size`.
Validate the upper bound of `obfuscate_markov_order` in `ObfuscateStep`
(next to the existing `order == 0` check) and reject values above a sane
maximum of 1000 with a clear `BAD_ARGUMENTS` exception instead. The
order is the Markov context length in code points and the model also
does work proportional to it for every consumed and generated code
point, so larger values are both useless and dangerous.
Failing query:
SELECT * FROM obfuscate(SELECT toString(number) AS s FROM numbers(8))
LIMIT 1 SETTINGS obfuscate_markov_order = 9223372036854775806
CI report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=42701&sha=89e32e03b37dd396290f842685b701f42a6389b7&name_0=PR&name_1=AST%20fuzzer%20%28amd_debug%2C%20targeted%29
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ecution The AI review pointed out that `1bdf2cff849` only fixed half of the `SETTINGS x = DEFAULT` issue: it applies the reset to the subquery's context in `QueryTreeBuilder`, but `QueryAnalyzer::resolveTableFunction` switches the table function's execution context to that subquery context only when `query_node->hasSettingsChanges()` is true. `SETTINGS x = DEFAULT` is parsed into `ASTSetQuery::default_settings`, not `changes`, so `settings_changes` stays empty and `hasSettingsChanges()` is false when the reset is the only inner setting. As a result a table function such as `obfuscate` still executed with the outer/session settings, even though the query result cache already treated the reset as effective. `04407_obfuscate_inner_settings_default_reset` did not catch this because its inner `SETTINGS` also sets `obfuscate_seed = 'stable'`, which makes `hasSettingsChanges()` true and bypasses the failing path. Detect a `DEFAULT`-only reset from the subquery's original AST and treat it like a settings change for the purpose of selecting the execution context. Add `04491_obfuscate_inner_default_only_reset` covering exactly that path (an inner subquery whose only setting is `obfuscate_markov_order = DEFAULT`), which fails before this change and passes after it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Pushed two commits addressing the latest CI failure and the AI Review AST fuzzer ( AI Review blocking finding — inner I verified the analyzer change compiles cleanly under the build flags ( The |
|
The only remaining CI red is This is not related to the
The PR is otherwise AI-Review ✅ Approve on the current head @groeneai, please investigate this flake: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=42701&sha=a65736334f32081b73d21b65dfb65c43bbf335de&name_0=PR&name_1=Fast%20test — there is currently no open tracking issue for the |
|
@alexey-milovidov confirmed: this is the pooled-connection desync family you described (recurrence of the closed #93018; the merged #93029 did not cover the CIDB, last 30 days. The Fix in progress: #108854 ("Fix flaky distributed query ..."). It disconnects out-of-sync pooled connections in |
|
Re-ran the No code changed. The Praktika re-run re-dispatched the build/test pipeline, which is re-running on the same head; the substantive blocker (the flake) is resolved. AI Review is ✅ Approve, and the only open review thread is the intentional |
|
The full CI pipeline re-dispatched by the This is not caused by this PR — it touches no AI Review is ✅ Approve and the only open review thread is the intentional |
|
Merged the current GitHub was reporting this PR as
The remaining CI red on the fresh pipeline is expected to be the master-wide |
LLVM Coverage ReportChanged lines: Changed C/C++ lines covered: 601/725 (82.90%) · Uncovered code |
5 participants
Closes: #39067
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Add table function
obfuscatewhich applies the same transformation as theclickhouse-obfuscatortool to the result of an arbitrary query, e.g.SELECT * FROM obfuscate(SELECT * FROM table). The seed can be controlled with theobfuscate_seedsetting; with an empty seed a fresh random one is derived per execution.