LowCardinality column implementation #33

Enmk · 2020-03-23T16:18:31Z

Implemented LowCardinality(String) and LowCardinality(FixedString) with native binary serialization and a multitude of tests.

To enable efficient LowCardinality implementation, added some methods to the Column interface (and implementation in most derived classes), and added ItemView class (see the documentation in sources).

Also, to enable receiving LC columns from the server, updated client TCP protocol revision to 54405 (DBMS_MIN_REVISION_WITH_LOW_CARDINALITY_TYPE). That also forced me to update client/server protocol a bit:

read server display string and version patch
send client version patch
fix parsing of DateTime type with timezone

Known limitations:

LowCardniality of Nullable is not supported.

Minor refactoing of columns_ut.cpp: moved timers to utils.h Basic support of LowCardinality column, with draft serialization. LowCardinality implementation LowCardinality fixes: Fixed ColumnLowCardinality::Load to update unique items map Updated ColumnLowCardinality::Swap to validate nested type of other column. Minor fix: moved line around Tests for ColumnLowCardinality moved performance-tests to performance_tests.cpp Added performance test for ColumnLowCardinality(String) and ColumnLowCardinality(FixedString) Added type parser unit-tests for LowCardinality Added client tests for LowCardinality Bumped revision up to indicate that LowCardinality is supported DUMP of current work Implemented ItemView struct, using it as return type of Column::GetItem() Fixed various bugs in ColumnLowCardinality: mostly serialization and deserialization-wise More tests, for LowCardinaility in isolation, unit tests, performance tests, integration tests. Fixed, updated and cleaned up test cases

… has values Updated tests

Moved ItemView to separate header Renamed ColumnLowCardinalityWrapper and renamed it into ColumnLowCardinalityT Udpdated tests accordingly

Enmk · 2020-03-24T11:58:41Z

Comparison of populating, inserting and selecting 1'000'000 items performance-wise:

	UInt64	String	FixedString(8)	LowCardinality(String)	LowCardinality(FixedString(8))
Appending	10186	16580	9503	136121	129960
INSERT	1465	20953	1495	922	964
SELECT	434942	473523	448177	514830	553979
Dictionary size:				118	116

Please note:

same 8-byte strings were used for both FixedString(8) and LowCardinaility(FixedString(8))
same 7-byte strings were used for both String and LowCardinaility(String)
dictionary size reflects number of unique items + 1 extra NULL-item which is an implementation detail.

Since it is not present in older versions of XCode

Since there is no clickhouse server for MacOS started

traceon

First bunch of comments. Still in progress.

ut/utils.h

ut/CMakeLists.txt

ut/client_ut.cpp

clickhouse/client.cpp

clickhouse/types/type_parser.cpp

clickhouse/columns/array.cpp

clickhouse/columns/enum.cpp

clickhouse/columns/nullable.h

…ones.

* client_version_patch in ClientInfo * every overload of Column::Swap() now throws an exception on attempt to swap with instance of wrong type. * ColumnNullable::Swap() throws is nested column is of unexpected type. * PausableTimer is now started in paused mode. * few whitespace changes here and there

Enmk · 2020-03-24T20:13:13Z

Updated and fixed all mentioned issues + fixed inconsistent whitespace (at least in code added/updated in this PR)

traceon

Could you please also revisit and review the Timers code and make it solid and bulletproof and impossible to misuse, as it is arguably the most important code in this PR :) since it measures the performance of a feature whose sole existence is based on the performance benefits it gives.

clickhouse/columns/lowcardinality.cpp

clickhouse/columns/column.h

traceon · 2020-03-24T18:52:11Z

clickhouse/columns/lowcardinality.cpp

+    // however some day they may.
+
+    // prefix
+    uint64_t key_version;


Unless it is a conscious performance-motivated decision backed by data, vars are better to always be initialized. (Here and several other places.)

clickhouse/columns/lowcardinality.cpp

traceon · 2020-03-24T19:43:06Z

clickhouse/columns/lowcardinality.cpp

+        return;
+
+    dictionary_column_->Swap(*col->dictionary_column_);
+    index_column_.swap(col->index_column_);


Shouldn't this be index_column_->Swap(*col->index_column_); too?

It doesn’t matter here, but for consistency, let’s update it.

ut/performance_tests.cpp

traceon · 2020-03-24T20:23:39Z

ut/performance_tests.cpp

+
+            timer.Start();
+            column.Save(&ostr);
+            ostr.Flush();


Do you really want to measure flush too?

Yep, since lots of interesting things can happen there, also I am measuring the whole process, the way it is in real cases.

OK, however potentially LowCardinality + flush, and regular type + flush numbers may differ insignificantly, if flush is heavy. While isolated LowCardinality may be like several times slower than a regular type, and that difference just dwarfed by flush overhead.

It is quite the opposite: serialization/deserialization of LowCardinality columns is MUCH faster (up to 100 times faster than bare String and up to 3 times faster than bare FixedSring columns)

Yes, sorry, my bad. But the idea remains the same, the observer wont be able to properly see that degree of difference.

ut/performance_tests.cpp

ut/utils.h

* Using simpler timer * More clean measurments of SELECT * minor whitespace/braces fixes

…m ColumnLowCardinality + Other minor fixes: mostly whitespace.

Enmk · 2020-03-25T09:38:54Z

After the most recent update to implementation and measurements:

	UInt64	String	FixedString(8)	LowCardinality(String)	LowCardinality(FixedString(8))
Append	10452	17879	10984	60052	61923
INSERT	1418	22340	1409	732	736
SELECT	4432	25419	2090	459	545

Tested against localhost server, so the IO lag is diminishable. Tested on 1M entries, time is in microseconds.

traceon · 2020-03-25T11:01:06Z

LGTM

Enmk added 11 commits March 23, 2020 17:59

Receiving display name and patch version from server

12d1b80

Fixed parsing DateTime type with timezone

a4fab80

Fixed ColumnLowCardinality constructor when dictionary column already…

dc523de

… has values Updated tests

Fixed: sending client patch version to server

4054499

Refactoring

62c39a6

Moved ItemView to separate header Renamed ColumnLowCardinalityWrapper and renamed it into ColumnLowCardinalityT Udpdated tests accordingly

Minor: cleanup

33d80ac

Removed Client::AppendFrom

413c8f7

Fixed compilation error

cdf2321

Fixed compilation errors

8110cd0

Bumped xcode version for Travis from 8.2 (end of 2016) to 9.4 (May 2018)

907bedb

Enmk requested a review from traceon March 23, 2020 20:22

Updated ItemView and ColumnLowCardinality to not use std::variant

3c51f41

Since it is not present in older versions of XCode

Enmk force-pushed the low-cardinality branch from 33b3f04 to 3c51f41 Compare March 24, 2020 12:00

Excluded performance tests from MacOS TravisCI builds

82c2912

Since there is no clickhouse server for MacOS started

traceon suggested changes Mar 24, 2020

View reviewed changes

Enmk added 2 commits March 24, 2020 22:25

Post-review fixes: changed parsing of quoted strings + lots of minor …

fd92e4c

…ones.

traceon suggested changes Mar 24, 2020

View reviewed changes

Enmk added 4 commits March 25, 2020 11:21

Post-PR fixes: Timer and formatting

85f99f2

* Using simpler timer * More clean measurments of SELECT * minor whitespace/braces fixes

Improved appending (faster and safer) and improved reading values fro…

b5d5f44

…m ColumnLowCardinality + Other minor fixes: mostly whitespace.

Comments and inlines

4e8e477

Minor: variable rename

33648df

Enmk added 2 commits March 25, 2020 12:57

Checking that total rows is OK

e712056

Minor: added comment and remove duplicate source from CMakeLists.txt

3682e35

traceon merged commit ad5fd98 into ClickHouse:master Mar 25, 2020

LowCardinality column implementation #33

LowCardinality column implementation #33

Uh oh!

Conversation

Enmk commented Mar 23, 2020

Uh oh!

Enmk commented Mar 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

traceon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Enmk commented Mar 24, 2020

Uh oh!

traceon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

traceon Mar 24, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

traceon Mar 24, 2020

Choose a reason for hiding this comment

Uh oh!

Enmk Mar 24, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

traceon Mar 24, 2020

Choose a reason for hiding this comment

Uh oh!

Enmk Mar 24, 2020

Choose a reason for hiding this comment

Uh oh!

traceon Mar 24, 2020

Choose a reason for hiding this comment

Uh oh!

Enmk Mar 25, 2020

Choose a reason for hiding this comment

Uh oh!

traceon Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Enmk commented Mar 25, 2020

Uh oh!

traceon commented Mar 25, 2020

Uh oh!

Uh oh!

Enmk commented Mar 24, 2020 •

edited

Loading

traceon left a comment •

edited

Loading

traceon Mar 25, 2020 •

edited

Loading