Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.hadoop.shaded.net.jpountz.lz4;
Comment thread
dongjoon-hyun marked this conversation as resolved.
Outdated

/**
* A temporary workaround for SPARK-36669. We should remove this after Hadoop 3.3.2 release

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to We should remove this after Hadoop 3.3.2 release, we had better have an IDed TODO officially. If we already have could you add it here, please?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've not created one, let me create it and add it here. Thanks.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added TODO(SPARK-36679).

* which fixes the LZ4 relocation in shaded Hadoop client libraries. This does not need
* implement all net.jpountz.lz4.LZ4Compressor API, just the ones used by Hadoop Lz4Compressor.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now this is not a test-only PR. Can we update the PR title and description accordingly?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea, let me update it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

*/
public final class LZ4Compressor {

private net.jpountz.lz4.LZ4Compressor lz4Compressor;

public LZ4Compressor(net.jpountz.lz4.LZ4Compressor lz4Compressor) {
this.lz4Compressor = lz4Compressor;
}

public void compress(java.nio.ByteBuffer src, java.nio.ByteBuffer dest) {
lz4Compressor.compress(src, dest);
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.hadoop.shaded.net.jpountz.lz4;

/**
* A temporary workaround for SPARK-36669. We should remove this after Hadoop 3.3.2 release
* which fixes the LZ4 relocation in shaded Hadoop client libraries. This does not need
* implement all net.jpountz.lz4.LZ4Factory API, just the ones used by Hadoop Lz4Compressor.
*/
public final class LZ4Factory {

private net.jpountz.lz4.LZ4Factory lz4Factory;

public LZ4Factory(net.jpountz.lz4.LZ4Factory lz4Factory) {
this.lz4Factory = lz4Factory;
}

public static LZ4Factory fastestInstance() {
return new LZ4Factory(net.jpountz.lz4.LZ4Factory.fastestInstance());
}

public LZ4Compressor highCompressor() {
return new LZ4Compressor(lz4Factory.highCompressor());
}

public LZ4Compressor fastCompressor() {
return new LZ4Compressor(lz4Factory.fastCompressor());
}

public LZ4SafeDecompressor safeDecompressor() {
return new LZ4SafeDecompressor(lz4Factory.safeDecompressor());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.hadoop.shaded.net.jpountz.lz4;

/**
* A temporary workaround for SPARK-36669. We should remove this after Hadoop 3.3.2 release
* which fixes the LZ4 relocation in shaded Hadoop client libraries. This does not need
* implement all net.jpountz.lz4.LZ4SafeDecompressor API, just the ones used by Hadoop
* Lz4Decompressor.
*/
public final class LZ4SafeDecompressor {
private net.jpountz.lz4.LZ4SafeDecompressor lz4Decompressor;

public LZ4SafeDecompressor(net.jpountz.lz4.LZ4SafeDecompressor lz4Decompressor) {
this.lz4Decompressor = lz4Decompressor;
}

public void decompress(java.nio.ByteBuffer src, java.nio.ByteBuffer dest) {
lz4Decompressor.decompress(src, dest);
}
}
7 changes: 7 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,13 @@
<enabled>false</enabled>
</snapshots>
</repository>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, remove empty line?

<repository>
<id>jitpack.io</id>
<url>https://jitpack.io</url>
<name>Jitpack.io repository</name>
<!-- needed for brotli-codec -->
</repository>
</repositories>
<pluginRepositories>
<pluginRepository>
Expand Down
4 changes: 3 additions & 1 deletion project/SparkBuild.scala
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,9 @@ object SparkBuild extends PomBuild {
"gcs-maven-central-mirror" at "https://maven-central.storage-download.googleapis.com/maven2/",
DefaultMavenRepository,
Resolver.mavenLocal,
Resolver.file("ivyLocal", file(Path.userHome.absolutePath + "/.ivy2/local"))(Resolver.ivyStylePatterns)
Resolver.file("ivyLocal", file(Path.userHome.absolutePath + "/.ivy2/local"))(Resolver.ivyStylePatterns),
// needed for brotli-codec
"jitpack.io" at "https://jitpack.io"
),
externalResolvers := resolvers.value,
otherResolvers := SbtPomKeys.mvnLocalRepository(dotM2 => Seq(Resolver.file("dotM2", dotM2))).value,
Expand Down
11 changes: 11 additions & 0 deletions sql/core/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,17 @@
<artifactId>htmlunit-driver</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.lz4</groupId>
<artifactId>lz4-java</artifactId>
<scope>test</scope>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have this as a test scope, I'm wondering if we need to mention some additional info to the customers in the migration document. What do you recommend for the release, @viirya ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For sql/core, we don't include lz4-java originally. So I only have it as test scope for test purpose. For customers, they don't need to do any change. If they need lz4 codec, they just add lz4-java dependency as usual.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this is the same behavior across all Spark versions (including 3.2.0 RC2), right?

If they need lz4 codec, they just add lz4-java dependency as usual.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I found core module already has lz4-java (compile) dependency. So we don't need add it here again.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's great!

</dependency>
<dependency>
<groupId>com.github.rdblue</groupId>
<artifactId>brotli-codec</artifactId>
<version>0.1.1</version>
<scope>test</scope>
</dependency>
</dependencies>
<build>
<outputDirectory>target/scala-${scala.binary.version}/classes</outputDirectory>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.spark.sql.execution.datasources

import org.apache.spark.sql.QueryTest
import org.apache.spark.sql.test.SQLTestUtils

abstract class DataSourceCodecTest extends QueryTest with SQLTestUtils {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are going to test only file-based data source, we can make a single simple suite like FileBasedDataSourceSuite.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, FileBasedDataSourceCodecSuite?

  private val allFileBasedDataSources = Seq("orc", "parquet", ...)

  allFileBasedDataSources.foreach { format =>
    test(s"... - $format") {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, sounds good. let me refactor it.

@cloud-fan cloud-fan Sep 6, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this code style is a bit hard to extend (e.g. how to test avro?) and I was planning to refactor the existing test suites as well.

I think a better solution is

trait FileSourceCodecSuite ... {
  def format: String
  ...
}

class ParquetCodecSuite extends FileSourceCodecSuite

class OrcCodecSuite ...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cloud-fan's idea is more close to what I have in mind at the beginning. @dongjoon-hyun is it good for you too?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I think these test suites can be in one file if possible.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, @viirya and @cloud-fan , +1 for the proposed structure in a single file.


protected def dataSourceName: String
protected val codecConfigName: String
protected def availableCodecs: Seq[String]

def testWithAllCodecs(name: String)(f: => Unit): Unit = {
for (codec <- availableCodecs) {
test(s"$name - data source $dataSourceName - codec: $codec") {
withSQLConf(codecConfigName -> codec) {
f
}
}
}
}

testWithAllCodecs("write and read - single partition") {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case seems to be included in write and read. Do we need this test case separately?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only the partition number is different.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, it looks like that. In that case, there is no difference in terms of the test coverage.

withTempPath { dir =>
testData
.repartition(1)
.write
.format(dataSourceName)
.save(dir.getCanonicalPath)

val df = spark.read.format(dataSourceName).load(dir.getCanonicalPath)
checkAnswer(df, testData)
}
}

testWithAllCodecs("write and read") {
withTempPath { dir =>
testData
.repartition(5)
.write
.format(dataSourceName)
.save(dir.getCanonicalPath)

val df = spark.read.format(dataSourceName).load(dir.getCanonicalPath)
checkAnswer(df, testData)
}
}
}


Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.spark.sql.execution.datasources.orc

import org.apache.spark.sql.execution.datasources.DataSourceCodecTest
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.test.SharedSparkSession

class OrcCodecTestSuite extends DataSourceCodecTest with SharedSparkSession{

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, OrcCodecTestSuite -> OrcCodecSuite?


override def dataSourceName: String = "orc"
override val codecConfigName = SQLConf.ORC_COMPRESSION.key
override protected def availableCodecs = Seq("none", "uncompressed", "snappy",
"zlib", "zstd", "lz4", "lzo")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To reviewers: As you see here, Apache ORC has no issue because it uses AircompressorCodec LZ4.

}

Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.spark.sql.execution.datasources.parquet

import org.apache.spark.sql.execution.datasources.DataSourceCodecTest
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.test.SharedSparkSession

class ParquetCodecTestSuite extends DataSourceCodecTest with SharedSparkSession {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, ParquetCodecTestSuite -> ParquetCodecSuite


override def dataSourceName: String = "parquet"
override val codecConfigName = SQLConf.PARQUET_COMPRESSION.key
// Exclude "lzo" because it is GPL-licenced so not included in Hadoop.
override protected def availableCodecs: Seq[String] = Seq("none", "uncompressed", "snappy",
"gzip", "brotli", "zstd", "lz4")
}