[SPARK-17974] Refactor FileCatalog classes to simplify the inheritance tree #15518

ericl · 2016-10-17T19:15:35Z

What changes were proposed in this pull request?

This renames BasicFileCatalog => FileCatalog, combines SessionFileCatalog with PartitioningAwareFileCatalog, and removes the old FileCatalog trait.

In summary,

MetadataLogFileCatalog extends PartitioningAwareFileCatalog
ListingFileCatalog extends PartitioningAwareFileCatalog
PartitioningAwareFileCatalog extends FileCatalog
TableFileCatalog extends FileCatalog

cc @cloud-fan @mallman

How was this patch tested?

Existing tests

rxin · 2016-10-17T21:26:02Z

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

+   * single root path from which partitions are discovered, or individual partitions may be
+   * specified by each path.
+   */
+  def rootPaths: Seq[Path]


what's "root" about this?

I would say "pretty much nothing" anymore.

In an earlier version, it was the "root" path of the table, excluding any partition dirs. The PR drifted away from that definition.

Now I'd say it could be reverted to paths.

I think they are the roots of the recursive traversal to discover leaf files. I am inclined to keep rootPaths since it's easy to grep for, vs paths which is too common.

SparkQA · 2016-10-17T21:28:26Z

Test build #67084 has finished for PR 15518 at commit 5d390c9.

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

rxin · 2016-10-17T21:31:45Z

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

+ * A collection of data files from a partitioned relation, along with the partition values in the
+ * form of an [[InternalRow]].
+ */
+case class Partition(values: InternalRow, files: Seq[FileStatus])


while you are doing it, perhaps we can rename this TaskPartition?

I always find "Partition" very confusing because it can mean 3 different things:

A block in HDFS.

A Hive partition.

A Spark task partition (or split).

I don't think these are the partitions tasks see. Confusingly, those seem to be named FilePartition. How about PartitionFiles?

mallman · 2016-10-17T21:45:17Z

...cala/org/apache/spark/sql/execution/datasources/parquet/ParquetPartitionDiscoverySuite.scala

-          assert(location.partitionSpec === PartitionSpec.emptySpec)
+        case LogicalRelation(
+            HadoopFsRelation(location: PartitioningAwareFileCatalog, _, _, _, _, _), _, _) =>
+          assert(location.partitionSpec() === PartitionSpec.emptySpec)
      }.getOrElse {
        fail(s"Expecting a ParquetRelation2, but got:\n$queryExecution")


We're not expecting a ParquetRelation2 anymore—more like a HadoopFsRelation with a PartitioningAwareFileCatalog.

ericl · 2016-10-17T23:22:55Z

Alright renamed PartitionFiles => PartitionDirectory, and PartitionDirectory => PartitionPath.

SparkQA · 2016-10-17T23:29:09Z

Test build #67095 has finished for PR 15518 at commit 0d0954d.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class PartitionDirectory(values: InternalRow, files: Seq[FileStatus])
- case class PartitionPath(values: InternalRow, path: Path)

SparkQA · 2016-10-18T00:33:42Z

Test build #67092 has finished for PR 15518 at commit 43c4374.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class PartitionFiles(values: InternalRow, files: Seq[FileStatus])

rxin · 2016-10-18T03:59:57Z

LGTM - merging in master.

rxin · 2016-10-18T04:26:07Z

Oops - just realized the tests for the latest commit failed. I will revert the patch.

yhuai · 2016-10-18T04:26:08Z

Seems this fails the scala style check.

…eritance tree ## What changes were proposed in this pull request? This renames `BasicFileCatalog => FileCatalog`, combines `SessionFileCatalog` with `PartitioningAwareFileCatalog`, and removes the old `FileCatalog` trait. In summary, ``` MetadataLogFileCatalog extends PartitioningAwareFileCatalog ListingFileCatalog extends PartitioningAwareFileCatalog PartitioningAwareFileCatalog extends FileCatalog TableFileCatalog extends FileCatalog ``` (note that this is a re-submission of apache#15518 which got reverted) ## How was this patch tested? Existing tests Author: Eric Liang <ekl@databricks.com> Closes apache#15533 from ericl/fix-scalastyle-revert.

…e tree ## What changes were proposed in this pull request? This renames `BasicFileCatalog => FileCatalog`, combines `SessionFileCatalog` with `PartitioningAwareFileCatalog`, and removes the old `FileCatalog` trait. In summary, ``` MetadataLogFileCatalog extends PartitioningAwareFileCatalog ListingFileCatalog extends PartitioningAwareFileCatalog PartitioningAwareFileCatalog extends FileCatalog TableFileCatalog extends FileCatalog ``` cc cloud-fan mallman ## How was this patch tested? Existing tests Author: Eric Liang <ekl@databricks.com> Closes apache#15518 from ericl/refactor-session-file-catalog.

…eritance tree ## What changes were proposed in this pull request? This renames `BasicFileCatalog => FileCatalog`, combines `SessionFileCatalog` with `PartitioningAwareFileCatalog`, and removes the old `FileCatalog` trait. In summary, ``` MetadataLogFileCatalog extends PartitioningAwareFileCatalog ListingFileCatalog extends PartitioningAwareFileCatalog PartitioningAwareFileCatalog extends FileCatalog TableFileCatalog extends FileCatalog ``` (note that this is a re-submission of apache#15518 which got reverted) ## How was this patch tested? Existing tests Author: Eric Liang <ekl@databricks.com> Closes apache#15533 from ericl/fix-scalastyle-revert.

…e tree ## What changes were proposed in this pull request? This renames `BasicFileCatalog => FileCatalog`, combines `SessionFileCatalog` with `PartitioningAwareFileCatalog`, and removes the old `FileCatalog` trait. In summary, ``` MetadataLogFileCatalog extends PartitioningAwareFileCatalog ListingFileCatalog extends PartitioningAwareFileCatalog PartitioningAwareFileCatalog extends FileCatalog TableFileCatalog extends FileCatalog ``` cc cloud-fan mallman ## How was this patch tested? Existing tests Author: Eric Liang <ekl@databricks.com> Closes apache#15518 from ericl/refactor-session-file-catalog.

…eritance tree ## What changes were proposed in this pull request? This renames `BasicFileCatalog => FileCatalog`, combines `SessionFileCatalog` with `PartitioningAwareFileCatalog`, and removes the old `FileCatalog` trait. In summary, ``` MetadataLogFileCatalog extends PartitioningAwareFileCatalog ListingFileCatalog extends PartitioningAwareFileCatalog PartitioningAwareFileCatalog extends FileCatalog TableFileCatalog extends FileCatalog ``` (note that this is a re-submission of apache#15518 which got reverted) ## How was this patch tested? Existing tests Author: Eric Liang <ekl@databricks.com> Closes apache#15533 from ericl/fix-scalastyle-revert.

ericl added 4 commits October 17, 2016 11:30

move files

798a882

Mon Oct 17 11:31:42 PDT 2016

db8a669

clean up classes

e2d8c1d

merge suites

5d390c9

rxin reviewed Oct 17, 2016

View reviewed changes

mallman reviewed Oct 17, 2016

View reviewed changes

ericl added 2 commits October 17, 2016 15:21

Mon Oct 17 15:21:53 PDT 2016

43c4374

rename to partdirectory

0d0954d

asfgit closed this in 8daa1a2 Oct 18, 2016

ericl mentioned this pull request Oct 18, 2016

[SPARK-17974] (try 2) Refactor FileCatalog classes to simplify the inheritance tree #15533

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-17974] Refactor FileCatalog classes to simplify the inheritance tree #15518

[SPARK-17974] Refactor FileCatalog classes to simplify the inheritance tree #15518

ericl commented Oct 17, 2016

rxin Oct 17, 2016

mallman Oct 17, 2016

ericl Oct 17, 2016

SparkQA commented Oct 17, 2016

rxin Oct 17, 2016 •

edited

Loading

ericl Oct 17, 2016

mallman Oct 17, 2016

ericl Oct 17, 2016

ericl commented Oct 17, 2016

SparkQA commented Oct 17, 2016

SparkQA commented Oct 18, 2016

rxin commented Oct 18, 2016

rxin commented Oct 18, 2016

yhuai commented Oct 18, 2016

[SPARK-17974] Refactor FileCatalog classes to simplify the inheritance tree #15518

[SPARK-17974] Refactor FileCatalog classes to simplify the inheritance tree #15518

Conversation

ericl commented Oct 17, 2016

What changes were proposed in this pull request?

How was this patch tested?

rxin Oct 17, 2016

Choose a reason for hiding this comment

mallman Oct 17, 2016

Choose a reason for hiding this comment

ericl Oct 17, 2016

Choose a reason for hiding this comment

SparkQA commented Oct 17, 2016

rxin Oct 17, 2016 • edited Loading

Choose a reason for hiding this comment

ericl Oct 17, 2016

Choose a reason for hiding this comment

mallman Oct 17, 2016

Choose a reason for hiding this comment

ericl Oct 17, 2016

Choose a reason for hiding this comment

ericl commented Oct 17, 2016

SparkQA commented Oct 17, 2016

SparkQA commented Oct 18, 2016

rxin commented Oct 18, 2016

rxin commented Oct 18, 2016

yhuai commented Oct 18, 2016

rxin Oct 17, 2016 •

edited

Loading