[SPARK-7968][CORE] Rename minPartitions to maxPartitions in wholeTextFiles/binaryFiles

[watermen]

The actual number of partition(task) in wholeTextFiles/binaryFiles is less than or equal to minPartitions, so maxPartitions is better than minPartitions.

[marmbrus]

This does not appear to be a SQL patch.

[andrewor14]

it's strange that this still defaults to defaultMinPartitions. Does that need to be fixed as well?

[watermen]

Because i don‘t know what is the best defalut value, one will be ok?

[andrewor14]

ok to test. @srowen does this seem valid to you?

[andrewor14]

Also, it seems that here we're changing parameter names of public APIs. This is not backward compatible right?

[SparkQA]

Test build #35172 has finished for PR 6518 at commit cd64289.

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

[watermen]

@andrewor14 You mean the code like wholeTextFiles("path", minPartitions = 1)? Others will not have the problem of backward compatible.

[andrewor14]

Yes, but that by itself maybe a reason why we can't merge this patch.

[watermen]

@andrewor14 Can we add new method newWholeTextFiles(path: String, maxPartitions: Int)? and

wholeTextFiles(path: String, minPartitions: Int = defaultMinPartitions) {
  newWholeTextFiles(path, minPartitions)
}

I want to rename strongly because minPartitions will mislead users.

[srowen]

I don't understand this. The value minPartitions is passed to InputFormat.setMinPartitions. Why do you think it's a max?

[watermen]

@srowen wholeTextFiles used to combine small files, and the actual number of partition(task) is less than or equal to partitionsNum user passed.

[srowen]

The number of partitions is generally equal to the number of files. I don't think it can be less; it can be more. Really, the minPartitions setting rarely does anything; it's just a suggestion anyway. It might cause Hadoop to return multiple splits per file (which would be bad here actually). But it is definitely not a maximum and you can see this is incorrect, as it's passed to setMinPartitions.

I suppose I'd argue that this arg should go away entirely as it seems like it can only hurt. At this point though it exists, and its name and doc is correct relative to what it does.

[andrewor14]

@watermen I think the decision then is to not merge this patch. Whether or not the intended change is correct (and it is likely not, according to @srowen), this breaks public API backward compatibility. Would you mind closing this PR then? Thanks for your work.