SNOW-983635 Allow ZSTD compression algorithm #654

sfc-gh-lthiede · 2023-12-05T16:53:49Z

Test setting ZSTD as default compression algorithm

sfc-gh-lthiede · 2024-01-04T14:42:41Z

Maybe add zstd-jni as dependency in README.md Edit: done

sfc-gh-lthiede · 2024-01-09T13:39:04Z

DEW testing

DEW tests using a local release of the Snowpipe Streaming SDK with my changes and ZSTD as the default compression algorithm passed.
To run successfully I had to add a dependency on zstd-jni to the DEW dependencies (PR)

sfc-gh-lthiede · 2024-01-22T14:49:09Z

How I ran the precommits

I ran a precommit with my branch rebased on sdk release 2.0.5.
I ran a second precommit with sdk release 2.0.5 to check which test failures are actually caused by my changes.
I ran both precommits with some changes to the BDEC file generator and its build in RegressionTests. (PR)
The changes fixed the file generator for SDK release 2.0.5 and improved the its build.

Results

4 tests failed, that also failed on master. 9 tests failed because of reduced byte counts. 1 test failed because of a higher byte count. 1 test failed unexpectedly.

Is zstd sometimes leading to a higher byte count a problem?
@sfc-gh-gdoci suggested that the unexpected failure is probably flaky. I ran another precommit and the failure didn't show up again.

sfc-gh-tzhang

Thanks for the change! I think we need more test coverage here, for example: we need coverage to make sure it works on other data types. We probably have most of them which you can just add a parameter like what you did in StreamingIngestBigFilesIT.java

sfc-gh-tzhang · 2024-01-23T05:32:33Z

README.md

@@ -17,6 +17,7 @@ The Snowflake Ingest Service SDK depends on the following libraries:

 * snowflake-jdbc (3.13.30 to 3.13.33) 
 * slf4j-api
+* com.github.luben:zstd-jni (1.5.0-1)


Why we decide to treat this as one of the special dependencies? @sfc-gh-lsembera has more context here if you need more help

Hi, the problem is that zstd-jni can't be shaded. Because of that we can't include it in our sdk's shaded jar. It has to be provided separately when customers want to build their application with our sdk. I thought this was reason enough to list it with our dependencies.

@sfc-gh-lsembera Any idea if we can do better here? Or any potential issue? I'm concerned that we keep adding to this list

I think it is good if shade fewer dependencies, and this change goes in that direction. Another advantage of the unshaded approach here is that if the customer does not want to use zstd and wants smaller distribution, they can exclude the zstd jar from their maven project.

Looks like we stop relocating com.github.luben, is there any concern that this is a behavior change?

src/main/java/org/apache/parquet/hadoop/BdecParquetWriter.java

sfc-gh-tzhang · 2024-01-23T05:42:18Z

What's our plan of making ZSTD the default?

sfc-gh-lthiede · 2024-01-23T09:53:49Z

@sfc-gh-tzhang In the protocol of our Streaming Ingest Repeating Eng Sync on Dec 5th we wrote

[@sfc-gh-gdoci /Lorenz] Rolling out (risky) changes in the SDK, e.g. switching from Parquet page version v1 to v2

A way to control client parameters from Snowflake server-side to rollback in case of issues before customers are asked to upgrade to SDK version with the fix, e.g. override parameters in clientConfigure or openChannel request

[Toby] Wanted to do smth similar for Kafka connector SNOW-980722

[Toby] IMO makes sense to do, it’s a priority question. Could be a good starter task for Alec when he returns

I think this also applies to making ZSTD the default

src/test/java/net/snowflake/ingest/streaming/internal/StreamingIngestIT.java

Description Testing

…hecking for unused dependencies Description Testing

…fault Description Testing

Description Testing

…ZSTD Description Testing

sfc-gh-gdoci

Thank you.

sfc-gh-tzhang

Please consider me as approved if you can check with @sfc-gh-lsembera to see if we can do better on the ZSTD dependency, thanks!

sfc-gh-tzhang · 2024-01-27T01:40:06Z

README.md

@@ -17,6 +17,7 @@ The Snowflake Ingest Service SDK depends on the following libraries:

 * snowflake-jdbc (3.13.30 to 3.13.33) 
 * slf4j-api
+* com.github.luben:zstd-jni (1.5.0-1)


@sfc-gh-lsembera Any idea if we can do better here? Or any potential issue? I'm concerned that we keep adding to this list

public_pom.xml

unblock PR

sfc-gh-tzhang · 2024-01-27T01:59:16Z

@sfc-gh-tzhang In the protocol of our Streaming Ingest Repeating Eng Sync on Dec 5th we wrote

[@sfc-gh-gdoci /Lorenz] Rolling out (risky) changes in the SDK, e.g. switching from Parquet page version v1 to v2

A way to control client parameters from Snowflake server-side to rollback in case of issues before customers are asked to upgrade to SDK version with the fix, e.g. override parameters in clientConfigure or openChannel request

[Toby] Wanted to do smth similar for Kafka connector SNOW-980722

[Toby] IMO makes sense to do, it’s a priority question. Could be a good starter task for Alec when he returns

I think this also applies to making ZSTD the default

Ah, thanks for the reminder, we can do this, is this something you want to start if you have free time?

sfc-gh-lsembera · 2024-01-29T10:02:23Z

pom.xml

@@ -691,6 +694,9 @@
            <configuration>
              <failOnWarning>true</failOnWarning>
              <ignoreNonCompile>true</ignoreNonCompile>
+              <ignoredDependencies>
+                <ignoredDependency>com.github.luben:zstd-jni</ignoredDependency>


Why is this needed?

We declare zstd-jni as a dependency even though it's not directly used in our code. If we didn't ignore zstd-jni here analyze-only would fail due to unused dependency.

In that case, we can use the runtime maven scope?

sfc-gh-lsembera · 2024-01-29T10:04:20Z

pom.xml

@@ -1092,6 +1095,12 @@
                    <exclude>org/slf4j/**</exclude>
                  </excludes>
                </filter>
+                <filter>


We are not shading this dependency, why is this filter needed?

public_pom.xml

sfc-gh-lsembera · 2024-01-29T10:08:11Z

README.md

@@ -17,6 +17,7 @@ The Snowflake Ingest Service SDK depends on the following libraries:

 * snowflake-jdbc (3.13.30 to 3.13.33) 
 * slf4j-api
+* com.github.luben:zstd-jni (1.5.0-1)


I think it is good if shade fewer dependencies, and this change goes in that direction. Another advantage of the unshaded approach here is that if the customer does not want to use zstd and wants smaller distribution, they can exclude the zstd jar from their maven project.

Description Testing

…ncies filtered from shading, because it's already excluded Description Testing

sfc-gh-lthiede · 2024-01-29T10:59:16Z

Ah, thanks for the reminder, we can do this, is this something you want to start if you have free time?
@sfc-gh-tzhang Currently I'm busy with other parts of my project

Description Testing

sfc-gh-tzhang

lgtm, thanks!

sfc-gh-tzhang · 2024-01-30T00:42:18Z

README.md

@@ -17,6 +17,7 @@ The Snowflake Ingest Service SDK depends on the following libraries:

 * snowflake-jdbc (3.13.30 to 3.13.33) 
 * slf4j-api
+* com.github.luben:zstd-jni (1.5.0-1)


Looks like we stop relocating com.github.luben, is there any concern that this is a behavior change?

sfc-gh-lthiede · 2024-02-01T09:48:13Z

The README.md of the ZSTD library say the following. This is the reason we can't shade it.

The Java classes cannot be renamed/minimized/relocated. JVM linking the native library depends on the class name that is trying to link the native part, so changing the class names will lead to failed linking at runtime.

https://github.com/luben/zstd-jni/

sfc-gh-xhuang approved these changes Dec 13, 2023

View reviewed changes

sfc-gh-lthiede force-pushed the lthiede-SNOW-983635-ZSTD-compression branch 4 times, most recently from eed81af to d671c1f Compare January 19, 2024 10:01

sfc-gh-lthiede marked this pull request as ready for review January 22, 2024 14:49

sfc-gh-lthiede requested review from sfc-gh-tzhang and a team as code owners January 22, 2024 14:49

sfc-gh-lthiede changed the title ~~SNOW-983635 Set ZSTD as default compression algorithm~~ SNOW-983635 Allow ZSTD compression algorithm Jan 22, 2024

sfc-gh-tzhang previously requested changes Jan 23, 2024

View reviewed changes

sfc-gh-lthiede commented Jan 23, 2024

View reviewed changes

src/test/java/net/snowflake/ingest/streaming/internal/StreamingIngestIT.java Outdated Show resolved Hide resolved

sfc-gh-lthiede requested a review from sfc-gh-tzhang January 24, 2024 08:53

sfc-gh-lthiede added 12 commits January 24, 2024 14:48

SNOW-983635 Set ZSTD as default compression algorithm

a7927d5

Description Testing

SNOW-983635 Print supported Compression algos

5db8db3

Description Testing

SNOW-983635 Test creating a variable of type RecyclingBufferPool

326f9b4

Description Testing

SNOW-983635 Add ZSTD dependency

fe06a7f

Description Testing

SNOW-983635 Add version number to zstd dependency

189e894

Description Testing

SNOW-983635 Sort pom

c95d2fe

Description Testing

SNOW-983635 Use same version of zstd dependency as parquet

86c5dc5

Description Testing

SNOW-983635 Reexecute probably failing static member initialization

efaf824

Description Testing

SNOW-983635 Print UnsatisfiedLinkError

4cee250

Description Testing

SNOW-983635 Don't relocated com.github.luben.zstd

01f7337

Description Testing

SNOW-983635 Don't use luben.zstd directly

df2938f

Description Testing

SNOW-983635 Hopefully correctly exclude zstd library from shading

cb52f29

Description Testing

sfc-gh-lthiede added 8 commits January 24, 2024 14:48

SNOW-983635 Add zstd-jni to dependencies in pom.xml

67c3a69

Description Testing

SNOW-983635 Sort pom

7f09641

Description Testing

SNOW-983635 Remove direct zstd dependency

24148e3

Description Testing

SNOW-983635 Add zstd-jni as explicit dependency but exclude it from c…

7143018

…hecking for unused dependencies Description Testing

Delete dependency-reduced-pom.xml

ab25c68

SNOW-983635 Add zstd-jni dependency to README.md and make ZSTD non de…

601d01b

…fault Description Testing

SNOW-983635 Remove unnecessary new line

a8cdc30

Description Testing

SNOW-983635 Parameterise StreamingIngestIT.java to run with GZIP and …

ff91de2

…ZSTD Description Testing

sfc-gh-lthiede force-pushed the lthiede-SNOW-983635-ZSTD-compression branch from 7d03433 to ff91de2 Compare January 24, 2024 14:49

sfc-gh-gdoci approved these changes Jan 26, 2024

View reviewed changes

sfc-gh-tzhang reviewed Jan 27, 2024

View reviewed changes

sfc-gh-tzhang self-requested a review January 27, 2024 01:54

sfc-gh-lsembera reviewed Jan 29, 2024

View reviewed changes

sfc-gh-lthiede added 3 commits January 29, 2024 10:27

SNOW-983635 Add even more datatype tests with ZSTD

0e743c8

Description Testing

SNOW-983635 consistently use compile in public pom

17c4b25

Description Testing

SNOW-983635 Fix AbstractDataTypeTest and remove zstd-jni from depende…

94c62bf

…ncies filtered from shading, because it's already excluded Description Testing

sfc-gh-lthiede added 4 commits January 29, 2024 12:02

SNOW-983635 Make zstd-jni a runtime dependency

584e75c

Description Testing

SNOW-983635 Sort dependencies is pom

4574c8e

Description Testing

SNOW-983635 Temporarily set ZSTD as default again to test recent changes

fa71d94

Description Testing

SNOW-983635 Revert setting ZSTD as default compression algorithm

a95b87f

Description Testing

sfc-gh-lsembera approved these changes Jan 29, 2024

View reviewed changes

sfc-gh-asen approved these changes Jan 29, 2024

View reviewed changes

sfc-gh-tzhang approved these changes Jan 30, 2024

View reviewed changes

sfc-gh-lthiede merged commit 84420bf into master Feb 1, 2024
14 checks passed

sfc-gh-lthiede deleted the lthiede-SNOW-983635-ZSTD-compression branch February 1, 2024 09:49

sfc-gh-tzhang mentioned this pull request Feb 23, 2024

V2.1.0 Release #682

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SNOW-983635 Allow ZSTD compression algorithm #654

SNOW-983635 Allow ZSTD compression algorithm #654

sfc-gh-lthiede commented Dec 5, 2023

sfc-gh-lthiede commented Jan 4, 2024 •

edited

Loading

sfc-gh-lthiede commented Jan 9, 2024

sfc-gh-lthiede commented Jan 22, 2024 •

edited

Loading

sfc-gh-tzhang left a comment

sfc-gh-tzhang Jan 23, 2024

sfc-gh-lthiede Jan 23, 2024

sfc-gh-tzhang Jan 27, 2024

sfc-gh-lsembera Jan 29, 2024

sfc-gh-tzhang Jan 30, 2024

sfc-gh-tzhang commented Jan 23, 2024

sfc-gh-lthiede commented Jan 23, 2024 •

edited by jira bot

Loading

sfc-gh-gdoci left a comment

sfc-gh-tzhang left a comment

sfc-gh-tzhang Jan 27, 2024

sfc-gh-tzhang commented Jan 27, 2024 •

edited by jira bot

Loading

sfc-gh-lsembera Jan 29, 2024

sfc-gh-lthiede Jan 29, 2024

sfc-gh-lsembera Jan 29, 2024

sfc-gh-lsembera Jan 29, 2024

sfc-gh-lsembera Jan 29, 2024

sfc-gh-lthiede commented Jan 29, 2024

sfc-gh-tzhang left a comment

sfc-gh-tzhang Jan 30, 2024

sfc-gh-lthiede commented Feb 1, 2024

SNOW-983635 Allow ZSTD compression algorithm #654

SNOW-983635 Allow ZSTD compression algorithm #654

Conversation

sfc-gh-lthiede commented Dec 5, 2023

sfc-gh-lthiede commented Jan 4, 2024 • edited Loading

sfc-gh-lthiede commented Jan 9, 2024

DEW testing

sfc-gh-lthiede commented Jan 22, 2024 • edited Loading

How I ran the precommits

Results

sfc-gh-tzhang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-tzhang commented Jan 23, 2024

sfc-gh-lthiede commented Jan 23, 2024 • edited by jira bot Loading

sfc-gh-gdoci left a comment

Choose a reason for hiding this comment

sfc-gh-tzhang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-tzhang commented Jan 27, 2024 • edited by jira bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-lthiede commented Jan 29, 2024

sfc-gh-tzhang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-lthiede commented Feb 1, 2024

sfc-gh-lthiede commented Jan 4, 2024 •

edited

Loading

sfc-gh-lthiede commented Jan 22, 2024 •

edited

Loading

sfc-gh-lthiede commented Jan 23, 2024 •

edited by jira bot

Loading

sfc-gh-tzhang commented Jan 27, 2024 •

edited by jira bot

Loading