Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dongjoon-hyun wants to merge 6 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7416e4a
[SPARK-22282][SQL] Remove OrcRelation.ORC_COMPRESSION
dongjoon-hyun Oct 16, 2017
3d18d75
Rename OrcRelation to OrcFileFormat object.
dongjoon-hyun Oct 16, 2017
a554112
Update comments.
dongjoon-hyun Oct 16, 2017
b7335ac
Address comments.
dongjoon-hyun Oct 16, 2017
2b0a092
fix typos.
dongjoon-hyun Oct 16, 2017
02792d9
fix
dongjoon-hyun Oct 16, 2017
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
8 changes: 3 additions & 5 deletions sql/hive/src/main/scala/org/apache/spark/sql/hive/orc/OrcFileFormat.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import org.apache.hadoop.io.{NullWritable, Writable}
import org.apache.hadoop.mapred.{JobConf, OutputFormat => MapRedOutputFormat, RecordWriter, Reporter}
import org.apache.hadoop.mapreduce._
import org.apache.hadoop.mapreduce.lib.input.{FileInputFormat, FileSplit}
import org.apache.orc.OrcConf.COMPRESS

import org.apache.spark.TaskContext
import org.apache.spark.sql.SparkSession
Expand Down Expand Up @@ -72,7 +73,7 @@ class OrcFileFormat extends FileFormat with DataSourceRegister with Serializable

val configuration = job.getConfiguration

configuration.set(OrcRelation.ORC_COMPRESSION, orcOptions.compressionCodec)
configuration.set(COMPRESS.getAttribute, orcOptions.compressionCodec)
configuration match {
case conf: JobConf =>
conf.setOutputFormat(classOf[OrcOutputFormat])
Expand All @@ -93,7 +94,7 @@ class OrcFileFormat extends FileFormat with DataSourceRegister with Serializable

override def getFileExtension(context: TaskAttemptContext): String = {
val compressionExtension: String = {
val name = context.getConfiguration.get(OrcRelation.ORC_COMPRESSION)
val name = context.getConfiguration.get(COMPRESS.getAttribute)
OrcRelation.extensionsForCompressionCodecNames.getOrElse(name, "")
}

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

private[orc] object OrcRelation extends HiveInspectors {
Copy link
Member

@HyukjinKwon HyukjinKwon Oct 16, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Oct 16, 2017

Choose a reason for hiding this comment

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

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

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

@viirya viirya Oct 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Oct 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@viirya viirya Oct 16, 2017

Choose a reason for hiding this comment

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

Oh, right, the parquet.


// This constant duplicates `OrcInputFormat.SARG_PUSHDOWN`, which is unfortunately not public.
private[orc] val SARG_PUSHDOWN = "sarg.pushdown"

Expand Down
4 changes: 3 additions & 1 deletion sql/hive/src/main/scala/org/apache/spark/sql/hive/orc/OrcOptions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ package org.apache.spark.sql.hive.orc

import java.util.Locale

import org.apache.orc.OrcConf.COMPRESS

import org.apache.spark.sql.catalyst.util.CaseInsensitiveMap
import org.apache.spark.sql.internal.SQLConf

Expand All @@ -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.
Copy link
Member

@gatorsmile gatorsmile Oct 16, 2017

Choose a reason for hiding this comment

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

Please update the comment.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Oct 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

@gatorsmile gatorsmile Oct 16, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Oct 16, 2017

Choose a reason for hiding this comment

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

I see.

val orcCompressionConf = parameters.get(OrcRelation.ORC_COMPRESSION)
val orcCompressionConf = parameters.get(COMPRESS.getAttribute)
val codecName = parameters
.get("compression")
.orElse(orcCompressionConf)
Expand Down
5 changes: 3 additions & 2 deletions sql/hive/src/test/scala/org/apache/spark/sql/hive/orc/OrcQuerySuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import java.sql.Timestamp

import org.apache.hadoop.conf.Configuration
import org.apache.hadoop.hive.ql.io.orc.{OrcStruct, SparkOrcNewRecordReader}
import org.apache.orc.OrcConf.COMPRESS
import org.scalatest.BeforeAndAfterAll

import org.apache.spark.sql._
Expand Down Expand Up @@ -180,7 +181,7 @@ class OrcQuerySuite extends QueryTest with BeforeAndAfterAll with OrcTest {
// Respect `orc.compress`.
Copy link
Member

@viirya viirya Oct 16, 2017

Choose a reason for hiding this comment

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

Shall we change this comment too?

Copy link
Member

@viirya viirya Oct 16, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Oct 16, 2017

Choose a reason for hiding this comment

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

Sure. Thank you for review, @viirya .

withTempPath { file =>
spark.range(0, 10).write
.option("orc.compress", "ZLIB")
.option(COMPRESS.getAttribute, "ZLIB")
.orc(file.getCanonicalPath)
val expectedCompressionKind =
OrcFileOperator.getFileReader(file.getCanonicalPath).get.getCompression
Expand All @@ -191,7 +192,7 @@ class OrcQuerySuite extends QueryTest with BeforeAndAfterAll with OrcTest {
withTempPath { file =>
spark.range(0, 10).write
.option("compression", "ZLIB")
.option("orc.compress", "SNAPPY")
.option(COMPRESS.getAttribute, "SNAPPY")
.orc(file.getCanonicalPath)
val expectedCompressionKind =
OrcFileOperator.getFileReader(file.getCanonicalPath).get.getCompression
Expand Down
5 changes: 3 additions & 2 deletions sql/hive/src/test/scala/org/apache/spark/sql/hive/orc/OrcSourceSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Member

@viirya viirya Oct 16, 2017

Choose a reason for hiding this comment

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

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")

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Oct 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

@viirya viirya Oct 16, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Oct 16, 2017

Choose a reason for hiding this comment

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

If you want, no problem at all. :)

Copy link
Member

@viirya viirya Oct 16, 2017

Choose a reason for hiding this comment

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

Just like to be consistent. Thanks. :)

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Oct 16, 2017

Choose a reason for hiding this comment

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

Yep!

import org.apache.spark.sql.{QueryTest, Row}
Expand Down Expand Up @@ -205,8 +206,8 @@ abstract class OrcSuite extends QueryTest with TestHiveSingleton with BeforeAndA
// `compression` -> `orc.compression` -> `spark.sql.orc.compression.codec`
withSQLConf(SQLConf.ORC_COMPRESSION.key -> "uncompressed") {
assert(new OrcOptions(Map.empty[String, String], conf).compressionCodec == "NONE")
val map1 = Map("orc.compress" -> "zlib")
val map2 = Map("orc.compress" -> "zlib", "compression" -> "lzo")
val map1 = Map(COMPRESS.getAttribute -> "zlib")
val map2 = Map(COMPRESS.getAttribute -> "zlib", "compression" -> "lzo")
assert(new OrcOptions(map1, conf).compressionCodec == "ZLIB")
assert(new OrcOptions(map2, conf).compressionCodec == "LZO")
}
Expand Down