Add support for JNI based decompression for zstd files

[gauravkm]

Initial results and estimates point to 10% improvement in CPU by using JNI.

The feature is controlled by a flag, that is false by default.

I tried to split the pull requests but there is no way to stack them across different forks.

[highker]

• Coding style comments
• Could you remove binary files like sample_zstd? Just randomly generate them with as temporary files and read them back

[highker]

Could you follow our developer guideline (https://github.com/prestodb/presto/wiki/Presto-Development-Guidelines#formatting) and clean up this patch including:

• Spell out every word (e.g., src -> source)
• Provide meaningful names to variables (e.g., i -> length)
• We don't use ArrayList, LinkedList, and other Java native containers unless necessary (e.g., mutability or nulls). Instead, we use Guava ImmutableSet/Map/List

[highker]

Let's use DataSize type

[gauravkm]

(y)

[highker]

Also, we enforce one parameter per line

[gauravkm]

Could you remove binary files like sample_zstd? Just randomly generate them with as temporary files and read them back

This are really ORC files. Not possible to generate them randomly. I can rename them to say sample_orc

[mbasmanova]

@gauravkm

This are really ORC files. Not possible to generate them randomly. I can rename them to say sample_orc

Gaurav, you can generate ORC files on the fly. See com.facebook.presto.orc.OrcTester#writeOrcColumnHive

[highker]

some comments

[highker]

Unrelated change?

[gauravkm]

Will put a separate diff if required. For some reason, this would not allow Idea to compile the code.

[highker]

else { is redundant

[highker]

spell out dst

[highker]

remove this.

[highker]

break a new line after }

[highker]

same

[highker]

spell out src

[highker]

one param per line

[highker]

Do we wanna test on Zstd.isError(size)?

[gauravkm]

Yes, we should I missed this part. I am working on making this change. Thanks for looking at it again. Will make the other changes as well.

[highker]

This reuses aircompressor. So two options maybe:

1. Share the same codepath with OrcZstdDecompressor and pass in the jni flag to decide which decompressor (jni or airlift) to use within OrcZstdDecompressor, or
2. Keep this class and remove aircompressor dependency.

[highker]

minor comments; otherwise lgtm

[highker]

nit: we don't usually use final on method. (Also, as a side note: we don't use final for tmp variables)

[highker]

same, remove final

[highker]

private

[highker]

put final to all of them

[highker]

Maybe just call it createTestingReaderOptions

[highker]

Also call this function createTestingReaderOptions and it should delegate to the one below with zstdJniDecompressionEnabled = false.

[highker]

move this closer to the other writeOrcColumnPresto

[highker]

Shall we assert on the data equal. Size equality seems a bit weak.

[highker]

nit

this.decompressor = (input, inputOffset, inputLength, output, outputOffset, maxOutputLength) -> {
    long size = Zstd.decompressByteArray(output, 0, maxOutputLength, input, inputOffset, inputLength);
    if (Zstd.isError(size)) {
        throw new RuntimeException(Zstd.getErrorName(size));
    }
    return toIntExact(size);
};

[gauravkm]

I am not quite sure if we should throw RuntimeException or MalformedInputException.

Currently decompress catches the MalformInputException (which is a runtime exception) and rethrows it as OrcCorruptException which is a checked exception.

If we throw Runtime exception, then this behavior changes.

[highker]

check state here zstdJniDecompressionEnabled is true only if compression is ZSTD

[highker]

Actually no; no need to check state

[highker]

well done!