Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-15825][SQL] Fix SMJ invalid results #13589

Closed
wants to merge 2 commits into from

Conversation

hvanhovell
Copy link
Contributor

@hvanhovell hvanhovell commented Jun 10, 2016

What changes were proposed in this pull request?

Code generated SortMergeJoin failed with wrong results when using structs as keys. This could (eventually) be traced back to the use of a wrong row reference when comparing structs.

How was this patch tested?

TBD

@SparkQA
Copy link

SparkQA commented Jun 10, 2016

Test build #60268 has finished for PR 13589 at commit 995c86a.

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

@rxin
Copy link
Contributor

rxin commented Jun 10, 2016

cc @davies

mystery of INPUT_ROW...

@@ -490,6 +490,7 @@ class CodegenContext {
addNewFunction(compareFunc, funcCode)
s"this.$compareFunc($c1, $c2)"
case schema: StructType =>
INPUT_ROW = "i"
val comparisons = GenerateOrdering.genComparisons(this, schema)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it better to use InternalRow $INPUT_ROW = null; in an assignment for funcCode, to clearly show intention of this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kiszk i is set to null here: https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala#L498

I'd rather not change this to INPUT_ROW since this could potentially make things even more confusing.

@a-roberts
Copy link
Contributor

a-roberts commented Jun 10, 2016

Still getting seg faults on Power and with Intel on OpenJDK with this change when we use spark-submit with two worker instances.

Git diff

--- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala
+++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala
@@ -490,6 +490,7 @@ class CodegenContext {
addNewFunction(compareFunc, funcCode)
s"this.$compareFunc($c1, $c2)"
case schema: StructType =>

  •  INPUT_ROW = "i"
    
    val comparisons = GenerateOrdering.genComparisons(this, schema)
    val compareFunc = freshName("compareStruct")
    val funcCode: String =

segv occurs four times

cat work/app-20160610093117-0000/*/stderr | grep -C 2 "Segmentation error"
16/06/10 09:34:27 INFO CodeGenerator: Code generated in 115.554506 ms
Unhandled exception
Type=Segmentation error vmState=0x00000000
J9Generic_Signal_Number=00000004 Signal_Number=0000000b Error_Value=00000000 Signal_Code=00000001
Handler1=00003FFF7B487AA0 Handler2=00003FFF7B312AC0

spark-env.sh has
export SPARK_WORKER_CORES=2
export SPARK_WORKER_INSTANCES=2
no settings for going off-heap

@robbinspg
Copy link
Member

As Adam says I still get the segv with OpenJDK on linux amd64 running our app. This fix does appear to fix the issue reported in https://issues.apache.org/jira/browse/SPARK-15825

@hvanhovell hvanhovell changed the title [SPARK-15822][SPARK-15825][SQL] Fix SMJ Segfault/Invalid results [SPARK-15825][SQL] Fix SMJ Segfault/Invalid results Jun 10, 2016
@hvanhovell hvanhovell changed the title [SPARK-15825][SQL] Fix SMJ Segfault/Invalid results [SPARK-15825][SQL] Fix SMJ invalid results Jun 10, 2016
@SparkQA
Copy link

SparkQA commented Jun 10, 2016

Test build #60302 has finished for PR 13589 at commit 08cc611.

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

@davies
Copy link
Contributor

davies commented Jun 10, 2016

This line

InternalRow i = null;

requires that INPUT_ROW should be i (could be changed to a refresh name). The fix looks reasonable to me.

@davies
Copy link
Contributor

davies commented Jun 10, 2016

Merging this into master and 2.0, thanks!

asfgit pushed a commit that referenced this pull request Jun 10, 2016
## What changes were proposed in this pull request?
Code generated `SortMergeJoin` failed with wrong results when using structs as keys. This could (eventually) be traced back to the use of a wrong row reference when comparing structs.

## How was this patch tested?
TBD

Author: Herman van Hovell <hvanhovell@databricks.com>

Closes #13589 from hvanhovell/SPARK-15822.

(cherry picked from commit e05a2fe)
Signed-off-by: Davies Liu <davies.liu@gmail.com>
@asfgit asfgit closed this in e05a2fe Jun 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants