Add Simple Streaming implementation by Michicosun · Pull Request #57830 · ClickHouse/ClickHouse

Michicosun · 2023-12-13T20:26:09Z

Changelog category (leave one):

New Feature

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

add streaming mode for every table engine via 2 processors in pipeline

example select:

set allow_experimental_streaming=1;
set allow_experimental_analyzer=1;

CREATE TABLE test (a String, b UInt64) ENGINE = MergeTree() ORDER BY a;

-- infinite query
SELECT a, b * b FROM test STREAM;

insert works as usual

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/

…/42990/simple_streaming

robot-ch-test-poll · 2023-12-13T22:21:11Z

This is an automated comment for commit 210d0b7 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
A Sync	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	❌ failure
CI running	A meta-check that indicates the running CI. Normally, it's in success or pending state. The failed status indicates some problems with the PR	⏳ pending
Mergeable Check	Checks if all other necessary checks are successful	❌ failure
Performance Comparison	Measure changes in query performance. The performance test report is described in detail here. In square brackets are the optional part/total tests	❌ failure
Stress test	Runs stateless functional tests concurrently from several clients to detect concurrency-related errors	❌ failure
Upgrade check	Runs stress tests on server version from last release and then tries to upgrade it to the version from the PR. It checks if the new server can successfully startup without any errors, crashes or sanitizer asserts	❌ failure

Successful checks

Check name	Description	Status
AST fuzzer	Runs randomly generated queries to catch program errors. The build type is optionally given in parenthesis. If it fails, ask a maintainer for help	✅ success
ClickBench	Runs [ClickBench](https://github.com/ClickHouse/ClickBench/) with instant-attach table	✅ success
ClickHouse build check	Builds ClickHouse in various configurations for use in further steps. You have to fix the builds that fail. Build logs often has enough information to fix the error, but you might have to reproduce the failure locally. The cmake options can be found in the build log, grepping for cmake. Use these options and follow the general build process	✅ success
Compatibility check	Checks that clickhouse binary runs on distributions with old libc versions. If it fails, ask a maintainer for help	✅ success
Docker keeper image	The check to build and optionally push the mentioned image to docker hub	✅ success
Docker server image	The check to build and optionally push the mentioned image to docker hub	✅ success
Docs check	Builds and tests the documentation	✅ 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
Flaky tests	Checks if new added or modified tests are flaky by running them repeatedly, in parallel, with more randomization. Functional tests are run 100 times with address sanitizer, and additional randomization of thread scheduling. Integrational tests are run up to 10 times. If at least once a new test has failed, or was too long, this check will be red. We don't allow flaky tests, read the doc	✅ success
Install packages	Checks that the built packages are installable in a clear environment	✅ success
Integration tests	The integration tests report. In parenthesis the package type is given, and in square brackets are the optional part/total tests	✅ success
PR Check	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Stateful tests	Runs stateful functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	✅ 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

…/42990/simple_streaming

al13n321

The changes look great! Sorry for the slow response. Please resolve the conflicts, update SettingsChangesHistory.h, optionally address the new nitpicks, and should be good to go.

al13n321 · 2024-06-28T00:18:35Z

src/Analyzer/TableExpressionModifiers.cpp

 {
+    buffer << "stream: " << has_stream;
+
    buffer << "final: " << has_final;


Suggested change

buffer << "final: " << has_final;

buffer << ", final: " << has_final;

al13n321 · 2024-06-28T00:22:32Z

src/Analyzer/TableExpressionModifiers.cpp

+        buffer << "STREAM";
+
    if (has_final)
        buffer << "FINAL";


Need a space between STREAM and FINAL, and update the conditions for the two buffer << ' ' calls below (or extract it into a helper lambda add_space_if_needed() or something). (Though ... STREAM FINAL is not a valid query, it's not obvious that this method can't get called before the query is rejected.)

al13n321 · 2024-06-28T00:29:52Z

src/Core/SettingsChangesHistory.h

+              }},
    {"24.3", {{"s3_connect_timeout_ms", 1000, 1000, "Introduce new dedicated setting for s3 connection timeout"},
              {"allow_experimental_shared_merge_tree", false, true, "The setting is obsolete"},
+              {"allow_experimental_streaming", false, false, "Allow to use Streaming Queries"},


Need to move it to the current month. (We'll probably merge this within a month, so it won't have to be moved again.)

al13n321 · 2024-06-28T00:42:44Z

src/Planner/CollectTableExpressionData.cpp

+
+            if (table_expression_modifiers && table_expression_modifiers->hasStream())
+                throw Exception(ErrorCodes::ILLEGAL_PREWHERE,
+                    "PREWHERE does not supported for Streaming Queries");


Suggested change

"PREWHERE does not supported for Streaming Queries");

"PREWHERE is not supported for Streaming Queries");

al13n321 · 2024-06-28T00:43:45Z

src/Planner/CollectTableExpressionData.cpp

                storage->getStorageID().getNameForLogs());
+        if (table_node->hasTableExpressionModifiers() && table_node->getTableExpressionModifiers()->hasStream())
+            throw Exception(ErrorCodes::ILLEGAL_PREWHERE,
+                "PREWHERE does not supported for Streaming Queries");


Suggested change

"PREWHERE does not supported for Streaming Queries");

"PREWHERE is not supported for Streaming Queries");

al13n321 · 2024-06-28T00:43:52Z

src/Planner/CollectTableExpressionData.cpp

                storage->getStorageID().getNameForLogs());
+        if (table_function_node->hasTableExpressionModifiers() && table_function_node->getTableExpressionModifiers()->hasStream())
+            throw Exception(ErrorCodes::ILLEGAL_PREWHERE,
+                "PREWHERE does not supported for Streaming Queries");


Suggested change

"PREWHERE does not supported for Streaming Queries");

"PREWHERE is not supported for Streaming Queries");

al13n321 · 2024-06-28T01:06:08Z

src/Storages/IStorage.cpp

                        getStorageID().getFullTableName(), data_path_in_backup);
 }

+StorageSnapshotPtr IStorage::getStorageSnapshot(const StorageMetadataPtr & metadata_snapshot, ContextPtr query_context) const


Can't these 3 methods just use default argument value instead of separate overloads?

virtual StorageSnapshotPtr getStorageSnapshot(const StorageMetadataPtr & metadata_snapshot, ContextPtr query_context, const StorageSnapshotSettings & additional_settings = {}) const;

actually it is because of clang-tidy warning: Default arguments on virtual or override methods are prohibited

al13n321 · 2024-06-28T01:18:56Z

src/Storages/MergeTree/MergeTreeSink.cpp

+            /// if block is added: push current_block to all subscribers under the same lock as commit.
+            /// But there may be a race between insert and creating a new subscription, in which case
+            /// the block may already have been cleared, but this is normal, because this is concurrent operations.


I would've liked to know how this translates to actual guarantees provided by streaming queries? Is this correct?:

Suggested change

/// if block is added: push current_block to all subscribers under the same lock as commit.

/// But there may be a race between insert and creating a new subscription, in which case

/// the block may already have been cleared, but this is normal, because this is concurrent operations.

/// If block is added: push current_block to all subscribers under the same lock as commit.

/// This ensures that streaming query receives each block at most once: either from the

/// initial read or from stream.

/// But there may be a race between insert and creating a new subscription, in which case

/// the block may already have been cleared, but this is normal: it's ok for streaming query

/// to miss a block that was inserted concurrently with starting the streaming query.

That is absolutely correct! The main idea that in case of concurrent insert (block P) and starting streaming query, it may not read the block P.

Michicosun · 2024-06-28T11:59:29Z

@al13n321 thank you for review!!

But I'm not recommend you to merge this pr, because:

I have complete implementation of streaming queries with cursors in different pr and it has no conflicts.
In this pr ReplicatedMergeTree is not supported and it cannot be, because streaming queries from replicated tables require cursors.

I see that issues are not critical, so maybe you can also check out final pr? 63312

I just merged this pr and queue mode into single and above that implemented cursors. Because commits are the same, I do not close previous prs, to make easier review.

Sorry about this move, but I needed to provide the final implementation of streaming queries read mechanics by the end of May (At least in the form of a pull request).

clickhouse-gh · 2024-07-30T13:13:19Z

Dear @al13n321, this PR hasn't been updated for a while. You will be unassigned. Will you continue working on it? If so, please feel free to reassign yourself.

Michicosun added 16 commits December 2, 2023 15:42

add stream keyword parsing

5a2e262

add subscription queue

f890d76

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

6eb5700

…/42990/simple_streaming

add streaming adapter to pipeline

205e0eb

change debug logs

3d12117

fix table_expression_modifiers eq

19e255d

change IStorage::write api

1368f7d

change description

3cb4040

add SubscriberNotifierProcessor

6d2dbcb

use notifier in write

d1df94e

add null-source reading for non-initialized streaming plans

d1f2c7f

add streaming feature flag

1e0c039

rename SubscriberNotifierProcessor -> SubscribersNotifierProcessor

e9a36cf

run format

f23b958

hpp -> h

e8e46df

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

263205a

…/42990/simple_streaming

alexey-milovidov added the can be tested Allows running workflows for external contributors label Dec 13, 2023

robot-ch-test-poll added the pr-feature Pull request with new product feature label Dec 13, 2023

Michicosun added 11 commits December 14, 2023 09:58

check context before deref

12cf465

fix style

156149b

implement waiting strategy for non-linux os

e282e98

fix unused private field

3c16134

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

39fcda5

…/42990/simple_streaming

fix subscription projections

a56d8db

change modifiers check order

3b6d387

add test

55b61b3

remove separator selects from test

a8c0dd7

update reference

1a94b60

add sleep before kill

1c653f8

Michicosun added 7 commits January 24, 2024 21:57

rename first test for common pattern

131e9e4

change streaming queries error-code numbers

bfcd674

add storage snapshot settings

abeb64d

fix style

038a4a5

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

d114f8f

…/42990/simple_streaming

push to subscribers under part-lock for merge-tree

745aa43

support distributed tables

3622f90

Michicosun force-pushed the issues/42990/simple_streaming branch from 0b00b24 to 3622f90 Compare January 29, 2024 20:22

Michicosun added 15 commits March 29, 2024 16:54

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

8105c6e

…/42990/simple_streaming

use new ParserKeyword api

f48ea9b

fix getStorageSnapshot inst

02a1729

fix MergeTree subscription push

0f987c1

add setting to SettingsChangesHistory.h

3cc1c7e

forbid prewhere

90b76ae

refactor checkHeaders

d382f7d

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

a378df3

…/42990/simple_streaming

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

2ede390

…/42990/simple_streaming

calculate projection via converting actions

f55889c

move async decision to work method

c06ee36

add correct finish handling

fdd3a2f

clear blocks in disable

7cbf072

fix style

a2bd7f4

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

210d0b7

…/42990/simple_streaming

Michicosun mentioned this pull request May 2, 2024

Streaming queries model with cursors #63312

Open

31 tasks

alexey-milovidov assigned al13n321 and unassigned al13n321 May 31, 2024

al13n321 approved these changes Jun 28, 2024

View reviewed changes

clickhouse-gh bot unassigned al13n321 Jul 30, 2024

	buffer << "final: " << has_final;
	buffer << ", final: " << has_final;

	"PREWHERE does not supported for Streaming Queries");
	"PREWHERE is not supported for Streaming Queries");

-            /// if block is added: push current_block to all subscribers under the same lock as commit.
-            /// But there may be a race between insert and creating a new subscription, in which case
-            /// the block may already have been cleared, but this is normal, because this is concurrent operations.
+            /// If block is added: push current_block to all subscribers under the same lock as commit.
+            /// This ensures that streaming query receives each block at most once: either from the
+            /// initial read or from stream.
+            /// But there may be a race between insert and creating a new subscription, in which case
+            /// the block may already have been cleared, but this is normal: it's ok for streaming query
+            /// to miss a block that was inserted concurrently with starting the streaming query.

Conversation

Michicosun commented Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

robot-ch-test-poll commented Dec 13, 2023 • edited by robot-ch-test-poll3 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

al13n321 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Michicosun commented Jun 28, 2024

Uh oh!

clickhouse-gh bot commented Jul 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Michicosun commented Dec 13, 2023 •

edited

Loading

robot-ch-test-poll commented Dec 13, 2023 •

edited by robot-ch-test-poll3

Loading