[SPARK-24073][SQL]: Rename DataReaderFactory to InputPartition.

[rdblue]

What changes were proposed in this pull request?

Renames:

• DataReaderFactory to InputPartition
• DataReader to InputPartitionReader
• createDataReaderFactories to planInputPartitions
• createUnsafeDataReaderFactories to planUnsafeInputPartitions
• createBatchDataReaderFactories to planBatchInputPartitions

This fixes the changes in SPARK-23219, which renamed ReadTask to
DataReaderFactory. The intent of that change was to make the read and
write API match (write side uses DataWriterFactory), but the underlying
problem is that the two classes are not equivalent.

ReadTask/DataReader function as Iterable/Iterator. One InputPartition is
a specific partition of the data to be read, in contrast to
DataWriterFactory where the same factory instance is used in all write
tasks. InputPartition's purpose is to manage the lifecycle of the
associated reader, which is now called InputPartitionReader, with an
explicit create operation to mirror the close operation. This was no
longer clear from the API because DataReaderFactory appeared to be more
generic than it is and it isn't clear why a set of them is produced for
a read.

How was this patch tested?

Existing tests, which have been updated to use the new name.

[SparkQA]

Test build #89799 has finished for PR 21145 at commit c364c05.

• This patch fails Scala style tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #89806 has finished for PR 21145 at commit cdf2b4d.

• This patch fails Scala style tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #89808 has finished for PR 21145 at commit cea3b86.

• This patch fails Scala style tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #89848 has finished for PR 21145 at commit 609ec14.

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

[dongjoon-hyun]

@rdblue . The re-naming of this PR seems to be partial. Could you replace KafkaMicroBatchDataReaderFactory with KafkaMicroBatchReadTask together in this PR? I guess there will be more occurrences like this.

[rdblue]

This fixes the API, not implementations, and it already touches 30+ files.

I'd rather not fix the downstream classes for two reasons. First, to avoid this becoming really large. Second, we need to be able to evolve these APIs without requiring changes to all implementations. I think we should avoid requiring changes to make everything consistent, or else there's tension between making necessary changes to the API and trying to move existing code to that API.

[dongjoon-hyun]

Yes. This kind of changes always become unnecessarily big. Since this PR turns the master branch into an inconsistent state, could you make a JIRA issue for the remaining tasks which this PR avoids? Then, someone else can help Apache Spark become more consistent later eventually in Apache Spark 2.4 (or 3.0) timeframe.

I think we should avoid requiring changes to make everything consistent

[rdblue]

Sure, good idea.

[arunmahadevan]

IMO, its better to keep it the current way.

DataReaderFactory implies that its something that produces DataReader which it does, whereas ReadTask is gives a notion that it does some reading whereas what it really does is createDataReader.

The current naming makes it consistent at the read/write side and I think the re-naming would add to the already confusing interfaces.

[rdblue]

@arunmahadevan, the problem is that the current naming is misleading. This is not a factory (it only produces one specific reader) and it does not have the same lifecycle as the write-side factory. Using parallel naming for the two implies an equivalence that doesn't exist.

[gatorsmile]

It sounds like both ReadTask and DataReaderFactory are not a good name here. Any better idea?

[rdblue]

I think ReadTask is fine. That name does not imply that you can use the object itself to read, but it does correctly show that it is one task in a larger operation. I think the name implies that it represents something to be read, which is correct, and it is reasonable to look at the API for that object to see how to read it. That can be clearly accomplished, so I don't think we need a different name.

[rdblue]

@cloud-fan and @henryr, do you have an opinion about naming here?

[henryr]

I don't mind ReadTask. It's imperfect because 'task' implies that this is a thing that can be executed, whereas this interface doesn't have a way to pass control to the task object. It's more like a description of a read task, but I think ReadTaskDescriptor would be a bit verbose.

I certainly agree that this is not a factory.

[cloud-fan]

I agree this is not a real factory, but it's not a real task either. I feel something like DataReaderHolder or ReadTaskDescriptor should be better. also cc @rxin @marmbrus

[gengliangwang]

Either names are not perfect.
It is not a real task, and it has a method name createDataReader, while there is createDataWriter in DataWriterFactory.
It is not a factory (design pattern). I did the renaming ReadTask -> DataReaderFactory to make read and write API consistent. It wasn't such misleading as expected, since the API in DataSourceReader is List<DataReaderFactory<Row>> createDataReaderFactories();.
Now I feel sorry that I didn't come up with a better naming at that time. But partially changing the naming to ReadTask now only makes things worse.
If there is a better name than both names, let's use it. Otherwise, I prefer DataReaderFactory to ReadTask.

[rdblue]

@gengliangwang, we can follow up with a rename for the streaming classes that already use this API. But there is no need to do that right now and make this commit larger.

I think I've already made it clear that I think DataReaderFactory is misleading and we should go with the originally proposed design, ReadTask.

[jose-torres]

I don't see the problem with the name ReadTask. In RDDs, we call the serializable representation of a partition for distribution to executors just Partition, and I've always found this pretty intuitive. Certainly it wouldn't be better to call it ComputeIteratorFactory instead.

[gatorsmile]

I still think ReadTask is confusing. I were asked by multiple people what is a ReadTask, especially when a Task is already clearly defined as a unit of execution in Spark. https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/scheduler/Task.scala#L31-L51

Let us avoid using Task here.

[henryr]

Ok - how about ReadTaskDescriptor? (In that case I think it would be ok to leave the method name as createReadTasks()).

[rdblue]

Hadoop uses the term "input split" for this. Would it be more clear if Spark adopted the same language?

[cloud-fan]

InputSplit seems a pretty good name for batch, since one split will be processed by one Spark task. How do streaming guys think about it?

[rdblue]

Now I'm rethinking the suggestion: InputSplit is a well-known Hadoop class that we probably shouldn't duplicate. What about using InputPartition instead? That makes it clear that the partitioning is on the input data and uses the more common term in Spark.

Is everyone okay with this? @jose-torres @gengliangwang @cloud-fan @henryr @arunmahadevan @gatorsmile?

[arunmahadevan]

InputPartition sounds fine, but is it ok to have a method like "createDataReader" inside it? Will create confusion when "inputPartition" is member of other classes like DataSourceRDDPartition?

It appears that the DataReaderFactory is kind of a wrapper for the Reader so that the Reader itself need not be serializable. I am also ok to leave it as is (though technically it may not be a factory).

[rdblue]

Okay, one more idea that captures the relationship between DataReader and ReadTask: What about using DataReadable? That's similar to the Iterable and Iterator relationship that the two classes have.

[cloud-fan]

So we want to expose 2 things in the naming:

1. it represents an input RDD partition
2. it creates DataReader

I think the first one really needs to be pointed out explicitly, while the second one is not that confusing to create a DataReader from a partition. So +1 on InputPartition.

[rdblue]

@gatorsmile, the Spark UI has used the term "task" for years to refer to the same thing. I don't think it is unreasonable to use the same term.

[SparkQA]

Test build #90334 has finished for PR 21145 at commit 250c1de.

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

[SparkQA]

Test build #90378 has finished for PR 21145 at commit 560ad6a.

• This patch fails Scala style tests.
• This patch merges cleanly.
• This patch adds no public classes.

[rdblue]

@cloud-fan, I've updated this PR to use InputPartition and similar names, since we seem to have consensus around it.

[SparkQA]

Test build #90380 has finished for PR 21145 at commit 3eff34b.

• This patch fails Scala style tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #90381 has finished for PR 21145 at commit ec53d12.

• This patch fails Scala style tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #90382 has finished for PR 21145 at commit 1423979.

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

[cloud-fan]

in the hadoop world there is InputFormat.getSplits, shall we follow and use getInputPartitions here?

[rdblue]

I think plan is a more accurate verb. To some Java people, get implies that the call is very cheap because it is associated with getters, which typically just return a field's value. Since that's not the case here and callers shouldn't consider this method cheap, I think it makes sense to use a different name that reflects what is actually happening: split planning.

[cloud-fan]

any other suggestions about naming? we are going to rename DataReaderFactory to InputPartition. cc @rxin @marmbrus @jose-torres

[gengliangwang]

InputPartition is good and straightforward 👍

[dongjoon-hyun]

Overall, +1 for the change.

[gatorsmile]

LGTM to InputPartition. CC @marmbrus @jose-torres to check the streaming side.

[jose-torres]

LGTM. I can own cleaning up the names of the streaming classes, probably wrapping that into the broader task of getting a design doc for the streaming reader API.

[rdblue]

Thanks @jose-torres! I appreciate not blocking this commit on those changes, since it would be difficult to keep this up to date from the other paths changing, while we discussed what to call these classes.

[gatorsmile]

Thanks! Merged to master.