[SPARK-22282][SQL] Rename OrcRelation to OrcFileFormat and remove ORC_COMPRESSION #19502

dongjoon-hyun · 2017-10-16T00:55:26Z

What changes were proposed in this pull request?

This PR aims to

Rename OrcRelation to OrcFileFormat object.
Replace OrcRelation.ORC_COMPRESSION with org.apache.orc.OrcConf.COMPRESS. Since SPARK-21422, we can use OrcConf.COMPRESS instead of Hive's.

// The references of Hive's classes will be minimized.
val ORC_COMPRESSION = "orc.compress"

How was this patch tested?

Pass the Jenkins with the existing and updated test cases.

HyukjinKwon · 2017-10-16T02:07:05Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/orc/OrcFileFormat.scala

@@ -256,9 +257,6 @@ private[orc] class OrcOutputWriter(
 }

 private[orc] object OrcRelation extends HiveInspectors {


@dongjoon-hyun, mind changing this to OrcFileFormat while we are here (see #14529)?

Sure. No problem. Thank you for review, @HyukjinKwon !

HyukjinKwon · 2017-10-16T02:13:59Z

LGTM btw.

dongjoon-hyun · 2017-10-16T02:18:46Z

Thank you, @HyukjinKwon !

gatorsmile · 2017-10-16T02:36:57Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/orc/OrcOptions.scala

@@ -42,7 +44,7 @@ private[orc] class OrcOptions(
  val compressionCodec: String = {
    // `compression`, `orc.compress`, and `spark.sql.orc.compression.codec` are
    // in order of precedence from highest to lowest.


Please update the comment.

Thank you for review, @gatorsmile .
The name orc.compress and precedence order is not changed. Which part do you want to change?

COMPRESS.getAttribute is not in our code base. Please change it to

// compression, orc.compress(i.e., OrcConf.COMPRESS), and spark.sql.orc.compression.codec are

SparkQA · 2017-10-16T02:55:55Z

Test build #82771 has finished for PR 19502 at commit 7416e4a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2017-10-16T04:11:25Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/orc/OrcQuerySuite.scala

@@ -180,7 +181,7 @@ class OrcQuerySuite extends QueryTest with BeforeAndAfterAll with OrcTest {
    // Respect `orc.compress`.


Shall we change this comment too?

Also the test name Respect orc.compress option when compression is unset.

Sure. Thank you for review, @viirya .

SparkQA · 2017-10-16T04:11:56Z

Test build #82775 has finished for PR 19502 at commit 3d18d75.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2017-10-16T04:15:10Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/orc/OrcSourceSuite.scala

@@ -19,6 +19,7 @@ package org.apache.spark.sql.hive.orc

 import java.io.File

+import org.apache.orc.OrcConf.COMPRESS
 import org.scalatest.BeforeAndAfterAll



Another test in OrcSourceSuite:

spark/sql/hive/src/test/scala/org/apache/spark/sql/hive/orc/OrcSourceSuite.scala

Line 153 in d8f4540

assert(new OrcOptions(Map("Orc.Compress" -> "NONE"), conf).compressionCodec == "NONE")

This is case sensitivity test case. We should not change this.

Oh, I was thinking something like COMPRESS.getAttribute.toUpperCase.

If you want, no problem at all. :)

Just like to be consistent. Thanks. :)

viirya · 2017-10-16T04:18:46Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/orc/OrcFileFormat.scala

@@ -255,10 +256,7 @@ private[orc] class OrcOutputWriter(
  }
 }

-private[orc] object OrcRelation extends HiveInspectors {
-  // The references of Hive's classes will be minimized.
-  val ORC_COMPRESSION = "orc.compress"


We have documented orc.compress as option name explicitly in

spark/sql/core/src/main/scala/org/apache/spark/sql/DataFrameWriter.scala

Line 523 in d8f4540

* `orc.compress` and `spark.sql.parquet.compression.codec`. If `orc.compress` is given,

Now we depends on the configuration name from an external library. But I think the configuration name should not be changed at all. So looks should be fine.

Yep. I agree. I don't think Apache ORC changes this in the future since this is a primitive configuration.
BTW, Thank you for pointing this doc. I'll fix some typos here.

Oh, right, the parquet.

SparkQA · 2017-10-16T05:39:47Z

Test build #82778 has finished for PR 19502 at commit a554112.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-10-16T07:05:01Z

Test build #82782 has finished for PR 19502 at commit 2b0a092.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-10-16T07:05:01Z

Test build #82783 has finished for PR 19502 at commit 02792d9.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-10-16T07:05:02Z

Test build #82781 has finished for PR 19502 at commit b7335ac.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2017-10-16T07:07:45Z

retest this please

SparkQA · 2017-10-16T09:56:26Z

Test build #82788 has finished for PR 19502 at commit 02792d9.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2017-10-16T16:28:38Z

Thank you for retesting, @HyukjinKwon !

dongjoon-hyun · 2017-10-16T16:29:13Z

@HyukjinKwon , @gatorsmile , @viirya .
This is ready for review again. Thanks!

gatorsmile · 2017-10-16T18:27:25Z

Thanks! Merged to master.

dongjoon-hyun · 2017-10-16T18:44:10Z

Thank you, @gatorsmile , @HyukjinKwon , and @viirya !

[SPARK-22282][SQL] Remove OrcRelation.ORC_COMPRESSION

7416e4a

HyukjinKwon reviewed Oct 16, 2017

View reviewed changes

Rename OrcRelation to OrcFileFormat object.

3d18d75

dongjoon-hyun changed the title ~~[SPARK-22282][SQL] Remove OrcRelation.ORC_COMPRESSION~~ [SPARK-22282][SQL] Rename OrcRelation to OrcFileFormat and remove ORC_COMPRESSION Oct 16, 2017

gatorsmile reviewed Oct 16, 2017

View reviewed changes

Update comments.

a554112

viirya reviewed Oct 16, 2017

View reviewed changes

dongjoon-hyun added 3 commits October 15, 2017 22:06

Address comments.

b7335ac

fix typos.

2b0a092

fix

02792d9

asfgit closed this in 561505e Oct 16, 2017

dongjoon-hyun deleted the SPARK-22282 branch October 16, 2017 18:44

		@@ -256,9 +257,6 @@ private[orc] class OrcOutputWriter(
		}

		private[orc] object OrcRelation extends HiveInspectors {

		@@ -180,7 +181,7 @@ class OrcQuerySuite extends QueryTest with BeforeAndAfterAll with OrcTest {
		// Respect `orc.compress`.

[SPARK-22282][SQL] Rename OrcRelation to OrcFileFormat and remove ORC_COMPRESSION #19502

[SPARK-22282][SQL] Rename OrcRelation to OrcFileFormat and remove ORC_COMPRESSION #19502

Uh oh!

Conversation

dongjoon-hyun commented Oct 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Oct 16, 2017

Uh oh!

dongjoon-hyun commented Oct 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Oct 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Oct 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya Oct 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Oct 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 16, 2017

Uh oh!

SparkQA commented Oct 16, 2017

Uh oh!

SparkQA commented Oct 16, 2017

Uh oh!

SparkQA commented Oct 16, 2017

Uh oh!

HyukjinKwon commented Oct 16, 2017

Uh oh!

SparkQA commented Oct 16, 2017

Uh oh!

dongjoon-hyun commented Oct 16, 2017

Uh oh!

dongjoon-hyun commented Oct 16, 2017

Uh oh!

gatorsmile commented Oct 16, 2017

Uh oh!

dongjoon-hyun commented Oct 16, 2017

dongjoon-hyun commented Oct 16, 2017 •

edited

Loading

dongjoon-hyun Oct 16, 2017 •

edited

Loading

dongjoon-hyun Oct 16, 2017 •

edited

Loading

viirya Oct 16, 2017 •

edited

Loading

dongjoon-hyun Oct 16, 2017 •

edited

Loading