[SPARK-7277][SQL] Throw exception if the property mapred.reduce.tasks is set to -1

[viirya]

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

As automatically determining the number of reducers is not supported (mapred.reduce.tasks is set to -1), we should throw exception to users.

[SparkQA]

Test build #31436 has finished for PR 5811 at commit 68a1c70.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • "public class " + className + extendsText + " implements java.io.Serializable
• This patch does not change any dependencies.

[squito]

typo: "Setting this property ..."

[squito]

actually can I suggest a small rewording a think is a little clearer?

Setting this property to -1 for automatically determing the number of reducers is not supported. Ignoring request, and showing ${SQLConf.SHUFFLE_PARTITIONS} instead

[squito]

could you also add a test that things work when you try to set it to -1? from the jira it seems that was a major goal here, to prevent incorrect results.

[squito]

though I wonder if instead it should fail (with a good error) if you give a bad value, rather than just logging a message and ignoring the request. any thoughts, @marmbrus?

[marmbrus]

Warnings seem likely to be missed. I would throw an exception.

[viirya]

Thanks. Updated to throw exception now. Also added a test for it.

[SparkQA]

Test build #31504 has finished for PR 5811 at commit 4ede705.

• This patch fails MiMa tests.
• This patch merges cleanly.
• This patch adds no public classes.
• This patch does not change any dependencies.

[viirya]

should not be a related failure.

[viirya]

test again please.

[squito]

Jenkins, retest this please

[SparkQA]

Test build #31552 has finished for PR 5811 at commit 4ede705.

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

[squito]

lgtm

[TanninOne]

Has anyone verified that a string comparison against -1 completely fully fixes the issue? Isn't it interpreted as an integer later?
What I'm getting at is: What if the user has the variable to "-01". or to "-2"? Maybe those were valid values for mapred.reduce.tasks as well? How was zero handled in hadoop?
Wouldn't it be safer and cleaner to test any variable against a valid range?
I.e. "if (Integer.parseInt("value") < 1) ..."

[viirya]

Setting mapred.reduce.tasks to '-1' has special meaning that asks Hive to automatically determine the number of reducers. It is the default value in hive-default.xml. You can also refer to HADOOPNUMREDUCERS.

Since it will parse the given string value, the configuration value of "-01" or "-2" should be able to be parsed to integer value too. As I can tell from the Hive sources, it checks if mapred.reduce.tasks is greater than 0 before using it in SemanticAnalyzer and SetReducerParallelism. So I think these values "-01", "-2" would have the same effect as setting it as '-1'.

Although I don't think it will be possible to have such values as setting values for mapred.reduce.tasks, it is still good to consider these cases. I will update it later.

[TanninOne]

Great, thanks for looking into this.

[SparkQA]

Test build #31754 has finished for PR 5811 at commit e518f96.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • trait LDAOptimizer
  • class EMLDAOptimizer extends LDAOptimizer
  • class OnlineLDAOptimizer extends LDAOptimizer

[marmbrus]

Thanks, merged to master and 1.4