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

Fix Array of low cardinality types serialization/deserialization #173

Merged
merged 10 commits into from
May 3, 2022

Conversation

arthurpassos
Copy link
Contributor

Context

Array of LowCardinality serialization/deserialization was broken as reported in #140. Issue was that clickhouse-cpp was serializing/deserializing the array offsets before the lowcardinality key_version and Clickhouse was expecting it to come after the key_version.

Solution was to implement a prefix, body and suffix serialization/deserialization approach similar to what can be found in Clickhouse/NativeWriter.

Changes

  1. Add SavePrefix, SaveBody and SaveSuffix methods to Column base class.
  2. Add and implement Save template method in Column base class. Column::Save calls SavePrefix, SaveBody and SaveSuffix respectively.
  3. Add unit and integration tests for array of low cardinality. See ut/array_of_low_cardinality_tests.cpp

Testing

  1. Ran existing tests and added new ones.

Closes

@CLAassistant
Copy link

CLAassistant commented Apr 26, 2022

CLA assistant check
All committers have signed the CLA.

@arthurpassos arthurpassos changed the title Array of lc serialization fix Fix Array of low cardinality types serialization/deserialization Apr 26, 2022
Copy link
Collaborator

@Enmk Enmk left a comment

Choose a reason for hiding this comment

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

Good, but not yet ready to merge

clickhouse/columns/array.h Show resolved Hide resolved
clickhouse/columns/column.cpp Outdated Show resolved Hide resolved
clickhouse/columns/column.cpp Show resolved Hide resolved
clickhouse/columns/column.cpp Show resolved Hide resolved
clickhouse/columns/column.h Outdated Show resolved Hide resolved
clickhouse/columns/column.h Outdated Show resolved Hide resolved
ut/array_of_low_cardinality_tests.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@Enmk Enmk left a comment

Choose a reason for hiding this comment

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

LGTM

@Enmk Enmk merged commit 323adbe into ClickHouse:master May 3, 2022
@huyphams
Copy link
Contributor

huyphams commented May 6, 2022

My app is still crashing after this merge, let me see which is the issue.

@huyphams
Copy link
Contributor

huyphams commented May 6, 2022

I found the issue, it will not work if the array of Array(LowCardinality(String)) is empty. For example (the last value is an empty array):

const auto testData = std::vector<std::vectorstd::string> {
{ "aa", "bb" },
{ "cc" },
{}
};

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.

4 participants