Skip to content

Partitioned write into s3 table function#23051

Merged
vdimir merged 43 commits intoClickHouse:masterfrom
excitoon-favorites:s3partitionedwrite
Aug 24, 2021
Merged

Partitioned write into s3 table function#23051
vdimir merged 43 commits intoClickHouse:masterfrom
excitoon-favorites:s3partitionedwrite

Conversation

@excitoon
Copy link
Contributor

@excitoon excitoon commented Apr 13, 2021

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

  • New Feature

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

Partitioned write into s3 table function

Detailed description / Documentation draft:

...

Shall resolve #15171

@robot-clickhouse robot-clickhouse added doc-alert pr-feature Pull request with new product feature labels Apr 13, 2021
@excitoon excitoon force-pushed the s3partitionedwrite branch from 3c6c6ae to 72e41be Compare May 25, 2021 06:45
@excitoon excitoon marked this pull request as ready for review May 31, 2021 08:46
@excitoon excitoon force-pushed the s3partitionedwrite branch 2 times, most recently from 8806330 to 7f1dfa4 Compare June 3, 2021 07:33
@excitoon
Copy link
Contributor Author

excitoon commented Jun 10, 2021

Failed tests are unrelated.

@excitoon excitoon force-pushed the s3partitionedwrite branch from 7f1dfa4 to e82baf7 Compare June 10, 2021 07:13
vdimir
vdimir reviewed Jun 12, 2021
View reviewed changes
Copy link
Member

@vdimir vdimir left a comment

Choose a reason for hiding this comment

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

Thank you for you changes!

I took a first look and left comments.

Also maybe I'll dig into details once again.

@vdimir vdimir self-assigned this Jun 12, 2021
@excitoon excitoon force-pushed the s3partitionedwrite branch from e82baf7 to ba0abd7 Compare July 14, 2021 07:50
vdimir
vdimir reviewed Jul 14, 2021
View reviewed changes
@excitoon excitoon force-pushed the s3partitionedwrite branch from f17b8dc to 0e6198e Compare July 14, 2021 13:04
vdimir
vdimir reviewed Jul 14, 2021
View reviewed changes
vdimir
vdimir reviewed Jul 14, 2021
View reviewed changes
Copy link
Member

@vdimir vdimir left a comment

Choose a reason for hiding this comment

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

Ok, generally it's good, except non-validated keys and unclear error message around it.

@vdimir
Copy link
Member

vdimir commented Jul 16, 2021

I've added partition key validation. We can change it later if it'll be too strict, I suppose it's better than adding restriction and broke some behavior in future.

@excitoon
Copy link
Contributor Author

excitoon commented Jul 19, 2021

For some reason GitHub does not show your commits separately https://github.com/ClickHouse/ClickHouse/pull/23051/files/82afef5..e91988b

excitoon
excitoon commented Jul 19, 2021
View reviewed changes
excitoon
excitoon commented Jul 19, 2021
View reviewed changes
excitoon
excitoon commented Jul 19, 2021
View reviewed changes
vdimir
vdimir reviewed Jul 19, 2021
View reviewed changes
src/Storages/StorageS3.cpp Outdated
Copy link
Member

@vdimir vdimir Jul 19, 2021

Choose a reason for hiding this comment

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

Also we have to check maximal length

@Enmk Enmk mentioned this pull request Jul 19, 2021
Closed
@excitoon excitoon force-pushed the s3partitionedwrite branch from 74662b0 to a7f683e Compare July 26, 2021 03:02
@excitoon
Copy link
Contributor Author

excitoon commented Jul 27, 2021

@vdimir Test is fixed but there is something wrong with RabbitMQ in integration tests.

@excitoon excitoon force-pushed the s3partitionedwrite branch from c16ffa9 to 04bb018 Compare July 28, 2021 10:55
@excitoon excitoon requested a review from vdimir July 28, 2021 10:55
@excitoon
Copy link
Contributor Author

excitoon commented Jul 28, 2021 •
edited
Loading

@vdimir Can you look through it again? I made some rework after breaking changes.

@vdimir
Copy link
Member

vdimir commented Jul 28, 2021

Test is fixed but there is something wrong with RabbitMQ in integration tests

RabbitMQ tests broken in master

Can you look through it again? I made some rework after breaking changes

Ok, I'm going to look at the changes

@excitoon excitoon force-pushed the s3partitionedwrite branch from a183136 to 6fe63a8 Compare August 23, 2021 06:22
@vdimir
Copy link
Member

vdimir commented Aug 24, 2021

Commit 98acccb is only merged master to resolve conflicts, no changes, build in Fast test passed, will merge. Full CI passed on commit 6fe63a8

@vdimir vdimir merged commit 5f4ca42 into ClickHouse:master Aug 24, 2021
@vdimir vdimir deleted the s3partitionedwrite branch August 24, 2021 07:58
@sevirov
Copy link
Contributor

sevirov commented Aug 24, 2021

Internal documentation ticket: DOCSUP-13742

@olgarev olgarev mentioned this pull request Sep 8, 2021
Merged
@alexey-milovidov alexey-milovidov mentioned this pull request Oct 16, 2021
Closed
@den-crane den-crane mentioned this pull request Nov 18, 2021
Closed
@alexey-milovidov
Copy link
Member

alexey-milovidov commented Feb 6, 2022

@excitoon @vdimir an issue has been found: #34348

@alexey-milovidov
Copy link
Member

alexey-milovidov commented Jun 18, 2022

@excitoon @vdimir a bug has been found: #38199

@alexey-milovidov alexey-milovidov mentioned this pull request Feb 26, 2025
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vdimir vdimir vdimir approved these changes

Assignees

@vdimir vdimir

Labels

pr-feature Pull request with new product feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Partitioned write into s3 table function / S3 table

6 participants

@excitoon @vdimir @sevirov @alexey-milovidov @filimonov @robot-clickhouse