Skip to content

Support SQL standard VALUES clause as a table expression#100143

Merged
Avogar merged 26 commits intoClickHouse:masterfrom
Desel72:fix/issue-99704-v2
Mar 25, 2026
Merged

Support SQL standard VALUES clause as a table expression#100143
Avogar merged 26 commits intoClickHouse:masterfrom
Desel72:fix/issue-99704-v2

Conversation

@Desel72
Copy link
Copy Markdown
Contributor

@Desel72 Desel72 commented Mar 19, 2026

Adds support for the SQL standard VALUES clause syntax: SELECT * FROM (VALUES (1, 'a'), (2, 'b')) AS t(id, val)

This is rewritten internally to use the existing values table function, so all existing type inference and column naming logic is reused.

Closes #99605

Changelog category (leave one):

  • New Feature

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Support SQL standard VALUES clause as a table expression in FROM, e.g. SELECT * FROM (VALUES (1, 'a'), (2, 'b')) AS t(id, val).

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

Adds support for the SQL standard `VALUES` clause syntax:
`SELECT * FROM (VALUES (1, 'a'), (2, 'b')) AS t(id, val)`

This is rewritten internally to use the existing `values` table function,
so all existing type inference and column naming logic is reused.

Closes ClickHouse#99605

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@alexey-milovidov alexey-milovidov added the can be tested Allows running workflows for external contributors label Mar 19, 2026
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented Mar 19, 2026

Workflow [PR], commit [954d1d3]

Summary:

job_name test_name status info comment
BuzzHouse (amd_msan) failure
MemorySanitizer: use-of-uninitialized-value (STID: 3229-5315) FAIL cidb, issue ISSUE CREATED

AI Review

Summary

This PR adds support for SQL-standard VALUES in FROM by rewriting (VALUES ...) into an internal SQLStandardValues table function. The implementation direction is good, but there are blockers around rollout policy and one parser-semantic issue: the feature is enabled unconditionally (no experimental gate), and the parser currently accepts non-standard row forms like (VALUES 1, 2).

PR Metadata
  • ⚠️ Changelog category is currently New Feature, but this change is intended as experimental (docs mention allow_experimental_sql_standard_values_clause). It should be Experimental Feature.
  • ⚠️ Changelog entry text is clear, but it should explicitly state experimental status and how to enable it.

Exact replacement:

  • Changelog category: - Experimental Feature
  • Changelog entry: Add experimental support for SQL-standard \ \VALUES` in
    `FROM` (e.g.
    `SELECT * FROM (VALUES (1, 'a')) AS t(id, val)`). Enable with
    `allow_experimental_sql_standard_values_clause = 1`.`
Missing context
  • ⚠️ No CI report/log links were provided in the review input, so runtime/regression signals could not be cross-checked.
Findings
  • ❌ Blockers

    • [src/Parsers/ExpressionElementParsers.cpp:194] SQL-standard VALUES support is introduced unconditionally in parser logic, but this is a new behavior and should be guarded behind an experimental setting for safe rollout.
      • Suggested fix: gate this branch by allow_experimental_sql_standard_values_clause (or equivalent parser-accessible setting) and keep default behavior unchanged when disabled.
  • ⚠️ Majors

    • [src/Parsers/ExpressionElementParsers.cpp:202] The parser uses ParserExpression directly for each VALUES item, so non-standard forms like (VALUES 1, 2) are accepted. SQL-standard form requires row constructors.
      • Suggested fix: require each item to be a parenthesized row expression before adding it to SQLStandardValues arguments.
    • [docs/en/sql-reference/statements/select/from.md:24] The new example shows VALUES usage without clearly stating/enforcing the experimental gate, which can mislead users.
      • Suggested fix: add explicit note or setup line with SET allow_experimental_sql_standard_values_clause = 1;.
    • [docs/en/sql-reference/table-functions/values.md:198] Docs still say rewrite goes to values, while code rewrites to SQLStandardValues.
      • Suggested fix: update wording to reflect the actual internal rewrite target.
ClickHouse Rules
Item Status Notes
Deletion logging
Serialization versioning
Core-area scrutiny
No test removal
Experimental gate New parser behavior is enabled by default
No magic constants
Backward compatibility ⚠️ New syntax behavior changes parser acceptance unconditionally
SettingsChangesHistory.cpp
PR metadata quality ⚠️ Category/entry should reflect experimental rollout
Safe rollout ⚠️ Missing feature flag gate in implementation
Compilation time
Final Verdict
  • Status: ⚠️ Request changes
  • Minimum required actions:
    • Add an experimental gate for SQL-standard VALUES parser behavior.
    • Tighten parser to require row-constructor form for VALUES items (or explicitly document and justify broader grammar).
    • Align docs and PR metadata with actual rollout behavior.

@clickhouse-gh clickhouse-gh Bot added the pr-feature Pull request with new product feature label Mar 19, 2026
Copy link
Copy Markdown
Member

@alexey-milovidov alexey-milovidov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good.

@alexey-milovidov alexey-milovidov self-assigned this Mar 19, 2026
Comment thread src/Parsers/ExpressionElementParsers.cpp
@Desel72
Copy link
Copy Markdown
Contributor Author

Desel72 commented Mar 20, 2026

Hi @alexey-milovidov
I think failure isn't related with me. Could you please review this?

@alexey-milovidov
Copy link
Copy Markdown
Member

@Desel72, it shows that some additions to the docs are needed for a new feature. At least mention this feature in the docs in the appropriate place.

Comment thread src/Parsers/ExpressionElementParsers.cpp
Desel72 and others added 4 commits March 21, 2026 23:36
…ix/issue-99704-v2

# Conflicts:
#	ci/jobs/scripts/check_style/aspell-ignore/en/aspell-dict.txt
…House into fix/issue-99704-v2

# Conflicts:
#	ci/jobs/scripts/check_style/aspell-ignore/en/aspell-dict.txt
…rd_values_clause` setting

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Comment thread docs/en/sql-reference/statements/select/from.md
-- https://github.com/ClickHouse/ClickHouse/issues/99605

SET enable_analyzer = 1;
SET allow_experimental_sql_standard_values_clause = 1;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add targeted negative/regression coverage for the parser rewrite edge cases:

  1. A query that validates a first-row single-string literal is treated as row data (not as values structure), e.g. (VALUES ('x UInt8'), (2)).
  2. A query that asserts the syntax is rejected when allow_experimental_sql_standard_values_clause = 0.

Right now the test only covers happy-path enabled usage, so these two important failure modes can regress silently.

Comment thread src/Core/Settings.cpp Outdated
DECLARE(Bool, allow_experimental_ytsaurus_dictionary_source, false, R"(
Experimental dictionary source for integration with YTsaurus.
)", EXPERIMENTAL) \
DECLARE(Bool, allow_experimental_sql_standard_values_clause, false, R"(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR marks this as New Feature, but this syntax is explicitly gated by allow_experimental_sql_standard_values_clause. Please switch the PR template Changelog category to Experimental Feature to match ClickHouse rollout policy.

@Avogar Avogar self-assigned this Mar 24, 2026
Comment thread src/Core/Settings.cpp Outdated
DECLARE(Bool, allow_experimental_ytsaurus_dictionary_source, false, R"(
Experimental dictionary source for integration with YTsaurus.
)", EXPERIMENTAL) \
DECLARE(Bool, allow_experimental_sql_standard_values_clause, false, R"(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why AI decided to ask you to put it under experimental setting. The feature is not that big and complex to do it. Let's remove the setting and allow this syntax out of the box. Ignore AI comments about experimental setting

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, remove this setting, as I said, it's not needed

Comment thread docs/en/sql-reference/table-functions/values.md
Comment thread src/Core/Settings.cpp Outdated
Experimental dictionary source for integration with YTsaurus.
)", EXPERIMENTAL) \
DECLARE(Bool, allow_experimental_sql_standard_values_clause, false, R"(
Enables support for the SQL standard VALUES clause as a table expression in FROM, e.g. SELECT * FROM (VALUES (1, 'a'), (2, 'b')) AS t(id, val). Internally rewritten to the existing values table function.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setting description says the syntax is rewritten to the existing values table function, but this PR now rewrites to SQLStandardValues. Please update this text to match the actual implementation to avoid confusing users and future maintainers.

Suggested wording: "Internally rewritten to the internal SQLStandardValues table function."

Comment thread src/Core/Settings.cpp Outdated
DECLARE(Bool, allow_experimental_ytsaurus_dictionary_source, false, R"(
Experimental dictionary source for integration with YTsaurus.
)", EXPERIMENTAL) \
DECLARE(Bool, allow_experimental_sql_standard_values_clause, false, R"(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, remove this setting, as I said, it's not needed

Comment thread src/TableFunctions/TableFunctionValues.cpp
/// SQL standard VALUES clause: (VALUES (1, 'a'), (2, 'b'))
/// Rewritten by the parser to SQLStandardValues(tuple(1, 'a'), tuple(2, 'b'))
/// This checks the experimental setting and never interprets the first arg as schema.
class TableFunctionSQLStandardValues : public ITableFunction
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can avoid creating new class for this, just make TableFunctionValues template with a bool flag:

template <bool interpret_first_argument_as_structure>
class TableFunctionValues : public ITableFunction

And check this template argument in all appropriate places, also function name can depend on templat argument.

…put getTypesFromArgument in anonymous namespace
ParserExpression expr_parser;

ASTPtr value_expr;
if (!expr_parser.parse(pos, value_expr, expected))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ParserExpression here accepts any expression, so (VALUES 1, 2) is parsed successfully even though SQL-standard VALUES requires row constructors (expr, ...) per row.

This makes the new syntax less strict than documented and can introduce parsing ambiguities with future grammar extensions. Please require each item after VALUES to be a parenthesized row expression (or at minimum reject non-parenthesized items) before rewriting to SQLStandardValues.

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented Mar 25, 2026

LLVM Coverage Report

Metric Baseline Current Δ
Lines 84.10% 84.10% +0.00%
Functions 24.50% 24.60% +0.10%
Branches 76.70% 76.70% +0.00%

PR changed lines: PR changed-lines coverage: 97.18% (69/71, 0 noise lines excluded)
Diff coverage report
Uncovered code

@Desel72
Copy link
Copy Markdown
Contributor Author

Desel72 commented Mar 25, 2026

@Avogar Thanks
MSan failure is in FileSegment::getDownloadedSize called from CachedOnDiskWriteBufferFromFile::appendFilesystemCacheLog during CREATE TABLE. This is a pre-existing issue in the filesystem cache layer, unrelated to the parser/table function changes in this PR.

@Avogar Avogar added this pull request to the merge queue Mar 25, 2026
Merged via the queue into ClickHouse:master with commit 438e5c9 Mar 25, 2026
151 of 153 checks passed
@robot-ch-test-poll2 robot-ch-test-poll2 added the pr-synced-to-cloud The PR is synced to the cloud repo label Mar 26, 2026
@Desel72
Copy link
Copy Markdown
Contributor Author

Desel72 commented Mar 26, 2026

Thanks @Avogar

@zlareb1
Copy link
Copy Markdown
Member

zlareb1 commented Mar 26, 2026

@Desel72 small doc update required #100791

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

can be tested Allows running workflows for external contributors pr-feature Pull request with new product feature pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support FROM (VALUES (...), ...) AS alias(col, ...) table value constructor

5 participants