You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@spark.apache.org by maropu <gi...@git.apache.org> on 2018/09/21 05:09:44 UTC

[GitHub] spark pull request #22512: [SPARK-25498][SQL][WIP] Fix SQLQueryTestSuite fai...

GitHub user maropu opened a pull request:

    https://github.com/apache/spark/pull/22512

    [SPARK-25498][SQL][WIP] Fix SQLQueryTestSuite failures when the interpreter mode enabled

    ## What changes were proposed in this pull request?
    This pr fixed test failures in `SQLQueryTestSuite` when the interpreter mode enabled. This pr addressed the two cases below;
    -  The current `InterpretedMutableProjection` can't handle `UnsafeRow` in the internal buffer `mutableRow`. `AggregationIterator` uses `MutableProjection` in that manner and `GenerateMutableProjection` can handle `UnsafeRow` as buffer internally.
    - `Literal` returns different a typed value between codegen and interpreter modes in some cases, e.g., `Literal(1, LongType)` returns a long value for the codegen mode and returns an int value for the interpreter mode. So, `InterpretedUnsafeProjection` fails when running `SQLQueryTestSuite`.
    
    ## How was this patch tested?
    Existing tests.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/maropu/spark InterpreterTest

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/spark/pull/22512.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #22512
    
----
commit 39c5e92713b86f342e756591235f9cbe25126f90
Author: Takeshi Yamamuro <ya...@...>
Date:   2018-09-21T04:25:53Z

    Fix test failures with the interpreter mode enabled

----


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #22512: [SPARK-25498][SQL] InterpretedMutableProjection should h...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22512
  
    Merged build finished. Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #22512: [SPARK-25498][SQL][WIP] Fix SQLQueryTestSuite failures w...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22512
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96395/
    Test FAILed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #22512: [SPARK-25498][SQL] InterpretedMutableProjection should h...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/22512
  
    **[Test build #97962 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/97962/testReport)** for PR 22512 at commit [`56698bb`](https://github.com/apache/spark/commit/56698bb3dc428474e032915185fab936b64d4984).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #22512: [SPARK-25498][SQL][WIP] Fix SQLQueryTestSuite failures w...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22512
  
    Merged build finished. Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #22512: [SPARK-25498][SQL] InterpretedMutableProjection should h...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22512
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/4460/
    Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #22512: [SPARK-25498][SQL] InterpretedMutableProjection should h...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22512
  
    Merged build finished. Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #22512: [SPARK-25498][SQL] InterpretedMutableProjection s...

Posted by maropu <gi...@git.apache.org>.

Github user maropu commented on a diff in the pull request:

    https://github.com/apache/spark/pull/22512#discussion_r227609747
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/InternalRow.scala ---
    @@ -143,4 +144,24 @@ object InternalRow {
         case u: UserDefinedType[_] => getAccessor(u.sqlType)
         case _ => (input, ordinal) => input.get(ordinal, dataType)
       }
    +
    +  /**
    +   * Returns a writer for an `InternalRow` with given data type.
    +   */
    +  def getWriter(ordinal: Int, dt: DataType): (InternalRow, Any) => Unit = dt match {
    +    case BooleanType => (input, v) => input.setBoolean(ordinal, v.asInstanceOf[Boolean])
    +    case ByteType => (input, v) => input.setByte(ordinal, v.asInstanceOf[Byte])
    +    case ShortType => (input, v) => input.setShort(ordinal, v.asInstanceOf[Short])
    +    case IntegerType | DateType => (input, v) => input.setInt(ordinal, v.asInstanceOf[Int])
    +    case LongType | TimestampType => (input, v) => input.setLong(ordinal, v.asInstanceOf[Long])
    +    case FloatType => (input, v) => input.setFloat(ordinal, v.asInstanceOf[Float])
    +    case DoubleType => (input, v) => input.setDouble(ordinal, v.asInstanceOf[Double])
    +    case DecimalType.Fixed(precision, _) =>
    +      (input, v) => input.setDecimal(ordinal, v.asInstanceOf[Decimal], precision)
    +    case CalendarIntervalType | BinaryType | _: ArrayType | StringType | _: StructType |
    +         _: MapType | _: ObjectType | _: UserDefinedType[_] =>
    +      (input, v) => input.update(ordinal, v)
    +    case NullType => (input, v) => {}
    --- End diff --
    
    ok


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #22512: [SPARK-25498][SQL] InterpretedMutableProjection should h...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22512
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/99590/
    Test FAILed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #22512: [SPARK-25498][SQL] InterpretedMutableProjection should h...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/22512
  
    **[Test build #99601 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/99601/testReport)** for PR 22512 at commit [`4cdc504`](https://github.com/apache/spark/commit/4cdc5040feb3da1e4cf9efcf434138d5873fae04).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #22512: [SPARK-25498][SQL] InterpretedMutableProjection should h...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/22512
  
    **[Test build #97952 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/97952/testReport)** for PR 22512 at commit [`a014145`](https://github.com/apache/spark/commit/a014145f94c3c771b42e7a2e73bd67d596ea802e).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #22512: [SPARK-25498][SQL] InterpretedMutableProjection should h...

Posted by maropu <gi...@git.apache.org>.

Github user maropu commented on the issue:

    https://github.com/apache/spark/pull/22512
  
    I'm looking into the failure reason... (passed in the local, but failed in the jenkins...)


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #22512: [SPARK-25498][SQL] InterpretedMutableProjection should h...

Posted by maropu <gi...@git.apache.org>.

Github user maropu commented on the issue:

    https://github.com/apache/spark/pull/22512
  
    ok, I'll add tests.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #22512: [SPARK-25498][SQL][WIP] Fix SQLQueryTestSuite failures w...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/22512
  
    **[Test build #96923 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96923/testReport)** for PR 22512 at commit [`31c623f`](https://github.com/apache/spark/commit/31c623fbe2aa88f8533fd4d477a125f62cc8ab6e).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #22512: [SPARK-25498][SQL] InterpretedMutableProjection s...

Posted by cloud-fan <gi...@git.apache.org>.

Github user cloud-fan commented on a diff in the pull request:

    https://github.com/apache/spark/pull/22512#discussion_r227200656
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/InterpretedMutableProjection.scala ---
    @@ -49,10 +51,54 @@ class InterpretedMutableProjection(expressions: Seq[Expression]) extends Mutable
       def currentValue: InternalRow = mutableRow
     
       override def target(row: InternalRow): MutableProjection = {
    +    // If `mutableRow` is `UnsafeRow`, `MutableProjection` accepts fixed-length types only
    +    assert(!row.isInstanceOf[UnsafeRow] ||
    +      validExprs.forall { case (e, _) => UnsafeRow.isFixedLength(e.dataType) })
         mutableRow = row
         this
       }
     
    +  private[this] val fieldWriters = validExprs.map { case (e, i) =>
    +    val writer = generateRowWriter(i, e.dataType)
    +    if (!e.nullable) {
    +      (v: Any) => writer(v)
    +    } else {
    +      (v: Any) => {
    +        if (v == null) {
    +          mutableRow.setNullAt(i)
    +        } else {
    +          writer(v)
    +        }
    +      }
    +    }
    +  }
    +
    +  private def generateRowWriter(ordinal: Int, dt: DataType): Any => Unit = dt match {
    --- End diff --
    
    we have `InternalRow.getAccessor`, shall we move this method there too?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #22512: [SPARK-25498][SQL] InterpretedMutableProjection should h...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22512
  
    Merged build finished. Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #22512: [SPARK-25498][SQL] InterpretedMutableProjection s...

Posted by maropu <gi...@git.apache.org>.

Github user maropu commented on a diff in the pull request:

    https://github.com/apache/spark/pull/22512#discussion_r227226902
  
    --- Diff: sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala ---
    @@ -140,6 +141,14 @@ class SQLQueryTestSuite extends QueryTest with SharedSQLContext {
         val input = fileToString(new File(testCase.inputFile))
     
         val (comments, code) = input.split("\n").partition(_.startsWith("--"))
    +
    +    // Runs all the tests on both codegen-only and interpreter modes. Since explain results differ
    +    // when `WHOLESTAGE_CODEGEN_ENABLED` disabled, we don't run these tests now.
    +    val codegenConfigSets = Array(("false", "NO_CODEGEN"), ("true", "CODEGEN_ONLY")).map {
    +      case (wholeStageCodegenEnabled, codegenFactoryMode) =>
    +        Array( // SQLConf.WHOLESTAGE_CODEGEN_ENABLED.key -> wholeStageCodegenEnabled,
    --- End diff --
    
    ok


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #22512: [SPARK-25498][SQL] InterpretedMutableProjection should h...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22512
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/5641/
    Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #22512: [SPARK-25498][SQL] InterpretedMutableProjection should h...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22512
  
    Merged build finished. Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #22512: [SPARK-25498][SQL] InterpretedMutableProjection s...

Posted by asfgit <gi...@git.apache.org>.

Github user asfgit closed the pull request at:

    https://github.com/apache/spark/pull/22512


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #22512: [SPARK-25498][SQL] InterpretedMutableProjection should h...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22512
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/3982/
    Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #22512: [SPARK-25498][SQL][WIP] Fix SQLQueryTestSuite failures w...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22512
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/3670/
    Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #22512: [SPARK-25498][SQL] InterpretedMutableProjection should h...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22512
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/97989/
    Test FAILed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #22512: [SPARK-25498][SQL] InterpretedMutableProjection should h...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/22512
  
    **[Test build #98058 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/98058/testReport)** for PR 22512 at commit [`5227e42`](https://github.com/apache/spark/commit/5227e422cda4d110c6b5a950ebdc49d6e25914f1).
     * This patch **fails Spark unit tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #22512: [SPARK-25498][SQL] InterpretedMutableProjection s...

Posted by cloud-fan <gi...@git.apache.org>.

Github user cloud-fan commented on a diff in the pull request:

    https://github.com/apache/spark/pull/22512#discussion_r238172470
  
    --- Diff: sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala ---
    @@ -148,12 +156,21 @@ class SQLQueryTestSuite extends QueryTest with SharedSQLContext {
           })
           // When we are regenerating the golden files we don't need to run all the configs as they
           // all need to return the same result
    -      if (regenerateGoldenFiles && configs.nonEmpty) {
    +      if (regenerateGoldenFiles) {
             configs.take(1)
    --- End diff --
    
    what if `configs` is empty? `take(1)` will fail


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #22512: [SPARK-25498][SQL] InterpretedMutableProjection should h...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22512
  
    Merged build finished. Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #22512: [SPARK-25498][SQL] InterpretedMutableProjection should h...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22512
  
    Merged build finished. Test FAILed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #22512: [SPARK-25498][SQL] InterpretedMutableProjection s...

Posted by maropu <gi...@git.apache.org>.

Github user maropu commented on a diff in the pull request:

    https://github.com/apache/spark/pull/22512#discussion_r238489997
  
    --- Diff: sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala ---
    @@ -148,12 +156,25 @@ class SQLQueryTestSuite extends QueryTest with SharedSQLContext {
           })
           // When we are regenerating the golden files we don't need to run all the configs as they
           // all need to return the same result
    -      if (regenerateGoldenFiles && configs.nonEmpty) {
    -        configs.take(1)
    +      if (regenerateGoldenFiles) {
    +        if (configs.nonEmpty) {
    +          configs.take(1)
    +        } else {
    +          Array.empty[Array[(String, String)]]
    --- End diff --
    
    ok


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #22512: [SPARK-25498][SQL][WIP] Fix SQLQueryTestSuite failures w...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22512
  
    Merged build finished. Test FAILed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org