[SPARK-15382][SQL] Fix a bug in sampling with replacement

[maropu]

What changes were proposed in this pull request?

This pr to fix a bug below in sampling with replacement

val df = Seq((1, 0), (2, 0), (3, 0)).toDF("a", "b")
df.sample(true, 2.0).withColumn("c", monotonically_increasing_id).select($"c").show
+---+
|  c|
+---+
|  0|
|  1|
|  1|
|  1|
|  2|
+---+

How was this patch tested?

Added a test in DataFrameSuite.

[maropu]

@HyukjinKwon @rxin could you check this?

[HyukjinKwon]

Thank you for cc me @maropu
I like this change. BTW, we may have to add a condition for enforcing sampling ratio <= 1.0 too.

[rxin]

Can you describe the bug?

[maropu]

SampleExec possibly generates multiple rows from a single row when withReplacement=true.
So, CodegenContext#copyResult should be true.
See: https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala#L107

[maropu]

@HyukjinKwon thanks your comment. I'll add the requirement.

[SparkQA]

Test build #64392 has finished for PR 14800 at commit 97dde82.

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

[SparkQA]

Test build #64400 has finished for PR 14800 at commit 81c41d5.

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

[srowen]

Also, is it really necessary to limit the sample rate to be <= 1? It's not incoherent to want to sample 200% of a data set if it is with replacement. You'd just be generating a data set 2x the size drawn from the same empirical distribution.

[HyukjinKwon]

@srowen Actually, we are already enforcing it to 100% when the replacement is disabled. So, I suggested this to match this up when it is enabled. Yes, it seems not related with the bug this PR is trying to fix. I apologise for the irrelevant comment.

Ah, it is enforced into 100% when the replacement is disabled because there should be replacements when it exceeds. I see. I thought sampling is to have a representative smaller population from a larger one and therefore, it is not sensible when it exceeds 100%.

[maropu]

In the definition of statistic terms, Sampling is to select a subset of whole data
So, I think the sample rate to be <= 1 is more reasonable.
See: https://en.wikipedia.org/wiki/Sampling_(statistics)

[srowen]

True, but, in the with-replacement case, you're no longer selecting a subset to begin with, because an element can appear twice. "Sample" does generally mean "take a smaller set" but it also means things like "sampling from a distribution".

I wouldn't feel strongly about it except that we're taking away behavior that worked fine.
The RDD API for example, doesn't enforce that the rate must be <= 1 (even for without replacement, which is wrong).

[maropu]

yea, I see. I also have no strong opinion on this. So, both is okay to me.
For now, I'll remove the requirement. What do u think? cc: @HyukjinKwon

[HyukjinKwon]

I am okay with both too. I apologise for the irrelevant comment @maropu .

[maropu]

No problem, thanks your attention :) okay, I'll remove this.

[srowen]

LGTM as a targeted fix

[SparkQA]

Test build #64412 has finished for PR 14800 at commit 97dde82.

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

[srowen]

Merged to master/2.0

[rxin]

@maropu when I asked to describe the bug, I was referring to updating the pull request description to include the description of the bug. Please do that in the future. Thanks.

[maropu]

@rxin okay, I'll do that next time. thanks!