Skip to content
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.

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

[CH-431] not change columns to nulllable in expand operator #433

Open
wants to merge 765 commits into
base: clickhouse_backend
Choose a base branch
from

Conversation

shuai-xu
Copy link

@shuai-xu shuai-xu commented Apr 14, 2023

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in official stable or prestable release)

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

This pr fix the bug that columns type may be changed in expand operator, which will cause memory problem when serialize and deserialize them.
This close #431 .

Felixoid and others added 30 commits April 4, 2022 14:15
…ash_v2

One more try to resurrect build hash
Backport ClickHouse#35733 to 22.3: Added settings for insert of invalid IPv6, IPv4 values
Backport ClickHouse#35820 to 22.3: Avoid processing per-column TTL multiple times
- Allow define version as file
- Add inline cache
- Fix auto_release_type function
exmy and others added 24 commits March 21, 2023 10:35
…ence#354)

ShuffleSplitter improvement: support multiple subdirs
Support full join with join condition

Co-authored-by: shuai.li <[email protected]>
Support Decimal type in Gluten 
Co-authored-by: shuai.li <[email protected]>
…like Column 'deviceid' is not presented in input data (Kyligence#388)
@kyligence-git
Copy link
Collaborator

Can one of the admins verify this patch?

@shuai-xu shuai-xu changed the title not change columns to nulllable in expand operator [CH-431] not change columns to nulllable in expand operator Apr 14, 2023
Comment on lines 62 to 76
if (i < aggregating_expressions_columns_.size())
{
// do nothing with the aggregating columns.
cols.push_back(old_col);
continue;
}
if (old_col.type->isNullable())
cols.push_back(old_col);
else
{
auto null_map = DB::ColumnUInt8::create(0, 0);
auto null_col = DB::ColumnNullable::create(old_col.column, std::move(null_map));
auto null_type = std::make_shared<DB::DataTypeNullable>(old_col.type);
cols.push_back(DB::ColumnWithTypeAndName(null_col, null_type, old_col.name));
}
Copy link

@lgbo-ustc lgbo-ustc Apr 17, 2023

Choose a reason for hiding this comment

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

The reason for adding nullable here is that , for group by a,b roll up, the column a, b after this expand node could be null. we will have the following cases

a b explain
original a's values original b's values equal to group by a,b
original a's values NULL equal to group by a
NULL NULL equal to group by empty keys

@lwz9103 lwz9103 force-pushed the clickhouse_backend branch 2 times, most recently from dc60d55 to 8066113 Compare May 26, 2023 03:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrongly converting columns to nullable may cause momory problem