Skip to content

Commit

Permalink
[SPARK-32640][SQL] Downgrade Janino to fix a correctness bug
Browse files Browse the repository at this point in the history
### What changes were proposed in this pull request?

This PR reverts #27860 to downgrade Janino, as the new version has a bug.

### Why are the changes needed?

The symptom is about NaN comparison. For code below
```
if (double_value <= 0.0) {
  ...
} else {
  ...
}
```

If `double_value` is NaN, `NaN <= 0.0` is false and we should go to the else branch. However, current Spark goes to the if branch and causes correctness issues like SPARK-32640.

One way to fix it is:
```
boolean cond = double_value <= 0.0;
if (cond) {
  ...
} else {
  ...
}
```

I'm not familiar with Janino so I don't know what's going on there.

### Does this PR introduce _any_ user-facing change?

Yes, fix correctness bugs.

### How was this patch tested?

a new test

Closes #29495 from cloud-fan/revert.

Authored-by: Wenchen Fan <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
  • Loading branch information
cloud-fan authored and dongjoon-hyun committed Aug 20, 2020
1 parent d378dc5 commit 8b119f1
Show file tree
Hide file tree
Showing 6 changed files with 17 additions and 14 deletions.
4 changes: 2 additions & 2 deletions dev/deps/spark-deps-hadoop-2.7-hive-1.2
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ commons-beanutils/1.9.4//commons-beanutils-1.9.4.jar
commons-cli/1.2//commons-cli-1.2.jar
commons-codec/1.10//commons-codec-1.10.jar
commons-collections/3.2.2//commons-collections-3.2.2.jar
commons-compiler/3.1.2//commons-compiler-3.1.2.jar
commons-compiler/3.0.16//commons-compiler-3.0.16.jar
commons-compress/1.8.1//commons-compress-1.8.1.jar
commons-configuration/1.6//commons-configuration-1.6.jar
commons-crypto/1.0.0//commons-crypto-1.0.0.jar
Expand Down Expand Up @@ -106,7 +106,7 @@ jakarta.inject/2.6.1//jakarta.inject-2.6.1.jar
jakarta.validation-api/2.0.2//jakarta.validation-api-2.0.2.jar
jakarta.ws.rs-api/2.1.6//jakarta.ws.rs-api-2.1.6.jar
jakarta.xml.bind-api/2.3.2//jakarta.xml.bind-api-2.3.2.jar
janino/3.1.2//janino-3.1.2.jar
janino/3.0.16//janino-3.0.16.jar
javassist/3.25.0-GA//javassist-3.25.0-GA.jar
javax.inject/1//javax.inject-1.jar
javax.servlet-api/3.1.0//javax.servlet-api-3.1.0.jar
Expand Down
4 changes: 2 additions & 2 deletions dev/deps/spark-deps-hadoop-2.7-hive-2.3
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ commons-beanutils/1.9.4//commons-beanutils-1.9.4.jar
commons-cli/1.2//commons-cli-1.2.jar
commons-codec/1.10//commons-codec-1.10.jar
commons-collections/3.2.2//commons-collections-3.2.2.jar
commons-compiler/3.1.2//commons-compiler-3.1.2.jar
commons-compiler/3.0.16//commons-compiler-3.0.16.jar
commons-compress/1.8.1//commons-compress-1.8.1.jar
commons-configuration/1.6//commons-configuration-1.6.jar
commons-crypto/1.0.0//commons-crypto-1.0.0.jar
Expand Down Expand Up @@ -119,7 +119,7 @@ jakarta.inject/2.6.1//jakarta.inject-2.6.1.jar
jakarta.validation-api/2.0.2//jakarta.validation-api-2.0.2.jar
jakarta.ws.rs-api/2.1.6//jakarta.ws.rs-api-2.1.6.jar
jakarta.xml.bind-api/2.3.2//jakarta.xml.bind-api-2.3.2.jar
janino/3.1.2//janino-3.1.2.jar
janino/3.0.16//janino-3.0.16.jar
javassist/3.25.0-GA//javassist-3.25.0-GA.jar
javax.inject/1//javax.inject-1.jar
javax.jdo/3.2.0-m3//javax.jdo-3.2.0-m3.jar
Expand Down
4 changes: 2 additions & 2 deletions dev/deps/spark-deps-hadoop-3.2-hive-2.3
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ commons-beanutils/1.9.4//commons-beanutils-1.9.4.jar
commons-cli/1.2//commons-cli-1.2.jar
commons-codec/1.10//commons-codec-1.10.jar
commons-collections/3.2.2//commons-collections-3.2.2.jar
commons-compiler/3.1.2//commons-compiler-3.1.2.jar
commons-compiler/3.0.16//commons-compiler-3.0.16.jar
commons-compress/1.8.1//commons-compress-1.8.1.jar
commons-configuration2/2.1.1//commons-configuration2-2.1.1.jar
commons-crypto/1.0.0//commons-crypto-1.0.0.jar
Expand Down Expand Up @@ -118,7 +118,7 @@ jakarta.inject/2.6.1//jakarta.inject-2.6.1.jar
jakarta.validation-api/2.0.2//jakarta.validation-api-2.0.2.jar
jakarta.ws.rs-api/2.1.6//jakarta.ws.rs-api-2.1.6.jar
jakarta.xml.bind-api/2.3.2//jakarta.xml.bind-api-2.3.2.jar
janino/3.1.2//janino-3.1.2.jar
janino/3.0.16//janino-3.0.16.jar
javassist/3.25.0-GA//javassist-3.25.0-GA.jar
javax.inject/1//javax.inject-1.jar
javax.jdo/3.2.0-m3//javax.jdo-3.2.0-m3.jar
Expand Down
2 changes: 1 addition & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@
<commons-pool2.version>2.6.2</commons-pool2.version>
<datanucleus-core.version>4.1.17</datanucleus-core.version>
<guava.version>14.0.1</guava.version>
<janino.version>3.1.2</janino.version>
<janino.version>3.0.16</janino.version>
<jersey.version>2.30</jersey.version>
<joda.version>2.10.5</joda.version>
<jodd.version>3.5.2</jodd.version>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,8 @@ import scala.util.control.NonFatal

import com.google.common.cache.{CacheBuilder, CacheLoader}
import com.google.common.util.concurrent.{ExecutionError, UncheckedExecutionException}
import org.codehaus.commons.compiler.{CompileException, InternalCompilerException}
import org.codehaus.commons.compiler.util.reflect.ByteArrayClassLoader
import org.codehaus.janino.{ClassBodyEvaluator, SimpleCompiler}
import org.codehaus.commons.compiler.CompileException
import org.codehaus.janino.{ByteArrayClassLoader, ClassBodyEvaluator, InternalCompilerException, SimpleCompiler}
import org.codehaus.janino.util.ClassFile

import org.apache.spark.{TaskContext, TaskKilledException}
Expand Down Expand Up @@ -1421,10 +1420,9 @@ object CodeGenerator extends Logging {
private def updateAndGetCompilationStats(evaluator: ClassBodyEvaluator): ByteCodeStats = {
// First retrieve the generated classes.
val classes = {
val scField = classOf[ClassBodyEvaluator].getDeclaredField("sc")
scField.setAccessible(true)
val compiler = scField.get(evaluator).asInstanceOf[SimpleCompiler]
val loader = compiler.getClassLoader.asInstanceOf[ByteArrayClassLoader]
val resultField = classOf[SimpleCompiler].getDeclaredField("result")
resultField.setAccessible(true)
val loader = resultField.get(evaluator).asInstanceOf[ByteArrayClassLoader]
val classesField = loader.getClass.getDeclaredField("classes")
classesField.setAccessible(true)
classesField.get(loader).asInstanceOf[JavaMap[String, Array[Byte]]].asScala
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2541,6 +2541,11 @@ class DataFrameSuite extends QueryTest
assert(df.schema === new StructType().add(StructField("d", DecimalType(38, 0))))
}
}

test("SPARK-32640: ln(NaN) should return NaN") {
val df = Seq(Double.NaN).toDF("d")
checkAnswer(df.selectExpr("ln(d)"), Row(Double.NaN))
}
}

case class GroupByKey(a: Int, b: Int)

0 comments on commit 8b119f1

Please sign in to comment.