SNOW-818891 Remove Arrow and other unneeded dependencies #497

sfc-gh-lsembera · 2023-05-16T13:18:18Z

This PR removes Arrow and the some transitive dependencies of hadoop-common: Apache Curator (Zookeeper client), commons-logging, jsch (SSH client). The JAR size shrunk from ~49MB to ~41MB. Build runtime has been cut significantly.

Testing: Dew tests passed

codecov · 2023-05-16T14:19:54Z

Codecov Report

Merging #497 (446e9ae) into master (5e54f7e) will decrease coverage by 2.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #497      +/-   ##
==========================================
- Coverage   76.97%   74.85%   -2.13%     
==========================================
  Files          79       77       -2     
  Lines        5273     4764     -509     
  Branches      460      419      -41     
==========================================
- Hits         4059     3566     -493     
+ Misses       1024     1017       -7     
+ Partials      190      181       -9

Impacted Files	Coverage Δ
...e/ingest/streaming/internal/AbstractRowBuffer.java	`94.00% <ø> (+1.51%)`	⬆️
...wflake/ingest/streaming/internal/BlobMetadata.java	`100.00% <ø> (ø)`
...wflake/ingest/streaming/internal/ChannelCache.java	`87.50% <ø> (ø)`
...owflake/ingest/streaming/internal/ChannelData.java	`97.87% <ø> (ø)`
...wflake/ingest/streaming/internal/FlushService.java	`79.02% <ø> (+0.58%)`	⬆️
...t/snowflake/ingest/streaming/internal/Flusher.java	`100.00% <ø> (ø)`
...ke/ingest/streaming/internal/ParquetRowBuffer.java	`87.69% <ø> (ø)`
...ake/ingest/streaming/internal/RegisterService.java	`88.17% <ø> (ø)`
...ternal/SnowflakeStreamingIngestClientInternal.java	`86.53% <ø> (-2.11%)`	⬇️
...ngest/streaming/internal/StreamingIngestStage.java	`76.47% <ø> (ø)`
... and 8 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

sfc-gh-japatel

LGTM. Thanks for cleanup!

sfc-gh-tzhang

Thanks @sfc-gh-lsembera ! This is huge! Please make sure to ask Andrey or Gloria to take a look

sfc-gh-tzhang · 2023-05-24T20:22:02Z

...ain/java/net/snowflake/ingest/streaming/internal/SnowflakeStreamingIngestChannelFactory.java

-          allocator);
-    }
-
-    private BufferAllocator createBufferAllocator() {


So the memory allocation logic is inside the Parquet library? What if we want to use a different allocator per channel/table for usage tracking and better estimation?

So the memory allocation logic is inside the Parquet library?

Yes.

What if we want to use a different allocator per channel/table for usage tracking and better estimation?

That's a good point, we might need it.

If we need it, we can get it from git history, it is not a complicated code.

sfc-gh-tzhang · 2023-05-24T20:27:54Z

The code coverage decreases by 2.5%, could you make sure to add the tests for Parquet?

sfc-gh-gdoci · 2023-05-25T07:46:33Z

src/main/java/net/snowflake/ingest/utils/Constants.java

    // Unused (previously Arrow with per column compression.
    // TWO(2),


nit: we can remove this too.

sfc-gh-gdoci · 2023-05-25T07:52:52Z

...ain/java/net/snowflake/ingest/streaming/internal/SnowflakeStreamingIngestChannelFactory.java

-          allocator);
-    }
-
-    private BufferAllocator createBufferAllocator() {


So the memory allocation logic is inside the Parquet library?

Yes.

What if we want to use a different allocator per channel/table for usage tracking and better estimation?

That's a good point, we might need it.

sfc-gh-rcheng · 2023-05-25T16:11:06Z

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

-          {"Arrow", Constants.BdecVersion.ONE},
-          {"Parquet_w/o_optimization", Constants.BdecVersion.THREE}
-        });
+        new Object[][] {{"Parquet_w/o_optimization", Constants.BdecVersion.THREE}});


we can remove this parameter logic as a whole, since it is just testing parquet

sfc-gh-azagrebin

Thanks Lukas!

sfc-gh-azagrebin · 2023-05-30T16:24:21Z

src/main/java/net/snowflake/ingest/streaming/internal/BlobBuilder.java

-        int compressedChunkLength = compressionResult.getSecond();
+        Pair<byte[], Integer> paddedChunk =
+            padChunk(chunkData, Constants.ENCRYPTION_ALGORITHM_BLOCK_SIZE_BYTES);
+        byte[] compressedAndPaddedChunkData = paddedChunk.getFirst();


Suggested change

byte[] compressedAndPaddedChunkData = paddedChunk.getFirst();

byte[] paddedChunkData = paddedChunk.getFirst();

sfc-gh-azagrebin · 2023-05-30T16:24:34Z

src/main/java/net/snowflake/ingest/streaming/internal/BlobBuilder.java

+        Pair<byte[], Integer> paddedChunk =
+            padChunk(chunkData, Constants.ENCRYPTION_ALGORITHM_BLOCK_SIZE_BYTES);
+        byte[] compressedAndPaddedChunkData = paddedChunk.getFirst();
+        int compressedChunkLength = paddedChunk.getSecond();


Suggested change

int compressedChunkLength = paddedChunk.getSecond();

int paddedChunkLength = paddedChunk.getSecond();

sfc-gh-azagrebin · 2023-05-30T16:28:03Z

src/main/java/net/snowflake/ingest/utils/Constants.java

  public enum BdecVersion {
-    /** Uses Arrow to generate BDEC chunks. */
-    ONE(1),


I would consider to comment it out for history, just to see what we used prev numbers for, like TWO(2)

Commented out

sfc-gh-azagrebin · 2023-05-30T16:29:15Z

src/main/resources/META-INF/third-party-licenses/REVISED_BSD.txt

@@ -1,30 +0,0 @@
-JSch 0.0.* was released under the GNU LGPL license.  Later, we have switched


is it Arrow related? how to know?

This PR removes other dependencies, not just Arrow. This license file is removed because the jsch (an SSH client pulled in by hadoop-common) is being removed.

sfc-gh-azagrebin · 2023-05-30T16:31:08Z

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


-@RunWith(Parameterized.class)


why remove Parameterized only in this test? :)

Removed from everywhere

sfc-gh-lsembera · 2023-05-31T11:49:15Z

Thanks @sfc-gh-lsembera ! This is huge! Please make sure to ask Andrey or Gloria to take a look

@sfc-gh-tzhang I reviewed the codecov report and there aren't any new holes in coverage. The reason for lower coverage is that the some higher-covered Arrow code has been removed, which lowered the overall coverage.

sfc-gh-lsembera force-pushed the lsembera/dependency-review branch from aa55d3c to cebefe0 Compare May 16, 2023 13:29

sfc-gh-lsembera force-pushed the lsembera/dependency-review branch 3 times, most recently from 67bbdc7 to e263fbb Compare May 24, 2023 12:36

sfc-gh-lsembera changed the title ~~SNOW-818891 Remove unneeded dependencies~~ SNOW-818891 Remove Arrow and other unneeded dependencies May 24, 2023

sfc-gh-lsembera force-pushed the lsembera/dependency-review branch 2 times, most recently from e5e6cce to 86cc28a Compare May 24, 2023 16:01

sfc-gh-lsembera marked this pull request as ready for review May 24, 2023 16:11

sfc-gh-lsembera requested review from a team and sfc-gh-tzhang as code owners May 24, 2023 16:11

sfc-gh-japatel approved these changes May 24, 2023

View reviewed changes

sfc-gh-tzhang approved these changes May 24, 2023

View reviewed changes

sfc-gh-tzhang requested review from sfc-gh-azagrebin and sfc-gh-gdoci May 24, 2023 20:27

sfc-gh-gdoci reviewed May 25, 2023

View reviewed changes

sfc-gh-rcheng approved these changes May 25, 2023

View reviewed changes

SNOW-818891 Remove Arrow and other unneeded dependenies

0534b00

sfc-gh-lsembera force-pushed the lsembera/dependency-review branch from 86cc28a to 0534b00 Compare May 30, 2023 09:40

Code review comments #1

063bcfc

sfc-gh-azagrebin approved these changes May 30, 2023

View reviewed changes

Code review fixes

446e9ae

sfc-gh-gdoci approved these changes May 31, 2023

View reviewed changes

sfc-gh-lsembera merged commit 9d1ab3d into master May 31, 2023
10 of 12 checks passed

sfc-gh-lsembera deleted the lsembera/dependency-review branch May 31, 2023 11:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SNOW-818891 Remove Arrow and other unneeded dependencies #497

SNOW-818891 Remove Arrow and other unneeded dependencies #497

sfc-gh-lsembera commented May 16, 2023 •

edited

Loading

codecov bot commented May 16, 2023 •

edited

Loading

sfc-gh-japatel left a comment

sfc-gh-tzhang left a comment

sfc-gh-tzhang May 24, 2023

sfc-gh-gdoci May 25, 2023

sfc-gh-lsembera May 25, 2023

sfc-gh-tzhang commented May 24, 2023

sfc-gh-gdoci May 25, 2023

sfc-gh-lsembera May 30, 2023

sfc-gh-gdoci May 25, 2023

sfc-gh-rcheng May 25, 2023

sfc-gh-lsembera May 30, 2023

sfc-gh-azagrebin left a comment

sfc-gh-azagrebin May 30, 2023

sfc-gh-lsembera May 31, 2023

sfc-gh-azagrebin May 30, 2023

sfc-gh-lsembera May 31, 2023

sfc-gh-azagrebin May 30, 2023

sfc-gh-lsembera May 31, 2023

sfc-gh-azagrebin May 30, 2023

sfc-gh-lsembera May 31, 2023 •

edited

Loading

sfc-gh-azagrebin May 30, 2023

sfc-gh-lsembera May 31, 2023

sfc-gh-lsembera commented May 31, 2023

		// Unused (previously Arrow with per column compression.
		// TWO(2),

	byte[] compressedAndPaddedChunkData = paddedChunk.getFirst();
	byte[] paddedChunkData = paddedChunk.getFirst();

	int compressedChunkLength = paddedChunk.getSecond();
	int paddedChunkLength = paddedChunk.getSecond();

		@@ -1,30 +0,0 @@
		JSch 0.0.* was released under the GNU LGPL license. Later, we have switched


		@RunWith(Parameterized.class)

SNOW-818891 Remove Arrow and other unneeded dependencies #497

SNOW-818891 Remove Arrow and other unneeded dependencies #497

Conversation

sfc-gh-lsembera commented May 16, 2023 • edited Loading

codecov bot commented May 16, 2023 • edited Loading

Codecov Report

sfc-gh-japatel 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-tzhang commented May 24, 2023

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-azagrebin 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-lsembera May 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-lsembera commented May 31, 2023

sfc-gh-lsembera commented May 16, 2023 •

edited

Loading

codecov bot commented May 16, 2023 •

edited

Loading

sfc-gh-lsembera May 31, 2023 •

edited

Loading