Skip to content

Merging data type Map #15806#17829

Merged
CurtizJ merged 33 commits intoClickHouse:masterfrom
CurtizJ:merging-map
Dec 16, 2020
Merged

Merging data type Map #15806#17829
CurtizJ merged 33 commits intoClickHouse:masterfrom
CurtizJ:merging-map

Conversation

@CurtizJ
Copy link
Copy Markdown
Member

@CurtizJ CurtizJ commented Dec 5, 2020

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

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

The main difference from the original PR is that fixed duplicated sizes of arrays by using the type Nested.

@robot-clickhouse robot-clickhouse added the pr-not-for-changelog This PR should not be mentioned in the changelog label Dec 5, 2020
create table table_map (a Map(String, String), b String) engine = MergeTree() order by a;
insert into table_map values ({'name':'zhangsan', 'gender':'male'}, 'name'), ({'name':'lisi', 'gender':'female'}, 'gender');
select a[b] from table_map;
select b from table_map where a = map('name','lisi', 'gender', 'female');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we have a map constructor that accepts parallel arrays of keys/values a-la sumMap? And functions like keys/values/items that deconstruct it back into parallel arrays or an array of tuples. This constructor looks like it only works for a statically known set of keys, which might be not very practical.

Copy link
Copy Markdown
Member Author

@CurtizJ CurtizJ Dec 7, 2020

Choose a reason for hiding this comment

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

Map can be created from tuple of arrays by using CAST. Like this:

CAST(([1, 2, 3], ['1', '2', 'foo']), 'Map(UInt8, String)')

Maybe we need more convenient syntax to do that.

keys and values columns will be available after #17310 will be merged. items can be implemented also as a CAST.

Copy link
Copy Markdown
Contributor

@akuzm akuzm left a comment

Choose a reason for hiding this comment

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

Left some minor comments above, overall looks good now.

@akuzm akuzm self-assigned this Dec 15, 2020
@CurtizJ
Copy link
Copy Markdown
Member Author

CurtizJ commented Dec 16, 2020

Fuzzer: #14974.

@CurtizJ
Copy link
Copy Markdown
Member Author

CurtizJ commented Dec 16, 2020

Integration tests are broken in master.

@CurtizJ CurtizJ merged commit b1dc807 into ClickHouse:master Dec 16, 2020
This was referenced Dec 20, 2020
@alexey-milovidov
Copy link
Copy Markdown
Member

@CurtizJ One corner case has been found: #18492

else if (literal->value.getType() == Field::Types::Map)
{
const Map & map = literal->value.get<Map>();
if (map.size() % 2)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What does this condition mean? Why is map size odd if there are NULL values?

Copy link
Copy Markdown
Member Author

@CurtizJ CurtizJ Feb 16, 2021

Choose a reason for hiding this comment

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

But it's not about null values. I think it's just an excessive check, which is actually unneeded.

}
catch (...)
{
throw;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

WTF

@alexey-milovidov
Copy link
Copy Markdown
Member

@akuzm a bug has been found: deserialization code is not exception safe.

@alexey-milovidov
Copy link
Copy Markdown
Member

Fixed here: #36333

@alexey-milovidov
Copy link
Copy Markdown
Member

@CurtizJ I don't understand why ColumnMap exists, maybe remove it?

@alexey-milovidov
Copy link
Copy Markdown
Member

Does it exist only to enable Map type in Field?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants