Support subcolumns in default and materialized expressions by Avogar · Pull Request #74403 · ClickHouse/ClickHouse

Avogar · 2025-01-09T18:25:59Z

Changelog category (leave one):

Improvement

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

Support subcolumns in default and materialized expressions

Documentation entry for user-facing changes

Documentation is written (mandatory for new features)

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

CI Settings (Only check the boxes if you know what you are doing)

All builds in Builds_1 and Builds_2 stages are always mandatory
and will run independently of the checks below:

Allow: All Required Checks
Allow: Stateless tests
Allow: Stateful tests
Allow: Integration Tests
Allow: Performance tests
Allow: All Builds
Allow: batch 1, 2 for multi-batch jobs
Allow: batch 3, 4, 5, 6 for multi-batch jobs

Run only fuzzers related jobs (libFuzzer fuzzers, AST fuzzers, BuzzHouse, etc.)
Exclude: AST fuzzers

robot-ch-test-poll1 · 2025-01-09T18:28:20Z

This is an automated comment for commit d16da86 with description of existing statuses. It's updated for the latest CI running

❌ Click here to open a full report in a separate page

Check name	Description	Status
Integration tests	The integration tests report. In parenthesis the package type is given, and in square brackets are the optional part/total tests	❌ failure

Successful checks

Check name	Description	Status
Builds	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Fast test	Normally this is the first check that is ran for a PR. It builds ClickHouse and runs most of stateless functional tests, omitting some. If it fails, further checks are not started until it is fixed. Look at the report to see which tests fail, then reproduce the failure locally as described here	✅ success
Install packages	Checks that the built packages are installable in a clear environment	✅ success
Stateless tests	Runs stateless functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	✅ success
Style check	Runs a set of checks to keep the code style clean. If some of tests failed, see the related log from the report	✅ success
Unit tests	Runs the unit tests for different release types	✅ success

src/Interpreters/addSubcolumnsExtraction.h

src/Interpreters/addSubcolumnsExtraction.cpp

KochetovNicolai · 2025-01-13T15:26:17Z

src/Interpreters/addSubcolumnsExtraction.cpp

+
+            /// Create an alias for getSubcolumn function so it has the name of the subcolumn.
+            const auto & alias_node = extract_subcolumns_dag.addAlias(function_node, input->result_name);
+            extract_subcolumns_dag.addOrReplaceInOutputs(alias_node);


I think it is safer to support the map with added output names. In this case, we can skip adding unused getSubcolumn, and replace this line to extract_subcolumns_dag.getOutputs().push_back(alias_node)

I think it is safer to support the map with added output names.

But how it's possible to add the same name as output twice here? Each added output corresponds to a different subcolumn. Using extract_subcolumns_dag.getOutputs().push_back(alias_node) should be enough.

TBH I used addOrReplaceInOutputs not because I was afraid of duplicates but because I didn't thinking about getOutputs().push_back :)

KochetovNicolai

Overall looks good!

…umns-in-default

clickhouse-gh · 2025-02-07T18:27:26Z

Workflow [PR], commit [a17c45c]

handgunman · 2025-05-10T13:56:07Z

This fix depends on the setting allow_experimental_analyzer=1;
Is that how it should be?

Avogar · 2025-05-12T10:26:46Z

This fix depends on the setting allow_experimental_analyzer=1;
Is that how it should be?

Do ypu have an example when it doesn't work without analyzer?

BTW, analyzer is not experimental anymore and enabled by default since 24.3. There is an alias enable_analyzer.

handgunman · 2025-05-12T12:17:37Z

yes
https://fiddle.clickhouse.com/79f28349-7f8a-480a-83b4-757ded7662ab

Avogar · 2025-05-12T16:04:25Z

I see, it is indeed a problem with old analyzer. But I don't think we want to fix such things with old analyzer, it's better to switch to the new one.

handgunman · 2025-05-12T19:20:51Z

It is not clear why this fix depends on this setting.

Avogar · 2025-05-12T19:23:05Z

The problem is in TreeRewriter implmentation when we read a subcolumn of a materialized column. TreeRewriter is used only in old analyzer.

handgunman · 2025-05-12T19:32:06Z

Okay, I'll wait for the full implementation.

Avogar · 2025-05-12T19:33:25Z

What do you mean by full implementation?
It works in new analyzer, so just use new analyzer.

handgunman · 2025-05-12T19:38:32Z

The allow_experimental_analyzer setting is disabled in the cloud service. For this reason, it doesn't work without setting in queries or profiles. Without this, accessibility will only be possible after removing the dependency on this setting.

Avogar · 2025-05-12T19:43:23Z

The allow_experimental_analyzer setting is disabled in the cloud service.

It is disabled for old services for compatibility. You can test if enabling it doesn't break your queries and enable it by default for your profile or ask the support to enable it by default for the whole service.

handgunman · 2025-05-12T20:15:06Z

I have already asked the service a question. Their position is unequivocal: experimental setting is disabled. I know about the profile and will use it in some cases. Thanks!

Avogar · 2025-05-12T20:16:40Z

Their position is unequivocal: experimental setting is disabled.

That is strange, analyzer is not expiremental anymore for long time already. Maybe they were confused by the setting name allow_experimental_analyzer, you can ask about enable_analyzer setting

handgunman · 2025-05-13T07:34:05Z

if the analyzer is turned on and the experimental one is turned off, the error is reproduced.
https://fiddle.clickhouse.com/4dc3617c-c416-47b4-931a-064d6189ef49

Avogar · 2025-05-13T09:50:15Z

There is no experimental analyzer. enable_analyzer is just an alias to allow_experimental_analyzer that was added when the analyzer became non experimental. The old setting is left for compatibility. In your example you just enabled analyzer and then disabled it just via another setting.

Support subcolumns in default and materialized expressions

30cf047

Avogar requested a review from KochetovNicolai January 9, 2025 18:26

Avogar mentioned this pull request Jan 9, 2025

[Umbrella] JSON type improvements #68428

Open

56 tasks

robot-ch-test-poll3 added the pr-improvement Pull request with some product improvements label Jan 9, 2025

Avogar added 2 commits January 9, 2025 21:06

Fix style check

ac2435a

Fix tests

49a6e20

KochetovNicolai reviewed Jan 13, 2025

View reviewed changes

src/Interpreters/addSubcolumnsExtraction.h Outdated Show resolved Hide resolved

KochetovNicolai reviewed Jan 13, 2025

View reviewed changes

src/Interpreters/addSubcolumnsExtraction.cpp Outdated Show resolved Hide resolved

KochetovNicolai reviewed Jan 13, 2025

View reviewed changes

src/Interpreters/addSubcolumnsExtraction.cpp Outdated Show resolved Hide resolved

KochetovNicolai reviewed Jan 13, 2025

View reviewed changes

KochetovNicolai approved these changes Jan 13, 2025

View reviewed changes

Avogar assigned KochetovNicolai Jan 13, 2025

Avogar added 3 commits January 13, 2025 17:04

Address review comments

9ac3817

Merge branch 'master' of github.com:ClickHouse/ClickHouse into subcol…

bb2e5e9

…umns-in-default

Merge branch 'master' of github.com:ClickHouse/ClickHouse into subcol…

59fc96f

…umns-in-default

Avogar force-pushed the subcolumns-in-default branch from a17dc26 to 4916e3c Compare January 30, 2025 19:39

Fix unneeded changes

b97d8c0

Avogar force-pushed the subcolumns-in-default branch from 4916e3c to b97d8c0 Compare January 30, 2025 19:42

Avogar and others added 4 commits January 30, 2025 19:43

Remove unneeded include

29725ea

Fix build

3957788

Merge branch 'master' of github.com:ClickHouse/ClickHouse into subcol…

d16da86

…umns-in-default

Merge branch 'master' into subcolumns-in-default

a17c45c

Avogar added this pull request to the merge queue Feb 10, 2025

Merged via the queue into ClickHouse:master with commit ece0003 Feb 10, 2025
115 of 116 checks passed

Avogar deleted the subcolumns-in-default branch February 10, 2025 12:41

robot-ch-test-poll3 added the pr-synced-to-cloud The PR is synced to the cloud repo label Feb 10, 2025

KochetovNicolai mentioned this pull request Feb 24, 2025

Fix NOT_FOUND_COLUMN_IN_BLOCK during alter of table with subcolumn in PK #76686

Merged

1 task

Conversation

Avogar commented Jan 9, 2025

Changelog category (leave one):

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

Documentation entry for user-facing changes

CI Settings (Only check the boxes if you know what you are doing)

Uh oh!

robot-ch-test-poll1 commented Jan 9, 2025 • edited by robot-clickhouse-ci-1 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KochetovNicolai Jan 13, 2025

Choose a reason for hiding this comment

Uh oh!

Avogar Jan 13, 2025

Choose a reason for hiding this comment

Uh oh!

KochetovNicolai left a comment

Choose a reason for hiding this comment

Uh oh!

clickhouse-gh bot commented Feb 7, 2025

Uh oh!

Uh oh!

handgunman commented May 10, 2025

Uh oh!

Avogar commented May 12, 2025

Uh oh!

handgunman commented May 12, 2025

Uh oh!

Avogar commented May 12, 2025

Uh oh!

handgunman commented May 12, 2025

Uh oh!

Avogar commented May 12, 2025

Uh oh!

handgunman commented May 12, 2025

Uh oh!

Avogar commented May 12, 2025

Uh oh!

handgunman commented May 12, 2025

Uh oh!

Avogar commented May 12, 2025

Uh oh!

handgunman commented May 12, 2025

Uh oh!

Avogar commented May 12, 2025

Uh oh!

handgunman commented May 13, 2025

Uh oh!

Avogar commented May 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

robot-ch-test-poll1 commented Jan 9, 2025 •

edited by robot-clickhouse-ci-1

Loading