[SPARK-18457][SQL] ORC and other columnar formats using HiveShim read all columns when doing a simple count #15898

aray · 2016-11-16T04:16:32Z

What changes were proposed in this pull request?

When reading zero columns (e.g., count(*)) from ORC or any other format that uses HiveShim, actually set the read column list to empty for Hive to use.

How was this patch tested?

Query correctness is handled by existing unit tests. I'm happy to add more if anyone can point out some case that is not covered.

Reduction in data read can be verified in the UI when built with a recent version of Hadoop say:

build/mvn -Pyarn -Phadoop-2.7 -Dhadoop.version=2.7.0 -Phive -DskipTests clean package

However the default Hadoop 2.2 that is used for unit tests does not report actual bytes read and instead just full file sizes (see FileScanRDD.scala line 80). Therefore I don't think there is a good way to add a unit test for this.

I tested with the following setup using above build options

case class OrcData(intField: Long, stringField: String)
spark.range(1,1000000).map(i => OrcData(i, s"part-$i")).toDF().write.format("orc").save("orc_test")

sql(
      s"""CREATE EXTERNAL TABLE orc_test(
         |  intField LONG,
         |  stringField STRING
         |)
         |STORED AS ORC
         |LOCATION '${System.getProperty("user.dir") + "/orc_test"}'
       """.stripMargin)

Results

query	Spark 2.0.2	this PR
`sql("select count(*) from orc_test").collect`	4.4 MB	199.4 KB
`sql("select intField from orc_test").collect`	743.4 KB	743.4 KB
`sql("select * from orc_test").collect`	4.4 MB	4.4 MB

…-no-col

rxin · 2016-11-16T04:55:31Z

cc @piaozhexiu I think you changed this last?

rxin · 2016-11-16T05:07:53Z

Can we add a unit test for this function?

HyukjinKwon · 2016-11-16T05:30:04Z

I was just watching the JIRA just for my curiosity. Actually, aren't we able to add test for SparkOrcNewRecordReader like the one as below (I tested this in OrcQuerySuite just for test purpose)?

test("Empty schema does not read data from ORC file") {
  val data = (1 to 2).zip(1 to 2)
  withOrcFile(data) { path =>
    val requrestedSchema = StructType(Nil)
    val conf = new Configuration()
    val physicalSchema = OrcFileOperator.readSchema(Seq(path), Some(conf)).get
    OrcRelation.setRequiredColumns(conf, physicalSchema, requrestedSchema)
    val maybeOrcReader = OrcFileOperator.getFileReader(path, Some(conf))
    assert(maybeOrcReader.isDefined)
    val orcRecordReader = new SparkOrcNewRecordReader(
      maybeOrcReader.get, conf, 0, maybeOrcReader.get.getContentLength)

    val recordsIterator = new RecordReaderIterator[OrcStruct](orcRecordReader)
    assert(recordsIterator.next().toString == "{null, null}")

    recordsIterator.close()
  }
}

SparkQA · 2016-11-16T06:01:00Z

Test build #68693 has finished for PR 15898 at commit 037ca1d.

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

aray · 2016-11-16T06:02:14Z

The code that is being changed originated 2 years ago with the addition of Hive 0.13 support by @zhzhan, see
7c89a8f#diff-77621c6c599cfdd24363967a500be1c5R109
Before that there was not even a null check.

I will add the test you are suggesting @HyukjinKwon

HyukjinKwon · 2016-11-16T06:27:51Z

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

+      val recordsIterator = new RecordReaderIterator[OrcStruct](orcRecordReader)
+      assert(recordsIterator.next().toString == "{null, null}")
+
+      recordsIterator.close()


Maybe, we should put some sugar on this something like try {..} finally {recordsIterator.close()} if this test looks reasonable. I just wanted to show that it might be able to be tested :).

tejasapatil · 2016-11-16T06:20:48Z

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

+  test("Empty schema does not read data from ORC file") {
+    val data = (1 to 2).zip(1 to 2)
+    withOrcFile(data) { path =>
+      val requrestedSchema = StructType(Nil)


typo in variable name

tejasapatil · 2016-11-16T06:21:49Z

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

  }
+
+  test("Empty schema does not read data from ORC file") {
+    val data = (1 to 2).zip(1 to 2)


feels weird to read. Would rather prefer simple Seq((1,1), (2,2))

tejasapatil · 2016-11-16T06:40:56Z

Feels like this will only help for select count(*) from table without any projections and filters.

Which version of ORC have you tested against ? In past I have seen PRs which do not work over older version of ORC and needed some fixes so it would be good to check.

aray · 2016-11-16T06:56:47Z

@tejasapatil yes that is the use case where this applies. It's only tested against whatever version is included in the hadoop2.7+hive build configuration listed above. Is there anything in particular you were concerned about compatibility wise?

SparkQA · 2016-11-16T08:08:37Z

Test build #68698 has finished for PR 15898 at commit d4e698c.

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

SparkQA · 2016-11-16T08:18:17Z

Test build #68701 has finished for PR 15898 at commit 8b5edd2.

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

SparkQA · 2016-11-16T08:29:39Z

Test build #68702 has finished for PR 15898 at commit a2fe576.

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

rxin · 2016-11-17T07:10:04Z

@tejasapatil does this lgtu?

rxin · 2016-11-18T05:25:34Z

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

+      try {
+        assert(recordsIterator.next().toString == "{null, null}")
+      } catch {
+        case e: Exception => fail(e)


why bother catching? the test case will fail anyway wouldn't it?

SparkQA · 2016-11-18T15:43:03Z

Test build #68853 has finished for PR 15898 at commit 022f32a.

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

rxin · 2016-11-18T19:18:21Z

Thanks - merging in master/branch-2.1.

… all columns when doing a simple count ## What changes were proposed in this pull request? When reading zero columns (e.g., count(*)) from ORC or any other format that uses HiveShim, actually set the read column list to empty for Hive to use. ## How was this patch tested? Query correctness is handled by existing unit tests. I'm happy to add more if anyone can point out some case that is not covered. Reduction in data read can be verified in the UI when built with a recent version of Hadoop say: ``` build/mvn -Pyarn -Phadoop-2.7 -Dhadoop.version=2.7.0 -Phive -DskipTests clean package ``` However the default Hadoop 2.2 that is used for unit tests does not report actual bytes read and instead just full file sizes (see FileScanRDD.scala line 80). Therefore I don't think there is a good way to add a unit test for this. I tested with the following setup using above build options ``` case class OrcData(intField: Long, stringField: String) spark.range(1,1000000).map(i => OrcData(i, s"part-$i")).toDF().write.format("orc").save("orc_test") sql( s"""CREATE EXTERNAL TABLE orc_test( | intField LONG, | stringField STRING |) |STORED AS ORC |LOCATION '${System.getProperty("user.dir") + "/orc_test"}' """.stripMargin) ``` ## Results query | Spark 2.0.2 | this PR ---|---|--- `sql("select count(*) from orc_test").collect`|4.4 MB|199.4 KB `sql("select intField from orc_test").collect`|743.4 KB|743.4 KB `sql("select * from orc_test").collect`|4.4 MB|4.4 MB Author: Andrew Ray <ray.andrew@gmail.com> Closes #15898 from aray/sql-orc-no-col. (cherry picked from commit 795e9fc) Signed-off-by: Reynold Xin <rxin@databricks.com>

… all columns when doing a simple count ## What changes were proposed in this pull request? When reading zero columns (e.g., count(*)) from ORC or any other format that uses HiveShim, actually set the read column list to empty for Hive to use. ## How was this patch tested? Query correctness is handled by existing unit tests. I'm happy to add more if anyone can point out some case that is not covered. Reduction in data read can be verified in the UI when built with a recent version of Hadoop say: ``` build/mvn -Pyarn -Phadoop-2.7 -Dhadoop.version=2.7.0 -Phive -DskipTests clean package ``` However the default Hadoop 2.2 that is used for unit tests does not report actual bytes read and instead just full file sizes (see FileScanRDD.scala line 80). Therefore I don't think there is a good way to add a unit test for this. I tested with the following setup using above build options ``` case class OrcData(intField: Long, stringField: String) spark.range(1,1000000).map(i => OrcData(i, s"part-$i")).toDF().write.format("orc").save("orc_test") sql( s"""CREATE EXTERNAL TABLE orc_test( | intField LONG, | stringField STRING |) |STORED AS ORC |LOCATION '${System.getProperty("user.dir") + "/orc_test"}' """.stripMargin) ``` ## Results query | Spark 2.0.2 | this PR ---|---|--- `sql("select count(*) from orc_test").collect`|4.4 MB|199.4 KB `sql("select intField from orc_test").collect`|743.4 KB|743.4 KB `sql("select * from orc_test").collect`|4.4 MB|4.4 MB Author: Andrew Ray <ray.andrew@gmail.com> Closes apache#15898 from aray/sql-orc-no-col.

aray added 3 commits March 29, 2016 05:33

meh

d6dbd47

Merge branch 'master' of https://github.com/apache/spark into sql-orc…

a4d1ce3

…-no-col

update comment to be consistent with logic

037ca1d

add unit test suggested by Hyukjin Kwon

d4e698c

HyukjinKwon reviewed Nov 16, 2016

View reviewed changes

address code review

8b5edd2

tejasapatil reviewed Nov 16, 2016

View reviewed changes

address @tejasapatil code review

a2fe576

rxin reviewed Nov 18, 2016

View reviewed changes

remove catch in unit test

022f32a

asfgit closed this in 795e9fc Nov 18, 2016

dongjoon-hyun mentioned this pull request Oct 18, 2017

[SPARK-22300][BUILD] Update ORC to 1.4.1 #19521

Closed

aray mentioned this pull request Feb 1, 2020

Zero column projection not handled correctly GoogleCloudDataproc/spark-bigquery-connector#73

Closed

[SPARK-18457][SQL] ORC and other columnar formats using HiveShim read all columns when doing a simple count #15898

[SPARK-18457][SQL] ORC and other columnar formats using HiveShim read all columns when doing a simple count #15898

Uh oh!

Conversation

aray commented Nov 16, 2016

What changes were proposed in this pull request?

How was this patch tested?

Results

Uh oh!

rxin commented Nov 16, 2016

Uh oh!

rxin commented Nov 16, 2016

Uh oh!

HyukjinKwon commented Nov 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Nov 16, 2016

Uh oh!

aray commented Nov 16, 2016

Uh oh!

HyukjinKwon Nov 16, 2016 • 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!

tejasapatil Nov 16, 2016

Choose a reason for hiding this comment

Uh oh!

tejasapatil Nov 16, 2016

Choose a reason for hiding this comment

Uh oh!

tejasapatil commented Nov 16, 2016

Uh oh!

aray commented Nov 16, 2016

Uh oh!

SparkQA commented Nov 16, 2016

Uh oh!

SparkQA commented Nov 16, 2016

Uh oh!

SparkQA commented Nov 16, 2016

Uh oh!

rxin commented Nov 17, 2016

Uh oh!

rxin Nov 18, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 18, 2016

Uh oh!

rxin commented Nov 18, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

HyukjinKwon commented Nov 16, 2016 •

edited

Loading

HyukjinKwon Nov 16, 2016 •

edited

Loading