Skip to content

Fix pcg deserialization#24538

Merged
tavplubix merged 7 commits intomasterfrom
fix_pcg_deserialization
Jun 24, 2021
Merged

Fix pcg deserialization#24538
tavplubix merged 7 commits intomasterfrom
fix_pcg_deserialization

Conversation

@tavplubix
Copy link
Copy Markdown
Member

@tavplubix tavplubix commented May 26, 2021 •
edited by alexey-milovidov
Loading

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

Changelog category (leave one):

  • Bug Fix

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fixed bug in deserialization of random generator state with might cause some data types such as AggregateFunction(groupArraySample(N), T)) to behave in a non-deterministic way

@robot-clickhouse robot-clickhouse added the pr-bugfix Pull request with bugfix, not backported by default label May 26, 2021
tavplubix
tavplubix commented May 26, 2021
View reviewed changes
tests/queries/0_stateless/01156_pcg_deserialization.sh
Comment on lines +16 to +17
$CLICKHOUSE_CLIENT -q "select countDistinct(n), countDistinct(a1) from t"

Copy link
Copy Markdown
Member Author

@tavplubix tavplubix May 26, 2021

Choose a reason for hiding this comment

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

It used to produce different results with Memory and MergeTree/Log, I have no ideas why:
https://gist.github.com/tavplubix/8fafec4bca6a1b78087afd99aea85ff9

Copy link
Copy Markdown
Member

@alexey-milovidov alexey-milovidov Jun 12, 2021

Choose a reason for hiding this comment

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

You did not specify seed, so random seed is selected.

@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented May 26, 2021

@tavplubix See also #20480

@nikitamikhaylov nikitamikhaylov self-assigned this May 31, 2021
@alesapin
Copy link
Copy Markdown
Member

alesapin commented Jun 7, 2021

@Mergifyio update

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Jun 7, 2021

Command update: success

Branch has been successfully updated

@tavplubix
Copy link
Copy Markdown
Member Author

tavplubix commented Jun 7, 2021

Fast test - 01156_pcg_deserialization - Illegal type AggregateFunction(groupArraySample(1), UInt8) of argument for aggregate function uniqExact
But it worked before merging with master

@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Jun 8, 2021

@tavplubix It is broken here: #24523

@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Jun 12, 2021

@Mergifyio update

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Jun 12, 2021

Command update: success

Branch has been successfully updated

@alexey-milovidov alexey-milovidov self-assigned this Jun 12, 2021
@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Jun 12, 2021

It should not affect #20480, because quantileDeterministic does not use rng.

@tavplubix
Copy link
Copy Markdown
Member Author

tavplubix commented Jun 23, 2021

Now the test fails due to another bug:

DB::Exception: Conversion from AggregateFunction(groupArraySample, UInt8) to AggregateFunction(groupArraySample(1), UInt8) is not supported

$ ./clickhouse local -q "select version(), toTypeName(groupArraySampleState(1)(toUInt8(number))) from numbers(10)"
21.7.1.7111	AggregateFunction(groupArraySample(1), UInt8)

$ ./clickhouse local -q "select version(), toTypeName(groupArraySampleState(1)(toUInt8(number))) from numbers(10)"
21.7.1.7128	AggregateFunction(groupArraySample, UInt8)

It should not ignore parameters of nested aggregate function. I will try to fix it.

@tavplubix tavplubix mentioned this pull request Jun 23, 2021
Closed
@tavplubix tavplubix merged commit b1263c1 into master Jun 24, 2021
@tavplubix tavplubix deleted the fix_pcg_deserialization branch June 24, 2021 07:40
This was referenced Jun 24, 2021
Merged
Closed
Merged
Merged
robot-clickhouse pushed a commit that referenced this pull request Jun 24, 2021
Backport #24538 to 21.6: Fix pcg deserialization
6d6b879
@robot-clickhouse robot-clickhouse mentioned this pull request Jun 24, 2021
Merged
@robot-clickhouse robot-clickhouse mentioned this pull request Jun 24, 2021
Merged
robot-clickhouse pushed a commit that referenced this pull request Jun 24, 2021
Backport #24538 to 21.3: Fix pcg deserialization
24f9585
robot-clickhouse pushed a commit that referenced this pull request Jun 24, 2021
Backport #24538 to 21.5: Fix pcg deserialization
f05e76a
@robot-clickhouse robot-clickhouse mentioned this pull request Jun 24, 2021
Merged
tavplubix added a commit that referenced this pull request Jun 25, 2021
@tavplubix
Merge pull request #25677 from ClickHouse/backport/21.6/24538
7fc6342 
Backport #24538 to 21.6: Fix pcg deserialization
tavplubix added a commit that referenced this pull request Jun 25, 2021
@tavplubix
Merge pull request #25678 from ClickHouse/backport/21.3/24538
8e98560 
Backport #24538 to 21.3: Fix pcg deserialization
tavplubix added a commit that referenced this pull request Jun 25, 2021
@tavplubix
Merge pull request #25679 from ClickHouse/backport/21.5/24538
7d6983c 
Backport #24538 to 21.5: Fix pcg deserialization
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexey-milovidov alexey-milovidov alexey-milovidov approved these changes

Assignees

@alexey-milovidov alexey-milovidov

Labels

pr-bugfix Pull request with bugfix, not backported by default

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@tavplubix @alexey-milovidov @alesapin @nikitamikhaylov @robot-clickhouse