ARROW-12110: [Java] Implement ZSTD compression by emkornfield · Pull Request #9822 · apache/arrow

emkornfield · 2021-03-27T04:17:07Z

Add ZSTD codec implementation.
Removes dependency on netty by using ArrowBuf methods.
Updates docs and archery test (will wait on CI to run archery if that fails will do more debugging.)

github-actions · 2021-03-27T04:17:24Z

https://issues.apache.org/jira/browse/ARROW-12110

nevi-me · 2021-03-27T04:21:25Z

dev/archery/archery/integration/runner.py

                skip.add("Go")
                skip.add("JS")
                skip.add("Rust")
-            if name == 'zstd':


A downside to removing this comment completely is that someone might not easily figure out how to skip tests based on name (when working on another implementation).

Do you mind adding a comment to the effect that if an implementer is skipping one of the compression formats, they could use if name == 'format'?

Added a link back to this PR>

Add link back to this PR for disabling specific codecs.

check decompressed length.

emkornfield · 2021-03-27T17:44:18Z

I ran archery tests for java locally and zstd seems to pass with this PR. I'm not sure how these changes could have affected the archery/crossbow tests.

@dianaclarke I think you might have been the last person to touch this area? Any thoughts?

dianaclarke · 2021-03-27T18:35:33Z

I ran archery tests for java locally and zstd seems to pass with this PR. I'm not sure how these changes could have affected the archery/crossbow tests.

@dianaclarke I think you might have been the last person to touch this area? Any thoughts?

I'm pretty new to Arrow, and I'm not quite sure I follow. What's failing?

The Archery & Crossbow / Archery Unittests and Crossbow Check Config check passed.

Are you referring to the Integration / AMD64 Conda Integration Test (pull_request) check? It appears to be out of disk space.

npm ERR! nospc ENOSPC: no space left on device, write
npm ERR! nospc There appears to be insufficient space on your system to finish.

I'm probably too n00b to be of much help on this issue, but good luck!

That said, I do hope to find some time to collaborate on the following PR with you all.

ARROW-10031: [CI][Java] Support Java benchmark in Archery (#8210)

I would be fun to get these Java benchmarks hooked up to Conbench. I have a blog post coming soon that better explains the Arrow/Archery/Conbench integration. Until then:

https://conbench.ursa.dev/
https://github.com/ursacomputing/conbench
https://github.com/ursacomputing/benchmarks

Cheers & stay safe!

emkornfield · 2021-03-27T21:33:17Z

@dianaclarke hmm, it looks like "Archery & Crossbow / Archery Unittests and Crossbow Check Config (pull_request" passed this time, it seems like it might be flaky (i don't know that there is a great way to see old runs). Yeah, I think the integration test failure is completely unrelated and there is already an open JIRA for it.

liyafan82 · 2021-03-29T09:36:55Z

java/compression/src/main/java/org/apache/arrow/compression/ZstdCompressionCodec.java

+
+  @Override
+  protected ArrowBuf doCompress(BufferAllocator allocator, ArrowBuf uncompressedBuffer) {
+    long maxSize = Zstd.compressBound(uncompressedBuffer.writerIndex());


Is it possible to cause too much waste of memroy in the worst case?

I think this is just a fact of life with compression algorithms. There are potentially ways of mitigating memory waste if changed the APIs around a little bit (trying to pack more data into a single ArrowBuf) or consolidating buffers into a larger buffer if compression level is high. But this is the guidance the documented methods give for sizing buffers.

liyafan82 · 2021-03-29T09:37:56Z

java/compression/src/main/java/org/apache/arrow/compression/ZstdCompressionCodec.java

+    long bytesWritten = Zstd.compressUnsafe(
+                          compressedBuffer.memoryAddress() + CompressionUtil.SIZE_OF_UNCOMPRESSED_LENGTH, dstSize,
+                          /*src*/uncompressedBuffer.memoryAddress(), /*srcSize=*/uncompressedBuffer.writerIndex(),
+                          /*level=*/3);


Should we make the level configurable?

maybe at some point, the default is 3. LZ4 also has a compression level which I don't think we are setting.

I'll open up a JIRA to track configuration.

liyafan82 · 2021-03-29T09:43:09Z

I am curious to know about the performance of this codec (I suppose it should be good, as there are no on-heap/off-heap data copies).
Maybe we can run some benchmarks after ARROW-11901 checks in the benchmarks.

emkornfield · 2021-03-29T15:38:57Z

Yes, I expect it to be good. I'm going to try to run some end-to-end experiments and I can report back numbers.

emkornfield · 2021-03-29T21:46:35Z

@dianaclarke https://github.com/apache/arrow/pull/9822/checks?check_run_id=2219940455

dianaclarke · 2021-03-29T22:13:57Z

@dianaclarke https://github.com/apache/arrow/pull/9822/checks?check_run_id=2219940455

Ah, now I see. I didn't write that test. I think @bkietz did, but I can try to find some time this week to dig into it.

dianaclarke · 2021-03-29T22:37:28Z

@dianaclarke https://github.com/apache/arrow/pull/9822/checks?check_run_id=2219940455

Ah, now I see. I didn't write that test. I think @bkietz did, but I can try to find some time this week to dig into it.

I see what the issue is. That test is assuming order, when it can't. I'll create a ticket and submit a PR shortly.

[Developer][Archery] Flaky test: test_static_runner_from_json
https://issues.apache.org/jira/browse/ARROW-12145

emkornfield · 2021-03-31T04:15:27Z

@liyafan82 I opened https://issues.apache.org/jira/browse/ARROW-12163 to track making compression levels configurable, any other concerns?

liyafan82 · 2021-03-31T04:38:16Z

@liyafan82 I opened https://issues.apache.org/jira/browse/ARROW-12163 to track making compression levels configurable, any other concerns?

@emkornfield My only concern is performance. If we are going to address it in ARROW-11901, this PR is ready for merge IMO.

emkornfield · 2021-03-31T04:40:04Z

@emkornfield My only concern is performance. If we are going to address it in ARROW-11901, this PR is ready for merge IMO.

Yes, I think functionality should come first and we can measure and iterate on performance. Thank you for your review.

emkornfield · 2021-04-01T03:58:22Z

going to merge.

* Add ZSTD codec implementation. * Removes dependency on netty by using ArrowBuf methods. * Updates docs and archery test (will wait on CI to run archery if that fails will do more debugging.) Closes apache#9822 from emkornfield/zstd Lead-authored-by: emkornfield <[email protected]> Co-authored-by: Micah Kornfield <[email protected]> Signed-off-by: Micah Kornfield <[email protected]>

emkornfield added 2 commits March 27, 2021 04:13

ARROW-12110: [Java] Implement ZSTD compression

b5bcf68

add missing file

c324527

emkornfield requested a review from liyafan82 March 27, 2021 04:17

github-actions bot added the Component: Java label Mar 27, 2021

nevi-me reviewed Mar 27, 2021

View reviewed changes

emkornfield added 3 commits March 26, 2021 21:28

Update runner.py

b97e83f

Add link back to this PR for disabling specific codecs.

Update ZstdCompressionCodec.java

cd0f4ba

check decompressed length.

Update runner.py

c9216a8

liyafan82 reviewed Mar 29, 2021

View reviewed changes

Merge branch 'master' into zstd

944f655

emkornfield requested a review from liyafan82 March 29, 2021 15:46

emkornfield closed this in 971a9d3 Apr 1, 2021

emkornfield mentioned this pull request Apr 1, 2021

ARROW-12168: [Go][IPC] Implement Compression handling for Arrow IPC #9862

Closed

emkornfield deleted the zstd branch April 7, 2021 15:38

asfimport mentioned this pull request May 12, 2021

[Java] Implement ZSTD buffer compression for java #27933

Closed

Conversation

emkornfield commented Mar 27, 2021

Uh oh!

github-actions bot commented Mar 27, 2021

Uh oh!

nevi-me Mar 27, 2021

Choose a reason for hiding this comment

Uh oh!

emkornfield Mar 27, 2021

Choose a reason for hiding this comment

Uh oh!

emkornfield commented Mar 27, 2021

Uh oh!

dianaclarke commented Mar 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emkornfield commented Mar 27, 2021

Uh oh!

liyafan82 Mar 29, 2021

Choose a reason for hiding this comment

Uh oh!

emkornfield Mar 29, 2021

Choose a reason for hiding this comment

Uh oh!

liyafan82 Mar 29, 2021

Choose a reason for hiding this comment

Uh oh!

emkornfield Mar 29, 2021

Choose a reason for hiding this comment

Uh oh!

emkornfield Mar 29, 2021

Choose a reason for hiding this comment

Uh oh!

liyafan82 commented Mar 29, 2021

Uh oh!

emkornfield commented Mar 29, 2021

Uh oh!

emkornfield commented Mar 29, 2021

Uh oh!

dianaclarke commented Mar 29, 2021

Uh oh!

dianaclarke commented Mar 29, 2021

Uh oh!

emkornfield commented Mar 31, 2021

Uh oh!

liyafan82 commented Mar 31, 2021

Uh oh!

emkornfield commented Mar 31, 2021

Uh oh!

emkornfield commented Apr 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dianaclarke commented Mar 27, 2021 •

edited

Loading