Workflow [PR], commit [a68d39c]

Summary: ✅

• Performance Comparison: Performance dashboard

AI Review

Summary

This PR adds the experimental eval table function and wires it into the analyzer, distributed execution, StorageView, docs, and regression tests. Most of the earlier review findings are fixed in the current code, but one distributed correctness gap remains: the named-collection override form remote(named_collection, database=<table_function>) still bypasses the new table-expression metadata, so the shard-side eval/view handling is incomplete there.

Findings

⚠️ Majors

• [src/TableFunctions/TableFunctionRemote.cpp:401-413; src/TableFunctions/TableFunctionEval.cpp:279-303] getTableExpressionArgumentIndexes still only recognizes the positional remote(..., <table_function>) / cluster(..., <table_function>) form. In the supported named-collection override syntax remote(collection, database=<table_function>), argument 1 is the equals(database, ...) wrapper, so this method returns {} and every consumer that relies on this metadata misses the inner table function. That leaves a live correctness hole in the PR's new distributed behavior: remote(named_collection, database=eval(q)) can still ship eval(q) to the shard without resolving outer WITH aliases on the initiator, and the same metadata gap also means nested eval is not rejected through the equivalent database=... path. The fix needs either a richer representation than flat positional indexes or explicit descent through the database= / db= wrapper in all of the current consumers, plus a regression for the named-collection path (including the serialize_query_plan = 1 variant if that syntax remains supported).

Final Verdict

• Status: ⚠️ Request changes
• Minimum required actions:
  • Handle remote(named_collection, database=<table_function>) consistently in the table-expression metadata / analyzer path instead of only the positional form.
  • Add focused regression coverage for remote(named_collection, database=eval(q)), and for nested eval rejection through the same database=... path if that syntax remains supported.

Hi @alexey-milovidov,

I've addressed the review comments above. The CI is still failing, but the remaining failed tests appear to be unrelated to this change. Could you please take another look when you have a moment? Thanks.

This was fixed by #105146. Let's update the branch.

Merged the latest master into the branch (it was ~3000 commits / 8 days behind) and fixed the one post-merge issue.

Post-merge fix: the development version advanced from 26.6 to 26.7 while this branch was open, and 02995_new_settings_history compares against a v26.6.1 baseline — it only treats a setting as documented when its SettingsChangesHistory entry has a version strictly greater than 26.6. The allow_experimental_eval_table_function entry was in the 26.6 section, so the test would have reported PLEASE ADD THE NEW SETTING TO SettingsChangesHistory.cpp. Moved it to the 26.7 section.

The merge itself was conflict-free. I verified all 12 changed translation units (including the new TableFunctionEval.cpp) still pass -fsyntax-only against a current master build, so there is no API drift from the 8 days of master movement.

Previous CI failures were all unrelated to this PR:

• Integration tests (amd_asan_ubsan, db disk, old analyzer, 5/6): the job was externally canceled (The operation was canceled.) after a ~43-minute hang in an unrelated test. All test_eval_table_function cases passed before the hang.
• Stress test (amd_tsan) / Hung check failed, possible deadlock found: chronic flake #107941 (1326 hits across 539 PRs incl. master in the last 20 days).
• AST fuzzer (amd_debug) / Block structure mismatch in A stream (STID 0993-27f0): chronic flake #108142 (205 hits across 151 PRs incl. master).

CI will re-run on the updated branch.

Addressed the AI review "Request changes" finding on `src/Analyzer/Resolve/QueryAnalyzer.cpp` in 82cc5e9.

Problem: the shard-side wrapper skip path deferred a wrapper such as `loop(eval(q))` to the remote shard without first resolving the inner `eval`'s non-query expression arguments. The direct case `remote(..., eval(q))` already resolves `q` on the initiator (the outer `WITH` alias list is stripped when the AST is shipped to the shard), but `remote(..., loop(eval(q)))` took the `hasShardSideResolvedTableFunctionArguments` branch and sent `eval(q)` to the shard with `q` unresolved → `UNKNOWN_IDENTIFIER`.

Fix: before taking the wrapper skip path, recursively resolve the non-query expression arguments of any `eval` nested inside the wrapper, descending through the wrapper's table-expression arguments (so `loop(loop(eval(q)))` is covered too).

Tests: added `test_remote_loop_eval_resolves_with_alias_argument_on_initiator` and its `serialize_query_plan = 1` variant to `test_eval_table_function`.

Verification (local debug build, single node acting as both initiator and shard via `remote('127.0.0.1:9100', ...)`):

• `loop(eval(q))`, `loop(eval(q))` + `serialize_query_plan = 1`, `loop(eval(a || b))`, and `loop(loop(eval(q)))` all return the expected row (previously `UNKNOWN_IDENTIFIER`).
• Direct `eval(q)` / `eval('literal')` / `loop(eval('literal'))` baselines still return the expected row.
• Stateless tests `04210_eval_table_function`, `04241_eval_table_function_old_analyzer`, `04324_eval_table_function_udf_name` pass against the built binary.

CI: the only red was `Stress test (amd_tsan)`/`(arm_tsan)` → "Hung check failed, possible deadlock found", which the CI summary itself links to the open master-wide flake #107941; all builds and the rest of the suite were green. A fresh CI run is triggered by this push.

Addressed the AI review "Request changes" finding on src/Storages/StorageView.cpp in a7c5299.

Problem: eval builds its inner StorageView from a generated query that may carry its own SETTINGS enable_analyzer = .... The structure is inferred in TableFunctionEval::getActualTableStructure using that inner context, but StorageView::readImpl selected the interpreter (InterpreterSelectQueryAnalyzer vs InterpreterSelectWithUnionQuery) from the outer query's context. So a generated SELECT ... SETTINGS enable_analyzer = 0 under an analyzer-enabled outer query was inferred with the old analyzer but executed with the new one — and the two disagree on, for example, scalar subquery column names (_subquery1 vs _subquery_1), so the executed result could not be converted to the inferred structure.

Fix: build the view's execution context first and select the analyzer mode from view_context->getSettingsRef()[allow_experimental_analyzer]. For eval this is the generated query's context — the same one schema inference uses — so the two paths now agree. For ordinary views the inner and outer settings are identical, so behaviour is unchanged.

This is the analyzer-enabled outer path only. Under the old analyzer an eval view is inlined via StorageView::replaceWithSubquery, so readImpl is not reached and the inverse direction (outer disables the analyzer, generated enables it) is handled by the inlining path, not here.

Test: added 04491_eval_table_function_analyzer_mode, which exercises a generated SETTINGS enable_analyzer = 0 under an analyzer-enabled outer query (returns 42; before the fix the converting step failed because of the _subquery1/_subquery_1 mismatch).

The one CI red on 82cc5e9 was Unit tests (msan, function_prop_fuzzer) / FunctionsStress.stress, which failed on SELECT reinterpret(a, CAST('Decimal(18, 7)' AS String)) ... with function returned column of unexpected type or scale: Decimal64 instead of Decimal(18, 7). That is an unrelated reinterpret-to-Decimal-scale regression on master (introduced by merged #108551), with a fix already in flight in #108878 — it has nothing to do with the eval table function. A fresh CI run is triggered by this push.

Fixed the Fast test failure on 04491_eval_table_function_analyzer_mode in dfb4ea6.

Root cause: the regression I added in the previous commit used an unaliased scalar subquery (SELECT (SELECT 42)) as the generated query's output column. Under the old analyzer the column of an unaliased subquery is named _subquery<N> from a global static std::atomic_uint64_t subquery_index in QueryAliasesVisitor, so the name produced while inferring the structure in TableFunctionEval::getActualTableStructure and the name produced again while executing the inner query in StorageView::readImpl are different counter values (_subquery102 vs another). They never match, so the test failed with Unknown column: _subquery102, there are only columns . (UNKNOWN_IDENTIFIER). This is not specific to eval — a plain CREATE VIEW v AS SELECT (SELECT 42) fails identically under both analyzers, because view-structure inference and view read are separate interpreter runs and the subquery counter advances between them. It was simply the wrong construct to test analyzer-mode consistency with.

Fix: replaced it with a deterministic divergence that still exercises the StorageView::readImpl analyzer-mode selection: SELECT 1 AS a, abs(a). The old analyzer substitutes the alias into the auto-generated name of the second column (abs(1)), while the new analyzer keeps the alias (abs(a)); both names are stable across runs. With the generated query pinning enable_analyzer = 0 under an analyzer-enabled outer query, the structure is inferred as abs(1), so execution must use the old analyzer too — without the readImpl fix it would re-analyze with the new analyzer, produce abs(a), and fail to resolve the inferred abs(1) column. Verified the behavioral facts on a current binary through the equivalent StorageView read path (create-old/read-new fails with UNKNOWN_IDENTIFIER, create-old/read-old works) and that the abs(1) name is stable across repeated runs.

Remaining AI-review finding (remote(collection, database=eval(q))): this is a real but niche gap. TableFunctionRemote::getTableExpressionArgumentIndexes only reports a direct second argument, so for the named-collection override form argument 1 is equals(database, eval(q)) and the method returns {}. Returning {1} alone would not fix it: the consumers (hasShardSideResolvedTableFunctionArguments, checkNoNestedEval, and the shard-side resolution in QueryAnalyzer.cpp) call …->as<ASTFunction>() on the indexed argument and would see equals, not a table function, so they also need to descend into the database=/db= wrapper to find the table-function value. As the review notes, this needs a representation richer than a flat argument index across the interface and all of its consumers — a cross-cutting change to the distributed-analyzer path. Left for a maintainer decision (and a follow-up with the remote(named_collection, database=eval(q)) regression, including the serialize_query_plan = 1 variant), since it touches several analyzer-critical translation units that need a full build to verify.

Addressed the AI review "Request changes" finding on src/Interpreters/ApplyWithSubqueryVisitor.cpp in eae8b08.

Problem: under the old analyzer, ApplyWithSubqueryVisitor substituted WITH literal aliases into the arguments of every ASTFunction named eval, keyed only by the function name. Because this visitor runs before QueryNormalizer applies prefer_column_name_to_alias, a scalar SQL UDF named eval used in an expression context was rewritten too. With a source column q, CREATE FUNCTION eval AS x -> x + 1, and WITH 10 AS q SELECT eval(q) FROM t SETTINGS enable_analyzer = 0, prefer_column_name_to_alias = 1, the argument was rewritten to eval(10) (result 11) instead of using the source column eval(1) (result 2).

Fix: the substitution is now driven from the table-function position (ASTTableExpression::table_function) via the new substituteWithLiteralsInEvalTableFunction, which descends into nested table functions (e.g. loop(eval(q))) but stops at subqueries, so only an actual eval table-function occurrence is rewritten. A scalar SQL UDF with the same name in an ordinary expression context is left to QueryNormalizer, which honours prefer_column_name_to_alias.

Test: added 04492_eval_scalar_udf_prefer_column (old-analyzer regression for a scalar SQL UDF named eval with prefer_column_name_to_alias = 1, plus the new-analyzer mirror and an assertion that the eval table function still substitutes WITH aliases under both analyzers).

Verification (local Debug build): the six covered queries return 2 / 11 / 2 / 11 / 42 / 43 as expected (the regression returned 11 instead of 2 before the fix), and the existing 04210 WITH-alias / 04241 old-analyzer / 04324 scalar-UDF eval cases all still pass.

The remaining AI-review finding (remote(named_collection, database=eval(q))) is unchanged: as noted above it needs a richer table-expression-argument representation across TableFunctionRemote and all of its distributed-analyzer consumers, and is left for a maintainer decision plus a follow-up. A fresh CI run is triggered by this push.

Merged master into the branch to resolve the merge conflict — the branch had fallen ~1216 commits behind and GitHub marked it CONFLICTING/DIRTY, which blocks merging.

This is a pure master merge with no code changes: the net diff versus master is still exactly the eval feature (36 files, all eval sources/tests/docs), and the setting allow_experimental_eval_table_function remains in the 26.7 section of SettingsChangesHistory (dev version is still 26.7, so 02995_new_settings_history stays green). I verified all 17 changed translation units still compile against the merged tree with -fsyntax-only — no API drift from the master churn.

The two Stress test (amd_tsan/arm_tsan) failures on the previous run were the chronic Hung-check flake (#107941), not related to this PR. Fresh CI is now running on the up-to-date base.

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered: 450/481 (93.56%) · Uncovered code

Full report · Diff report