[SPARK-21056][SQL] Use at most one spark job to list files in InMemoryFileIndex #18269

bbossy · 2017-06-11T11:56:12Z

What changes were proposed in this pull request?

This PR changes InMemoryFileIndex.listLeafFiles behaviour to launch at most one spark job to list leaf files.

##JIRA
https://issues.apache.org/jira/browse/SPARK-21056

Given partitioned file relation (e.g. parquet):
root/a=../b=../c=..
InMemoryFileIndex.listLeafFiles runs numberOfPartitions(a) times numberOfPartitions(b) spark jobs sequentially to list leaf files, if both numberOfPartitions(a) and numberOfPartitions(b) are below spark.sql.sources.parallelPartitionDiscovery.threshold and numberOfPartitions(c) is above spark.sql.sources.parallelPartitionDiscovery.threshold

Since the jobs are run sequentially, the overhead of the jobs dominates and the file listing operation can become significantly slower than listing the files from the driver.
I propose that InMemoryFileIndex.listLeafFiles should launch at most one spark job for listing leaf files.

How was this patch tested?

Adapted existing tests to match expected behaviour.

AmplabJenkins · 2017-06-11T11:57:50Z

Can one of the admins verify this patch?

bbossy · 2017-06-11T12:04:53Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/InMemoryFileIndex.scala

-        Array.empty[FileStatus]
+    val statuses = paths.flatMap { path =>
+      try {
+        val fs = path.getFileSystem(hadoopConf)


Would it be safe to use the same instance of fs for all the paths in a InMemoryFileIndex? If this is the case, I can move this back to where it was before.

Yes, it's the same instance and should be reused

Thanks! Fixed.

gatorsmile · 2017-06-12T07:05:03Z

Could you please update the PR description by copying the contents from the JIRA?

Any performance number you can share?

bbossy · 2017-06-12T12:22:40Z

@gatorsmile :

I ran a synthetic scenario to show what changes, since deploying this branch would be more involved.

I created two very simple relations on a small HDFS cluster (4 datanodes). Running spark with master local[16]. The machine has 16 physical cores plus 16 hyper threaded. The namenode is on a remote machine in the same network.

Setup:

scala> case class Foo(a: Int, b: Int, c: Int, d: Int)
defined class Foo

// manypartitions: 4 times 4 times 100 partitions. Parallel listing kicks in listing level 'c' both without and with this PR
scala> val data = for {
     | a <- 1.to(4)
     | b <- 1.to(4)
     | c <- 1.to(100)
     | } yield Foo(a,b,c,100)
data: scala.collection.immutable.IndexedSeq[Foo] = Vector(Foo(1,1,1,100), ...

scala> data.toDS.write.partitionBy("a", "b", "c").parquet("hdfs://namenode/user/bbossy/manypartitions")

// morepartitions: 10 times 10 times 100 partitions. Before this PR, 100 parallel listing jobs are spawned to list c, since each b contains more directories than the threshold. 
// With this PR, one parallel listing job is spawned to list all b partitions, since at level b, there are more paths to list than the threshold.
scala> val data = for {
     | a <- 1.to(10)
     | b <- 1.to(10)
     | c <- 1.to(100)
     | } yield Foo(a,b,c,1000)
data: scala.collection.immutable.IndexedSeq[Foo] = Vector(Foo(1,1,1,1000), ...
scala> data.toDS.write.partitionBy("a", "b", "c").parquet("hdfs://namenode/user/bbossy/morepartitions")

Using master branch before my commits:

scala> :pa
// Entering paste mode (ctrl-D to finish)

def time[R](block: => R): R = {
  val t0 = System.currentTimeMillis()
  val result = block
  println("Elapsed time: " + (System.currentTimeMillis - t0) + "ms")
  result
}

// Exiting paste mode, now interpreting.

time: [R](block: => R)R

scala> time(spark.read.parquet("hdfs://namenode/user/bbossy/manypartitions").collect)
Elapsed time: 6506ms
res2: Array[org.apache.spark.sql.Row] = Array([100,3,2,4], ...
scala> time(spark.read.parquet("hdfs://namenode/user/bbossy/manypartitions").collect)
Elapsed time: 2905ms
res3: Array[org.apache.spark.sql.Row] = Array([100,3,2,4], ...
scala> time(spark.read.parquet("hdfs://namenode/user/bbossy/manypartitions").collect)
Elapsed time: 2744ms
res4: Array[org.apache.spark.sql.Row] = Array([100,3,2,4], ...
scala> time(spark.read.parquet("hdfs://namenode/user/bbossy/manypartitions").collect)
Elapsed time: 2683ms
res5: Array[org.apache.spark.sql.Row] = Array([100,3,2,4], ...


scala> time(spark.read.parquet("hdfs://namenode/user/bbossy/morepartitions").collect)
Elapsed time: 16068ms
res6: Array[org.apache.spark.sql.Row] = Array([1000,6,8,31], ...
scala> time(spark.read.parquet("hdfs://namenode/user/bbossy/morepartitions").collect)
Elapsed time: 16047ms
res7: Array[org.apache.spark.sql.Row] = Array([1000,6,8,31], ...
scala> time(spark.read.parquet("hdfs://namenode/user/bbossy/morepartitions").collect)
Elapsed time: 15691ms
res8: Array[org.apache.spark.sql.Row] = Array([1000,6,8,31], ...
scala> time(spark.read.parquet("hdfs://namenode/user/bbossy/morepartitions").collect)
Elapsed time: 15767ms
res9: Array[org.apache.spark.sql.Row] = Array([1000,6,8,31], ...
scala>

UI timeline:

Using this PR:

// omitting def time...

scala> time(spark.read.parquet("hdfs://namenode/user/bbossy/manypartitions").collect)
Elapsed time: 6790ms
res0: Array[org.apache.spark.sql.Row] = Array([100,3,2,4], ...
scala> time(spark.read.parquet("hdfs://namenode/user/bbossy/manypartitions").collect)
Elapsed time: 4481ms
res1: Array[org.apache.spark.sql.Row] = Array([100,3,2,4], ...
scala> time(spark.read.parquet("hdfs://namenode/user/bbossy/manypartitions").collect)
Elapsed time: 4465ms
res2: Array[org.apache.spark.sql.Row] = Array([100,3,2,4], ...
scala> time(spark.read.parquet("hdfs://namenode/user/bbossy/manypartitions").collect)
Elapsed time: 4103ms
res3: Array[org.apache.spark.sql.Row] = Array([100,3,2,4], ...


scala> time(spark.read.parquet("hdfs://namenode/user/bbossy/morepartitions").collect)
Elapsed time: 4717ms
res4: Array[org.apache.spark.sql.Row] = Array([1000,6,8,31], ...
scala> time(spark.read.parquet("hdfs://namenode/user/bbossy/morepartitions").collect)
Elapsed time: 4434ms
res5: Array[org.apache.spark.sql.Row] = Array([1000,6,8,31], ...
scala> time(spark.read.parquet("hdfs://namenode/user/bbossy/morepartitions").collect)
Elapsed time: 5219ms
res6: Array[org.apache.spark.sql.Row] = Array([1000,6,8,31], ...
scala> time(spark.read.parquet("hdfs://namenode/user/bbossy/morepartitions").collect)
Elapsed time: 4429ms
res7: Array[org.apache.spark.sql.Row] = Array([1000,6,8,31], ...
scala>

UI timeline:

Is there something more specific that I should look into?

bbossy · 2017-06-13T08:25:43Z

ping @gatorsmile @srowen and possibly @cloud-fan : Would like to hear your thoughts on this.

cloud-fan · 2017-06-14T02:46:29Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/InMemoryFileIndex.scala

-    val fs = path.getFileSystem(hadoopConf)
+      sessionOpt: Option[SparkSession]): Seq[(Path, Seq[FileStatus])] = {
+    logTrace(s"Listing ${paths.mkString(", ")}")
+    val fs = paths.headOption.map(_.getFileSystem(hadoopConf))


how about

if (paths.isEmpty) { Nil } else { val fs = paths.head.getFileSystem(hadoopConf) ...... }

cloud-fan · 2017-06-14T02:48:09Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/InMemoryFileIndex.scala

    }

-    val filteredStatuses = statuses.filterNot(status => shouldFilterOut(status.getPath.getName))
+    val filteredStatuses = statuses.flatMap { case (path, fStatuses) =>


can we merge these flatMaps?

something like

paths.flatMap { path => try { val status = fs.get.listStatus(path) val filteredStatuses = statuses.filterNot(status => shouldFilterOut(status.getPath.getName)) .... } catch ... }

then we can still keep the previous code structure

kiszk · 2017-06-15T13:15:34Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/InMemoryFileIndex.scala

-    val filteredStatuses = statuses.filterNot(status => shouldFilterOut(status.getPath.getName))
+    val filteredStatuses = statuses.flatMap { case (path, fStatuses) =>
+      val filtered = fStatuses.filterNot(status => shouldFilterOut(status.getPath.getName))
+        if (filtered.isEmpty) {


nit: style issue. this if-then-else should be moved to left with 2 spaces

mallman · 2017-06-16T17:00:44Z

@bbossy I've built and deployed a branch of Spark 2.2 with your patch and compared its behavior to the same branch of Spark 2.2 without your patch. I'm seeing different behavior, but not what I expected.

My test table has three partition columns, ds, h and chunk. There are 1 ds values, 2 h values, and 51 chunk values, split into 27 and 24 partitions under the two h directories. I set spark.sql.sources.parallelPartitionDiscovery.threshold to 10. I believe this fits the scenario you're trying to remedy.

I use spark.read.parquet to load the table. When I load the table using the unpatched branch, Spark launches three jobs with 27, 24 and 1 stages, in that order. When I load the table using the patched branch, Spark launches three jobs with 51, 1 and 51 stages, in that order. Does this match your expectations? I was expecting to see Spark launch two jobs.

…ndex

bbossy · 2017-06-18T14:07:43Z

@mallman I'm not sure where this difference in behaviour is coming from. The following test in FileIndexSuite passes:

 test("mallman's scenario") {
    withSQLConf(SQLConf.PARALLEL_PARTITION_DISCOVERY_THRESHOLD.key -> "10") {
      withTempDir { dir =>
        for (chunk <- 1.to(27)) {
          val f = new File(dir, s"ds=1/h=one/chunk=$chunk/file.parquet")
          f.getParentFile.mkdirs()
          f.createNewFile()
        }
        for (chunk <- 1.to(24)) {
          val f = new File(dir, s"ds=1/h=two/chunk=$chunk/file.parquet")
          f.getParentFile.mkdirs()
          f.createNewFile()
        }
        HiveCatalogMetrics.reset()
        assert(HiveCatalogMetrics.METRIC_PARALLEL_LISTING_JOB_COUNT.getCount() == 0)
        new InMemoryFileIndex(spark, Seq(new Path(dir.getCanonicalPath)), Map.empty, None)
        assert(HiveCatalogMetrics.METRIC_PARALLEL_LISTING_JOB_COUNT.getCount() == 1)
      }
    }
  }

Does it match your scenario?

I'll dig around a bit later to see if I can come up with an explanation.

bbossy · 2017-06-18T14:08:24Z

@cloud-fan Could you take another look, please?

cloud-fan · 2017-06-19T05:10:38Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/InMemoryFileIndex.scala

+        try {
+          val fStatuses = fs.listStatus(path)
+          val filtered = fStatuses.filterNot(status => shouldFilterOut(status.getPath.getName))
+          if (filtered.nonEmpty) {


nit: filtered.map(path -> _), so that we don't need the if-else here, and the flatMap there

cloud-fan · 2017-06-19T05:13:37Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/InMemoryFileIndex.scala

+          fStatuses.map { f => path -> f }
+        }.partition { case (_, fStatus) => fStatus.isDirectory }
+        val pathsToList = dirs.map { case (_, fStatus) => fStatus.getPath }
+        val nestedFiles = if (pathsToList.nonEmpty) {


do we need this if check?

cloud-fan · 2017-06-19T05:19:58Z

let's wait @mallman 's response to make sure this patch does fix the problem

mallman · 2017-06-21T17:21:42Z

Hi @bbossy

Does it match your scenario?

It does not match my scenario. I'm reading files from HDFS. In your test, you're reading files from the local filesystem. Can you try a test using files stored in HDFS?

Also, I'm not testing with InMemoryFileIndex directly. Rather, I'm using spark.read.parquet. Can you try a test with that scenario?

HyukjinKwon · 2017-07-24T03:06:59Z

gentle ping @bbossy, I just want to be sure if it is in progress in any way.

bbossy · 2017-07-24T15:17:40Z

@HyukjinKwon I'll see that I can address the outstanding review comments in the next day or two.

bbossy commented Jun 11, 2017

View reviewed changes

cloud-fan reviewed Jun 14, 2017

View reviewed changes

kiszk reviewed Jun 15, 2017

View reviewed changes

bbossy added 3 commits June 18, 2017 13:56

SPARK-21056: use at most one spark job to list files in InMemoryFileI…

68155cc

…ndex

SPARK-21056: Use the same instance of FileSystem

2150f1c

SPARK-21056: Reorg code

579c567

bbossy force-pushed the SPARK-21056 branch from 97b2251 to 579c567 Compare June 18, 2017 12:00

bbossy added 2 commits June 18, 2017 14:03

SPARK-21056: Bail out early if paths is empty

cb21cf7

SPARK-21056: Fix serialization issue in bulkListLeafFiles

c4fba40

cloud-fan reviewed Jun 19, 2017

View reviewed changes

HyukjinKwon mentioned this pull request Jul 31, 2017

[INFRA] Close stale PRs #18780

Closed

asfgit closed this in 3a45c7f Aug 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-21056][SQL] Use at most one spark job to list files in InMemoryFileIndex #18269

[SPARK-21056][SQL] Use at most one spark job to list files in InMemoryFileIndex #18269

bbossy commented Jun 11, 2017 •

edited

Loading

AmplabJenkins commented Jun 11, 2017

bbossy Jun 11, 2017

srowen Jun 11, 2017

bbossy Jun 12, 2017

gatorsmile commented Jun 12, 2017

bbossy commented Jun 12, 2017

bbossy commented Jun 13, 2017

cloud-fan Jun 14, 2017

cloud-fan Jun 14, 2017

cloud-fan Jun 14, 2017

cloud-fan Jun 14, 2017

kiszk Jun 15, 2017 •

edited

Loading

mallman commented Jun 16, 2017

bbossy commented Jun 18, 2017

bbossy commented Jun 18, 2017

cloud-fan Jun 19, 2017 •

edited

Loading

cloud-fan Jun 19, 2017

cloud-fan commented Jun 19, 2017

mallman commented Jun 21, 2017

HyukjinKwon commented Jul 24, 2017

bbossy commented Jul 24, 2017

[SPARK-21056][SQL] Use at most one spark job to list files in InMemoryFileIndex #18269

[SPARK-21056][SQL] Use at most one spark job to list files in InMemoryFileIndex #18269

Conversation

bbossy commented Jun 11, 2017 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

AmplabJenkins commented Jun 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gatorsmile commented Jun 12, 2017

bbossy commented Jun 12, 2017

Setup:

Using master branch before my commits:

Using this PR:

bbossy commented Jun 13, 2017

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

kiszk Jun 15, 2017 • edited Loading

Choose a reason for hiding this comment

mallman commented Jun 16, 2017

bbossy commented Jun 18, 2017

bbossy commented Jun 18, 2017

cloud-fan Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Jun 19, 2017

mallman commented Jun 21, 2017

HyukjinKwon commented Jul 24, 2017

bbossy commented Jul 24, 2017

bbossy commented Jun 11, 2017 •

edited

Loading

kiszk Jun 15, 2017 •

edited

Loading

cloud-fan Jun 19, 2017 •

edited

Loading