Skip to content

Small refactoring in formats #31004

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
Avogar merged 4 commits into from
Nov 17, 2021
Merged

Small refactoring in formats #31004

Avogar merged 4 commits into from
Nov 17, 2021

Conversation

@Avogar
Copy link
Member

@Avogar Avogar commented Nov 2, 2021 •
edited
Loading

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

Try to make code in formats a bit better

@robot-clickhouse robot-clickhouse added the pr-not-for-changelog This PR should not be mentioned in the changelog label Nov 2, 2021
@Avogar
Copy link
Member Author

Avogar commented Nov 2, 2021

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Nov 2, 2021

update

✅ Branch has been successfully updated

@nikitamikhaylov nikitamikhaylov self-assigned this Nov 2, 2021
@Avogar
Copy link
Member Author

Avogar commented Nov 2, 2021

Fast test failure: #31011

Avogar
Avogar commented Nov 3, 2021
View reviewed changes
@Avogar Avogar mentioned this pull request Nov 3, 2021
Closed
@Avogar
Copy link
Member Author

Avogar commented Nov 8, 2021

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Nov 8, 2021

update

✅ Branch has been successfully updated

@Avogar
Copy link
Member Author

Avogar commented Nov 12, 2021

01059_storage_file_compression fail - found bug in master. WIll be fixed in #31265

@Avogar Avogar merged commit f27fcf8 into ClickHouse:master Nov 17, 2021
azat
azat reviewed Nov 20, 2021
View reviewed changes
tests/queries/0_stateless/01059_storage_file_compression.sh
${CLICKHOUSE_CLIENT} --query "CREATE TABLE file (x UInt64) ENGINE = File(TSV, '${CLICKHOUSE_DATABASE}/${m}.tsv.${m}')"
${CLICKHOUSE_CLIENT} --query "TRUNCATE TABLE file"
${CLICKHOUSE_CLIENT} --query "INSERT INTO file SELECT * FROM numbers(1000000)"
sleep 1
Copy link
Member

@azat azat Nov 20, 2021

Choose a reason for hiding this comment

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

@Avogar why this sleep is required?

Copy link
Member

@azat azat Nov 20, 2021

Choose a reason for hiding this comment

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

Actually seems that this PR likely breaks performance tests - https://clickhouse-test-reports.s3.yandex.net/0/fc9ef14a736d62f121d27d3a23490ef7198190be/performance_comparison/report.html#fail1

Copy link
Member

@azat azat Nov 20, 2021

Choose a reason for hiding this comment

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

Actually seems that this PR likely breaks performance tests - https://clickhouse-test-reports.s3.yandex.net/0/fc9ef14a736d62f121d27d3a23490ef7198190be/performance_comparison/report.html#fail1

#31565

Copy link
Member Author

@Avogar Avogar Nov 20, 2021

Choose a reason for hiding this comment

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

@Avogar why this sleep is required?

It was just a temporary solution to fix failing test, because I needed these changes in the other PRs. The real fix is in #31265

Copy link
Member Author

@Avogar Avogar Nov 20, 2021

Choose a reason for hiding this comment

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

Actually seems that this PR likely breaks performance tests - https://clickhouse-test-?>>reports.s3.yandex.net/0/fc9ef14a736d62f121d27d3a23490ef7198190be/performance_comparison/report.html#fail1

#31565

Thanks

@azat azat mentioned this pull request Nov 20, 2021
Merged
azat added a commit to azat/ClickHouse that referenced this pull request Nov 20, 2021
@azat
Fix MySQLWire format (in case of multiple writes)
6aa94ae 
In case of multiple writes File() engine will set doNotWritePrefix(),
and this will avoid serializations initilization, move this to do this
always.

Fixes: ClickHouse#31004
@azat azat mentioned this pull request Nov 20, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@azat azat azat left review comments

@KochetovNicolai KochetovNicolai KochetovNicolai approved these changes

@nikitamikhaylov nikitamikhaylov nikitamikhaylov approved these changes

Assignees

@nikitamikhaylov nikitamikhaylov

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Avogar @azat @KochetovNicolai @nikitamikhaylov @robot-clickhouse