add compression method for files: Xz by fibersel · Pull Request #16578 · ClickHouse/ClickHouse

fibersel · 2020-11-01T00:10:34Z

Changelog category (leave one):

New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
add *.xz compression/decompression support.It enables using *.xz in file() function.This closes #8828

…tmodules, *added cmake for xz

contrib/xz-cmake/CMakeLists.txt

azat · 2020-11-01T06:18:46Z

Where can I find tests for ZlibInflatingReadBuffer.h?I grepped tests/ folder and found nothing

src/IO/tests/zlib_buffers.cpp:#include <IO/ZlibInflatingReadBuffer.h>
src/IO/tests/zlib_buffers.cpp:        DB::ZlibInflatingReadBuffer inflating_buf(std::move(buf), DB::CompressionMethod::Gzip);

…sic code for encoding

…essor, tests passing

azat

Just a few minor comments until there is no can-be-tested label

azat · 2020-11-02T07:21:20Z

src/IO/LzmaWriteBuffer.cpp

-				return;
+		//std::cout << ret << " RET FIN" << std::endl;
+
+        if (ret == LZMA_STREAM_END) {


Looks like indent is wrong

azat · 2020-11-02T07:21:37Z

src/IO/LzmaWriteBuffer.cpp

            throw Exception(std::string("lzma stream encoding failed: ") + "; lzma version: " + LZMA_VERSION_STRING, ErrorCodes::LZMA_STREAM_ENCODER_FAILED);

-        std::cout << lstr.avail_in << std::endl;
+        //std::cout << lstr.avail_in << " " << lstr.avail_out << std::endl;


Forgot to remove? (and in a few other places)

azat · 2020-11-02T07:22:41Z

src/IO/LzmaWriteBuffer.cpp

+
+		//std::cout << ret << " RET IMPL" << std::endl;
+
+        if (ret == LZMA_STREAM_END) {


Coding style: curly bracket is always on a new line (and not only here but in a few other places) (except for } while (true))

azat · 2020-11-02T07:26:42Z

src/IO/LzmaReadBuffer.cpp

+            throw Exception(
+                std::string("lzma decoder finished, but stream is still alive: error code: ") + std::to_string(ret) + "; lzma version: " + LZMA_VERSION_STRING,
+                ErrorCodes::LZMA_STREAM_DECODER_FAILED);


This can use fmt-like constructor

Suggested change

throw Exception(

std::string("lzma decoder finished, but stream is still alive: error code: ") + std::to_string(ret) + "; lzma version: " + LZMA_VERSION_STRING,

ErrorCodes::LZMA_STREAM_DECODER_FAILED);

throw Exception(ErrorCodes::LZMA_STREAM_DECODER_FAILED, "lzma decoder finished, but stream is still alive: error code: {}; lzma version: {}", ret, LZMA_VERSION_STRING);

azat · 2020-11-02T19:11:14Z

src/IO/LzmaWriteBuffer.cpp

-		//std::cout << ret << " RET FIN" << std::endl;

-        if (ret == LZMA_STREAM_END) {
+        if (ret == LZMA_STREAM_END) 


trailing whitespace, and incorrect indent below (tabs instead of spaces, and tabs also not only here)

And by the way it is not only here, it is pretty easy to fix just using git, but please note that this will override the history (and that said that if you don't know git I guess it will be better if you will fix this in your editor)

git rebase --whitespace=fix upstream/master

Can I just format it with clang-format and options from .clang-format?

I guess (I've never used it)

azat · 2020-11-02T19:17:05Z

contrib/CMakeLists.txt

 add_subdirectory (replxx-cmake)
 add_subdirectory (ryu-cmake)
 add_subdirectory (unixodbc-cmake)
+add_subdirectory (xz)


Looks like you've forget to add submodule

Isn't it?
https://github.com/ClickHouse/ClickHouse/pull/16578/files#diff-fe7afb5c9c916e521401d3fcfb4277d5071798c3baf83baf11d6071742823584R193

Not really, this is just to know where to fetch the submodule from.

You need to run smth like this:

git submodule add https://github.com/xz-mirror/xz contrib/xz

And this will create a special file contrib/xz in the git index (but in the filesystem you will see it as a content of the repository), that will contain the HEAD for the submodule

Also to make fasttest update your submodule you need to add contrib/xz into

ClickHouse/docker/test/fasttest/run.sh

Line 130 in f10a520

SUBMODULES_TO_UPDATE=(contrib/boost contrib/zlib-ng contrib/libxml2 contrib/poco contrib/libunwind contrib/ryu contrib/fmtlib contrib/base64 contrib/cctz contrib/libcpuid contrib/double-conversion contrib/libcxx contrib/libcxxabi contrib/libc-headers contrib/lz4 contrib/zstd contrib/fastops contrib/rapidjson contrib/re2 contrib/sparsehash-c11 contrib/croaring)

Without fasttest other tests won't be run

Also it's possible to disable xz in fasttest.

fibersel · 2020-11-02T20:06:58Z

I kind of fixed it with clang-format.I manually look for tabs with vim and don't find it.Looks like all whitespaces consist of single spaces now.

azat · 2020-11-02T20:47:41Z

src/CMakeLists.txt

+set (LZMA_LIBRARY liblzma)
+set (LZMA_INCLUDE_DIR ${ClickHouse_SOURCE_DIR}/contrib/xz/src/liblzma/api)


And I guess it is better to move this out into separate contrib/xz-cmake/CMakeLists.txt, and add an interface library.
One of similar files is contrib/protobuf-cmake/CMakeLists.txt (protobuf has it's own cmake rules, while in clickhouse there is just a wrapper that install some options)

What should I do to pass description check?
I've added record to CHANCHELOG.md

What should I do to pass description check?

Update description of PR and include changelog entry using specified format.
There is template for PRs - https://raw.githubusercontent.com/ClickHouse/ClickHouse/master/.github/PULL_REQUEST_TEMPLATE.md

Just copy it and modify the template, but leave the format.

Yes, it's much better to provide our own CMakeLists.

CMake often cannot be safely reused (without building unneeded targets, polluting build options).

fibersel · 2020-11-02T22:16:30Z

Ok, it seems, that I should write a wrapper for CMake

fibersel · 2020-11-02T23:02:25Z

It seems like tuklib_cpucores calls additional function that sets TUKLIB_CPUCORES_DEFINITIONS to TUKLIB_CPUCORES_SCHED_GETAFFINITY, but after that check still fails.
https://github.com/xz-mirror/xz/blob/master/cmake/tuklib_cpucores.cmake#L39

azat · 2020-11-03T08:33:16Z

It seems like tuklib_cpucores calls additional function that sets TUKLIB_CPUCORES_DEFINITIONS to TUKLIB_CPUCORES_SCHED_GETAFFINITY, but after that check still fails.

The reason is that if (DEFINED CACHED{VAR}) is added only in 3.14

You can try upgrade fasttest image to 20.04 (there are few images that already uses it anyway, so I guess it is fine, @alesapin ?) and it will contain cmake 3.16

ClickHouse/docker/test/fasttest/Dockerfile

Line 2 in 268c805

FROM ubuntu:19.10

P.S. you can also send a patch to the uptream to bump cmake requirements:
https://github.com/xz-mirror/xz/blob/869b9d1b4edd6df07f819d360d306251f8147353/CMakeLists.txt#L50

alexey-milovidov · 2020-11-03T11:39:22Z

We usually write our own CMakeLists for libraries. Because original CMakeLists are intended to build a library itself, not to build it for another project.

fibersel · 2020-11-03T22:52:24Z

2020-11-04 01:46:00 -- Using CMake version 3.13.4
Is seems like after updating base image to version 20.04 cmake is not being updated to 3.16

azat · 2020-11-04T04:16:19Z

Is seems like after updating base image to version 20.04 cmake is not being updated to 3.16

Yeah, this is because fasttest image is not updated on per-PR bases (as most of the images)

alexey-milovidov · 2020-11-07T19:30:57Z

src/IO/LzmaWriteBuffer.cpp

+            std::string("lzma preset failed: ") + "; lzma version: " + LZMA_VERSION_STRING, ErrorCodes::LZMA_STREAM_ENCODER_FAILED);
+
+
+    lzma_filter filters[] = {


Missing comments (explanations), the code is unclear to me.

alexey-milovidov

We also need a functional test.

grep 'Content-Encoding'
in test/queries/0_stateless

We need to make sure that xz tool can read our xz-compressed data
and we can read data compressed with the xz tool.

alexey-milovidov · 2020-11-07T19:33:48Z

src/IO/tests/lzma_buffers.cpp

+#include <IO/WriteHelpers.h>
+#include <IO/ReadHelpers.h>
+
+int main(int, char **)


Did you run it? (And what are the results in comparison with xz?)

Yes, I ran. But I suspect, that my implementation is not completely optimal:

(base) [centos@clickhouse build]$ ./src/IO/tests/lzma_buffers
Writing done. Elapsed: 12.78 s., 6.17 MB/s
Reading done. Elapsed: 0.79 s., 100.24 MB/s

(base) [centos@clickhouse build]$ ./src/IO/tests/zlib_buffers
Writing done. Elapsed: 8.04 s., 110.55 MB/s
Reading done. Elapsed: 4.44 s., 200.08 MB/s

It's hard to say... Better to compare with xz tool, and we expect that performance should not be significantly lower.

alexey-milovidov

Ok, only some small changes required.

alexey-milovidov · 2020-11-07T19:35:55Z

Maybe we can also add a test for file table function?

alexey-milovidov · 2020-11-07T19:37:18Z

You can find similar tests in the following way:

look at the git blame for the code related to CompressionMethod;
copy sha of the commit;
search the commit on GitHub;
open "issues" - there are also pull requests;
now you have found pull requests where the feature was added;
there should be the related tests.

…o issue-8828

…r file() function

…unction, formatted code

fibersel · 2020-11-11T01:53:45Z

HTTPHandlers.cpp has too many changes because I did clang-format -i src/Server/HTTPHandler.cpp before commiting.

…o issue-8828

alexey-milovidov · 2020-11-11T22:10:35Z

src/IO/CompressionMethod.cpp

        return CompressionMethod::Zlib;
    if (*method_str == "brotli" || *method_str == "br")
        return CompressionMethod::Brotli;
+    if (*method_str == "LZMA" || *method_str == "xz")


A small note... xz is actually LZMA2. But why do we need this name? Maybe just leave xz only.

Is there the only place to remove LZMA?
Is it ok to leave all buffer filenames with LZMA* prefix?

alexey-milovidov

LGTM.

There are some leftover comments that are related to another xz implementation... we can try it in the next PR.

fibersel · 2020-11-11T22:23:48Z

LGTM.

There are some leftover comments that are related to another xz implementation... we can try it in the next PR.

I had sent mail to contributor of fast-lzma2 maintainer, but he has not responded yet.I am not sure that fast-lzma2 is compatible with classic *.xz format and it's repository does not contain CMake file, so we need to adapt their implementation.I have faced some problems with that.Can we merge this implementation and keep in mind existence of another implementation?

alexey-milovidov · 2020-11-11T23:09:12Z

Yes, we can merge before trying fast-lzma2.

alexey-milovidov · 2020-11-11T23:09:37Z

You can remove commented code before we'll merge.

alexey-milovidov · 2020-11-11T23:57:59Z

I've tested decompression performance, it's the same as for xz tool.

alexey-milovidov · 2020-11-11T23:59:53Z

Maybe you will be also interested in the internals of pixz tool.

fibersel · 2020-11-12T00:02:13Z

Maybe you will be also interested in the internals of pixz tool.

I will keep it in mind, but now I am going to deal with main task.

akuzm · 2020-11-12T06:59:39Z

This PR broke the Fast Test.

#16908

I'll merge the fix for now, so that we have at least some tests, but it's not entirely correct. CMake should be able to finish when the xz submodule is not cloned -- it's not an essential one.

azat · 2020-11-22T19:55:21Z

.gitmodules

 	path = contrib/miniselect
 	url = https://github.com/danlark1/miniselect
+[submodule "contrib/xz"]
+       path = contrib/xz


Uses spaces for indent, while all other lines uses tabs (since git submodule add uses them)

a.palagashvili added 3 commits November 1, 2020 02:56

*added adapters' boilerplate for Lzma buffers, *added submodule to gi…

c1abf5d

…tmodules, *added cmake for xz

removed extra record from cmake

5982f9f

changed moment of stream object initialization

731e274

azat reviewed Nov 1, 2020

View reviewed changes

contrib/xz-cmake/CMakeLists.txt Outdated Show resolved Hide resolved

contrib/xz-cmake/CMakeLists.txt Outdated Show resolved Hide resolved

contrib/xz-cmake/CMakeLists.txt Outdated Show resolved Hide resolved

a.palagashvili added 2 commits November 1, 2020 21:40

fixed cmake for building, added test for compression method, added ba…

be2b002

…sic code for encoding

fixed compressor testing, added base logic for compressor and decompr…

495cd47

…essor, tests passing

azat reviewed Nov 2, 2020

View reviewed changes

alexey-milovidov added the can be tested label Nov 2, 2020

robot-clickhouse added the submodule changed At least one submodule changed in this PR. label Nov 2, 2020

a.palagashvili added 2 commits November 2, 2020 15:16

resolved git modules

805bfd2

fixed codestyle, added record to changelog

2ad01c5

azat reviewed Nov 2, 2020

View reviewed changes

fixed whitespaces, added hidden submodule file

ba6fa5d

azat reviewed Nov 2, 2020

View reviewed changes

robot-clickhouse added doc-alert pr-feature Pull request with new product feature labels Nov 2, 2020

added record for fasttest

8098f86

attempt to update fasttest version

8ecf1d0

a.palagashvili added 2 commits November 4, 2020 02:47

added newline for files

f9cebbf

replaced null with nullptr

986d13d

akuzm added the force tests label Nov 4, 2020

added eof initializing in constuctor

53a064b

alexey-milovidov reviewed Nov 7, 2020

View reviewed changes

alexey-milovidov requested changes Nov 7, 2020

View reviewed changes

alexey-milovidov self-assigned this Nov 7, 2020

alexey-milovidov mentioned this pull request Nov 8, 2020

Integrate and test experimental compression libraries. #16775

Closed

a.palagashvili added 3 commits November 9, 2020 19:53

Merge branch 'master' of https://github.com/ClickHouse/ClickHouse int…

6286775

…o issue-8828

renamed files, added new library, changed error codes, added tests fo…

f999ea2

…r file() function

added and successfully passed tests for content encoding and file() f…

124ef2f

…unction, formatted code

a.palagashvili added 3 commits November 11, 2020 14:10

Merge branch 'master' of https://github.com/ClickHouse/ClickHouse int…

9479052

…o issue-8828

fixed style, xz check fasttest skipped, removed fast-lzma2

55d05c9

regenerated ya.make

1b06fd9

fibersel requested a review from alexey-milovidov November 11, 2020 16:21

alexey-milovidov reviewed Nov 11, 2020

View reviewed changes

alexey-milovidov approved these changes Nov 11, 2020

View reviewed changes

remove commented code

fe5800a

alexey-milovidov merged commit 34b2a46 into ClickHouse:master Nov 11, 2020

azat reviewed Nov 22, 2020

View reviewed changes

lehasm mentioned this pull request Feb 18, 2021

DOCSUP-4328: documented xz compression method #20540

Merged

fibersel mentioned this pull request Mar 11, 2021

Topic Operator: Changing replication factor strimzi/strimzi-kafka-operator#191

Closed


		//std::cout << ret << " RET IMPL" << std::endl;

		if (ret == LZMA_STREAM_END) {

		set (LZMA_LIBRARY liblzma)
		set (LZMA_INCLUDE_DIR ${ClickHouse_SOURCE_DIR}/contrib/xz/src/liblzma/api)

		std::string("lzma preset failed: ") + "; lzma version: " + LZMA_VERSION_STRING, ErrorCodes::LZMA_STREAM_ENCODER_FAILED);


		lzma_filter filters[] = {

Conversation

fibersel commented Nov 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

azat commented Nov 1, 2020

Uh oh!

azat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azat Nov 2, 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!

Choose a reason for hiding this comment

Uh oh!

fibersel commented Nov 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fibersel commented Nov 2, 2020

Uh oh!

fibersel commented Nov 2, 2020

Uh oh!

azat commented Nov 3, 2020

Uh oh!

alexey-milovidov commented Nov 3, 2020

Uh oh!

fibersel commented Nov 3, 2020

Uh oh!

azat commented Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexey-milovidov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fibersel Nov 7, 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!

alexey-milovidov Nov 7, 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!

alexey-milovidov left a comment

Choose a reason for hiding this comment

Uh oh!

alexey-milovidov commented Nov 7, 2020

fibersel commented Nov 1, 2020 •

edited

Loading

azat Nov 2, 2020 •

edited

Loading

azat commented Nov 4, 2020 •

edited

Loading

fibersel Nov 7, 2020 •

edited

Loading

alexey-milovidov Nov 7, 2020 •

edited

Loading

alexey-milovidov commented Nov 7, 2020 •

edited

Loading

fibersel commented Nov 11, 2020 •

edited

Loading