You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by kiszk <gi...@git.apache.org> on 2017/07/02 08:31:23 UTC

[GitHub] spark pull request #18503: [WIP][SPARK-21271][SQL] Ensure Unsafe.sizeInBytes...

GitHub user kiszk opened a pull request:

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

    [WIP][SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multiple of 8

    ## What changes were proposed in this pull request?
    
    This PR ensures that `Unsafe.sizeInBytes` must be a multiple of 8. It it is not satisfied. `Unsafe.hashCode` causes the assertion violation.
    
    ## How was this patch tested?
    
    Will add test cases

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

    $ git pull https://github.com/kiszk/spark SPARK-21271

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

    https://github.com/apache/spark/pull/18503.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 #18503
    
----
commit eb87c63f653917af5a221b780681bb7de6fd217c
Author: Kazuaki Ishizaki <is...@jp.ibm.com>
Date:   2017-07-02T08:28:30Z

    insert assert to ensure Unsafe.sizeInBytes is a multiple of 8

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

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/18503#discussion_r129471461
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/HDFSBackedStateStoreProvider.scala ---
    @@ -363,7 +363,11 @@ private[state] class HDFSBackedStateStoreProvider extends StateStoreProvider wit
                 val valueRowBuffer = new Array[Byte](valueSize)
                 ByteStreams.readFully(input, valueRowBuffer, 0, valueSize)
                 val valueRow = new UnsafeRow(valueSchema.fields.length)
    -            valueRow.pointTo(valueRowBuffer, valueSize)
    +            // If valueSize in existing file is not multiple of 8, floor it to multiple of 8.
    +            // This is work around for the following.
    +            // Pre-Spark 2.3 mistakenly append 4 bytes to the value row in
    --- End diff --
    
    nit: `Prior to Spark 2.3, we mistakenly ...`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:

    https://github.com/apache/spark/pull/18503#discussion_r128408940
  
    --- Diff: sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/UnsafeRow.java ---
    @@ -167,6 +167,7 @@ public UnsafeRow() {}
        */
       public void pointTo(Object baseObject, long baseOffset, int sizeInBytes) {
         assert numFields >= 0 : "numFields (" + numFields + ") should >= 0";
    +    assert sizeInBytes % 8 == 0 : "sizeInBytes (" + sizeInBytes + ") should be a multiple of 8";
    --- End diff --
    
    Yes, done.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

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/18503#discussion_r125870470
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/HDFSBackedStateStoreProvider.scala ---
    @@ -350,20 +350,24 @@ private[state] class HDFSBackedStateStoreProvider extends StateStoreProvider wit
               throw new IOException(
                 s"Error reading delta file $fileToRead of $this: key size cannot be $keySize")
             } else {
    -          val keyRowBuffer = new Array[Byte](keySize)
    +          // If key size in an existing file is not a multiple of 8, round it to multiple of 8
    +          val keyAllocationSize = ((keySize + 7) / 8) * 8
    +          val keyRowBuffer = new Array[Byte](keyAllocationSize)
    --- End diff --
    
    why we have this logic? what do we write into the state store?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [WIP][SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a m...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [WIP][SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a m...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:

    https://github.com/apache/spark/pull/18503#discussion_r127185799
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/HDFSBackedStateStoreProvider.scala ---
    @@ -350,20 +350,24 @@ private[state] class HDFSBackedStateStoreProvider extends StateStoreProvider wit
               throw new IOException(
                 s"Error reading delta file $fileToRead of $this: key size cannot be $keySize")
             } else {
    -          val keyRowBuffer = new Array[Byte](keySize)
    +          // If key size in an existing file is not a multiple of 8, round it to multiple of 8
    +          val keyAllocationSize = ((keySize + 7) / 8) * 8
    +          val keyRowBuffer = new Array[Byte](keyAllocationSize)
    --- End diff --
    
    @cloud-fan @zsxwing @tdas @kunalkhamar 
    I misunderstood. To store a state, `HDFSBackedStateStoreProvider` is used.
    
    I added [a new test suite](https://github.com/apache/spark/pull/18503/files#diff-b601a70172e88e90cea0020b978d0b5bR482) to check `HDFSBackedStateStoreProvider` for storing and restoring state, as @kunalkhamar suggested [here](https://github.com/apache/spark/pull/18503#discussion_r126028838).
    
    Do you think it makes sense?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

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/18503#discussion_r128439416
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/HDFSBackedStateStoreProvider.scala ---
    @@ -363,7 +363,8 @@ private[state] class HDFSBackedStateStoreProvider extends StateStoreProvider wit
                 val valueRowBuffer = new Array[Byte](valueSize)
                 ByteStreams.readFully(input, valueRowBuffer, 0, valueSize)
                 val valueRow = new UnsafeRow(valueSchema.fields.length)
    -            valueRow.pointTo(valueRowBuffer, valueSize)
    +            // If valueSize in existing file is not multiple of 8, round it down to multiple of 8
    +            valueRow.pointTo(valueRowBuffer, (valueSize / 8) * 8)
    --- End diff --
    
    yes, because the extra bytes that exceed the 8-bytes boundary in `UnsafeRow` will never be read, so it's safe to ignore them. And here we still respect the row length when reading the binary, so no data will be missed.
    
    @kiszk can we add more comments to explain why this can happen? We should say that before Spark 2.3 we mistakenly append 4 bytes to the value row in aggregate buffer, which gets persisted into the checkpoint data and so on


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/18503
  
    **[Test build #79583 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/79583/testReport)** for PR 18503 at commit [`7f5a269`](https://github.com/apache/spark/commit/7f5a269c70c7dfe22d7d990dc075061682c09804).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

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/18503#discussion_r128133349
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/HDFSBackedStateStoreProvider.scala ---
    @@ -350,20 +350,24 @@ private[state] class HDFSBackedStateStoreProvider extends StateStoreProvider wit
               throw new IOException(
                 s"Error reading delta file $fileToRead of $this: key size cannot be $keySize")
             } else {
    -          val keyRowBuffer = new Array[Byte](keySize)
    +          // If key size in an existing file is not a multiple of 8, round it to multiple of 8
    --- End diff --
    
    I don't think we can round. Assume the actual length of an unsafe row is 8, and previously we will append 4 bytes and have an unsafe row with 12 bytes, and save it to checkpoint. So here, when we reading old checkppint, we need to read 12 bytes, and set the length to 8.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

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/18503#discussion_r125638233
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/HDFSBackedStateStoreProvider.scala ---
    @@ -350,20 +350,24 @@ private[state] class HDFSBackedStateStoreProvider extends StateStoreProvider wit
               throw new IOException(
                 s"Error reading delta file $fileToRead of $this: key size cannot be $keySize")
             } else {
    -          val keyRowBuffer = new Array[Byte](keySize)
    +          // If key size in an existing file is not a multiple of 8, round it to multiple of 8
    +          val keyAllocationSize = ((keySize + 7) / 8) * 8
    +          val keyRowBuffer = new Array[Byte](keyAllocationSize)
    --- End diff --
    
    so `RowBasedKeyValueBatch` is the format for state store? cc @zsxwing 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:

    https://github.com/apache/spark/pull/18503#discussion_r128408918
  
    --- Diff: sql/core/src/test/scala/org/apache/spark/sql/streaming/StreamSuite.scala ---
    @@ -479,6 +479,61 @@ class StreamSuite extends StreamTest {
           CheckAnswer((1, 2), (2, 2), (3, 2)))
       }
     
    +  testQuietly("store to and recover from a checkpoint") {
    --- End diff --
    
    Ah, you are right. This test currently relies on internal assert at `Unsafe.pointTo` for checking a multiple of 8 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

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/18503#discussion_r129471510
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/HDFSBackedStateStoreProvider.scala ---
    @@ -363,7 +363,11 @@ private[state] class HDFSBackedStateStoreProvider extends StateStoreProvider wit
                 val valueRowBuffer = new Array[Byte](valueSize)
                 ByteStreams.readFully(input, valueRowBuffer, 0, valueSize)
                 val valueRow = new UnsafeRow(valueSchema.fields.length)
    -            valueRow.pointTo(valueRowBuffer, valueSize)
    +            // If valueSize in existing file is not multiple of 8, floor it to multiple of 8.
    +            // This is work around for the following.
    +            // Pre-Spark 2.3 mistakenly append 4 bytes to the value row in
    +            // `FixedLengthRowBasedKeyValueBatch`, which gets persisted into the checkpoint data
    --- End diff --
    
    and `VariableLengthRowBasedKeyValueBatch`, let's just say `RowBasedKeyValueBatch`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

Posted by tdas <gi...@git.apache.org>.
Github user tdas commented on a diff in the pull request:

    https://github.com/apache/spark/pull/18503#discussion_r128122780
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/HDFSBackedStateStoreProvider.scala ---
    @@ -413,21 +417,25 @@ private[state] class HDFSBackedStateStoreProvider extends StateStoreProvider wit
               throw new IOException(
                 s"Error reading snapshot file $fileToRead of $this: key size cannot be $keySize")
             } else {
    -          val keyRowBuffer = new Array[Byte](keySize)
    +          // If key size in an existing file is not a multiple of 8, round it to multiple of 8
    +          val keyAllocationSize = ((keySize + 7) / 8) * 8
    +          val keyRowBuffer = new Array[Byte](keyAllocationSize)
               ByteStreams.readFully(input, keyRowBuffer, 0, keySize)
     
               val keyRow = new UnsafeRow(keySchema.fields.length)
    -          keyRow.pointTo(keyRowBuffer, keySize)
    +          keyRow.pointTo(keyRowBuffer, keyAllocationSize)
     
               val valueSize = input.readInt()
               if (valueSize < 0) {
                 throw new IOException(
                   s"Error reading snapshot file $fileToRead of $this: value size cannot be $valueSize")
               } else {
    -            val valueRowBuffer = new Array[Byte](valueSize)
    +            // If value size in an existing file is not a multiple of 8, round it to multiple of 8
    +            val valueAllocationSize = ((valueSize + 7) / 8) * 8
    --- End diff --
    
    For example, how about something like this 
    `class UnsafeRow { def readFromStream(bytes: Int): this  }`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:

    https://github.com/apache/spark/pull/18503#discussion_r126077681
  
    --- Diff: sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/FixedLengthRowBasedKeyValueBatch.java ---
    @@ -62,7 +62,7 @@ public UnsafeRow appendRow(Object kbase, long koff, int klen,
     
         keyRowId = numRows;
         keyRow.pointTo(base, recordOffset, klen);
    -    valueRow.pointTo(base, recordOffset + klen, vlen + 4);
    --- End diff --
    
    @ooq thank you for pointing out interesting discussion.
    This discussion seems to make sense for page management. The question of @cloud-fan and me is whether `valueRow` uses only `vlen`. I think that `+4` is for page management.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:

    https://github.com/apache/spark/pull/18503#discussion_r125887920
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/HDFSBackedStateStoreProvider.scala ---
    @@ -350,20 +350,24 @@ private[state] class HDFSBackedStateStoreProvider extends StateStoreProvider wit
               throw new IOException(
                 s"Error reading delta file $fileToRead of $this: key size cannot be $keySize")
             } else {
    -          val keyRowBuffer = new Array[Byte](keySize)
    +          // If key size in an existing file is not a multiple of 8, round it to multiple of 8
    +          val keyAllocationSize = ((keySize + 7) / 8) * 8
    +          val keyRowBuffer = new Array[Byte](keyAllocationSize)
    --- End diff --
    
    [Here](https://github.com/apache/spark/pull/18503#issuecomment-312936884) is why I added this logic. 
    I believe that this failure occurs since [the checkpoint file](https://github.com/apache/spark/blob/master/sql/core/src/test/resources/structured-streaming/checkpoint-version-2.1.0/state/0/1/1.delta) does not have a value record whose size is not a multiple of 8 (i.e. 28). Thus, I always round up its size to a multiple of 8.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/18503
  
    **[Test build #79956 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/79956/testReport)** for PR 18503 at commit [`cc467de`](https://github.com/apache/spark/commit/cc467de59a5cb7de0e8bdf3421e4a019b31feaff).
     * This patch **fails due to an unknown error code, -9**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:

    https://github.com/apache/spark/pull/18503#discussion_r125546491
  
    --- Diff: sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/FixedLengthRowBasedKeyValueBatch.java ---
    @@ -62,7 +62,7 @@ public UnsafeRow appendRow(Object kbase, long koff, int klen,
     
         keyRowId = numRows;
         keyRow.pointTo(base, recordOffset, klen);
    -    valueRow.pointTo(base, recordOffset + klen, vlen + 4);
    +    valueRow.pointTo(base, recordOffset + klen, vlen + 8);
    --- End diff --
    
    Line 59 puts long value whose length is 8. In summary, line 57 and 59 consumes `vlen + 8` bytes from `base + recordOffset+ klen`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

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/18503#discussion_r125637685
  
    --- Diff: sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/FixedLengthRowBasedKeyValueBatch.java ---
    @@ -62,7 +62,7 @@ public UnsafeRow appendRow(Object kbase, long koff, int klen,
     
         keyRowId = numRows;
         keyRow.pointTo(base, recordOffset, klen);
    -    valueRow.pointTo(base, recordOffset + klen, vlen + 4);
    --- End diff --
    
    I'm wondering why we did this before. Was it a mistake?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

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/18503#discussion_r125546902
  
    --- Diff: sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/FixedLengthRowBasedKeyValueBatch.java ---
    @@ -62,7 +62,7 @@ public UnsafeRow appendRow(Object kbase, long koff, int klen,
     
         keyRowId = numRows;
         keyRow.pointTo(base, recordOffset, klen);
    -    valueRow.pointTo(base, recordOffset + klen, vlen + 4);
    +    valueRow.pointTo(base, recordOffset + klen, vlen + 8);
    --- End diff --
    
    strictly speaking, the final long value doesn't belong to value row, why are we doing so?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

Posted by tdas <gi...@git.apache.org>.
Github user tdas commented on a diff in the pull request:

    https://github.com/apache/spark/pull/18503#discussion_r128122256
  
    --- Diff: sql/core/src/test/scala/org/apache/spark/sql/streaming/StreamSuite.scala ---
    @@ -479,6 +479,61 @@ class StreamSuite extends StreamTest {
           CheckAnswer((1, 2), (2, 2), (3, 2)))
       }
     
    +  testQuietly("store to and recover from a checkpoint") {
    --- End diff --
    
    I dont think this test is needed. There are existing tests that already test reading from checkpoints, etc. The critical test was reading 2.1 checkpoint files which seems to be passing.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/18503
  
    **[Test build #79929 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/79929/testReport)** for PR 18503 at commit [`54be80e`](https://github.com/apache/spark/commit/54be80ef4849fddb3ff51c53a64173883d1ed026).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

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/18503#discussion_r128438054
  
    --- Diff: sql/core/src/test/scala/org/apache/spark/sql/streaming/StreamSuite.scala ---
    @@ -479,6 +479,61 @@ class StreamSuite extends StreamTest {
           CheckAnswer((1, 2), (2, 2), (3, 2)))
       }
     
    +  testQuietly("store to and recover from a checkpoint") {
    --- End diff --
    
    shall we remove this test


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [WIP][SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a m...

Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on the issue:

    https://github.com/apache/spark/pull/18503
  
    @cloud-fan I believe that [this failure](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/79048/testReport/) occurs since [the checkpoint file](https://github.com/apache/spark/blob/master/sql/core/src/test/resources/structured-streaming/checkpoint-version-2.1.0/state/0/1/1.delta) does not have a value record whose size is not a multiple of 8 (i.e. 28).
    
    We can solve this failure by regenerating these files. However, the root issue is that checkpoint files, whose size may not be a multiple of 8, exist in the production environment. Should we resize the value record size for `UnsafeRow` to a multiple of 8 if it is not a multiple of 8?
    
    What do you think?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [WIP][SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a m...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:

    https://github.com/apache/spark/pull/18503#discussion_r125658293
  
    --- Diff: sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/FixedLengthRowBasedKeyValueBatch.java ---
    @@ -62,7 +62,7 @@ public UnsafeRow appendRow(Object kbase, long koff, int klen,
     
         keyRowId = numRows;
         keyRow.pointTo(base, recordOffset, klen);
    -    valueRow.pointTo(base, recordOffset + klen, vlen + 4);
    --- End diff --
    
    I have the same question.
    @sameeragarwal had [similar question](https://github.com/apache/spark/pull/14174#r71253933) one years ago. However, no response from @ooq 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:

    https://github.com/apache/spark/pull/18503
  
    This PR we changed `FixedLengthRowBasedKeyValueBatch`, I want to know if it has some external impacts, like checkpoint data, or it's just used internally?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:

    https://github.com/apache/spark/pull/18503#discussion_r125703851
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/HDFSBackedStateStoreProvider.scala ---
    @@ -350,20 +350,24 @@ private[state] class HDFSBackedStateStoreProvider extends StateStoreProvider wit
               throw new IOException(
                 s"Error reading delta file $fileToRead of $this: key size cannot be $keySize")
             } else {
    -          val keyRowBuffer = new Array[Byte](keySize)
    +          // If key size in an existing file is not a multiple of 8, round it to multiple of 8
    +          val keyAllocationSize = ((keySize + 7) / 8) * 8
    +          val keyRowBuffer = new Array[Byte](keyAllocationSize)
    --- End diff --
    
    I think that `RowBasedKeyValueBatch` is not used for state store in `HDFSBackedStateStoreProvider` for now.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:

    https://github.com/apache/spark/pull/18503#discussion_r125963457
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/HDFSBackedStateStoreProvider.scala ---
    @@ -350,20 +350,24 @@ private[state] class HDFSBackedStateStoreProvider extends StateStoreProvider wit
               throw new IOException(
                 s"Error reading delta file $fileToRead of $this: key size cannot be $keySize")
             } else {
    -          val keyRowBuffer = new Array[Byte](keySize)
    +          // If key size in an existing file is not a multiple of 8, round it to multiple of 8
    +          val keyAllocationSize = ((keySize + 7) / 8) * 8
    +          val keyRowBuffer = new Array[Byte](keyAllocationSize)
    --- End diff --
    
    If OK, would it be possible to let us know how to re-generated these files?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

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/18503#discussion_r129471398
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/HDFSBackedStateStoreProvider.scala ---
    @@ -363,7 +363,11 @@ private[state] class HDFSBackedStateStoreProvider extends StateStoreProvider wit
                 val valueRowBuffer = new Array[Byte](valueSize)
                 ByteStreams.readFully(input, valueRowBuffer, 0, valueSize)
                 val valueRow = new UnsafeRow(valueSchema.fields.length)
    -            valueRow.pointTo(valueRowBuffer, valueSize)
    +            // If valueSize in existing file is not multiple of 8, floor it to multiple of 8.
    +            // This is work around for the following.
    --- End diff --
    
    nit: `This is a workaround for the following:`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

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/18503#discussion_r128133007
  
    --- Diff: sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/UnsafeRow.java ---
    @@ -167,6 +167,7 @@ public UnsafeRow() {}
        */
       public void pointTo(Object baseObject, long baseOffset, int sizeInBytes) {
         assert numFields >= 0 : "numFields (" + numFields + ") should >= 0";
    +    assert sizeInBytes % 8 == 0 : "sizeInBytes (" + sizeInBytes + ") should be a multiple of 8";
    --- End diff --
    
    I think we only need the assertion here, in `pointTo`, and in `setTotalSize`. Other places are just checking length for existing unsafe rows, which is unnecessary.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/18503
  
    **[Test build #79214 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/79214/testReport)** for PR 18503 at commit [`8a7a948`](https://github.com/apache/spark/commit/8a7a9483ba727d61486b8fa111a026a86abdfb64).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by tdas <gi...@git.apache.org>.
Github user tdas commented on the issue:

    https://github.com/apache/spark/pull/18503
  
    @cloud-fan @kiszk thanks for confirming. LGTM pending tests.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:

    https://github.com/apache/spark/pull/18503#discussion_r126116820
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/HDFSBackedStateStoreProvider.scala ---
    @@ -350,20 +350,24 @@ private[state] class HDFSBackedStateStoreProvider extends StateStoreProvider wit
               throw new IOException(
                 s"Error reading delta file $fileToRead of $this: key size cannot be $keySize")
             } else {
    -          val keyRowBuffer = new Array[Byte](keySize)
    +          // If key size in an existing file is not a multiple of 8, round it to multiple of 8
    +          val keyAllocationSize = ((keySize + 7) / 8) * 8
    +          val keyRowBuffer = new Array[Byte](keyAllocationSize)
    --- End diff --
    
    We have to exactly know how these check point files were generated. Were these files generated by a method that @kunalkhamar? 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [WIP][SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a m...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/18503
  
    **[Test build #79770 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/79770/testReport)** for PR 18503 at commit [`762f02a`](https://github.com/apache/spark/commit/762f02a2c9211ab953a2dc4b2d9938911f2e883d).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

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/18503#discussion_r125958525
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/HDFSBackedStateStoreProvider.scala ---
    @@ -350,20 +350,24 @@ private[state] class HDFSBackedStateStoreProvider extends StateStoreProvider wit
               throw new IOException(
                 s"Error reading delta file $fileToRead of $this: key size cannot be $keySize")
             } else {
    -          val keyRowBuffer = new Array[Byte](keySize)
    +          // If key size in an existing file is not a multiple of 8, round it to multiple of 8
    +          val keyAllocationSize = ((keySize + 7) / 8) * 8
    +          val keyRowBuffer = new Array[Byte](keyAllocationSize)
    --- End diff --
    
    cc @kunalkhamar @zsxwing is it ok if we re-generate these files?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

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/18503#discussion_r126074568
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/HDFSBackedStateStoreProvider.scala ---
    @@ -350,20 +350,24 @@ private[state] class HDFSBackedStateStoreProvider extends StateStoreProvider wit
               throw new IOException(
                 s"Error reading delta file $fileToRead of $this: key size cannot be $keySize")
             } else {
    -          val keyRowBuffer = new Array[Byte](keySize)
    +          // If key size in an existing file is not a multiple of 8, round it to multiple of 8
    +          val keyAllocationSize = ((keySize + 7) / 8) * 8
    +          val keyRowBuffer = new Array[Byte](keyAllocationSize)
    --- End diff --
    
    ok we need to figure out what's going on, seems there are other places we may have wrong size in UnsafeRow


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [WIP][SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a m...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:

    https://github.com/apache/spark/pull/18503#discussion_r125939285
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/HDFSBackedStateStoreProvider.scala ---
    @@ -350,20 +350,24 @@ private[state] class HDFSBackedStateStoreProvider extends StateStoreProvider wit
               throw new IOException(
                 s"Error reading delta file $fileToRead of $this: key size cannot be $keySize")
             } else {
    -          val keyRowBuffer = new Array[Byte](keySize)
    +          // If key size in an existing file is not a multiple of 8, round it to multiple of 8
    +          val keyAllocationSize = ((keySize + 7) / 8) * 8
    +          val keyRowBuffer = new Array[Byte](keyAllocationSize)
    --- End diff --
    
    While I checked [this commit](https://github.com/apache/spark/pull/17216/commits/60ec7da08ea9b55d6f307c894e0dbd0a116d9413) that put checkpoint files, this did not describe how these files were generated.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

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/18503#discussion_r125545662
  
    --- Diff: sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/FixedLengthRowBasedKeyValueBatch.java ---
    @@ -62,7 +62,7 @@ public UnsafeRow appendRow(Object kbase, long koff, int klen,
     
         keyRowId = numRows;
         keyRow.pointTo(base, recordOffset, klen);
    -    valueRow.pointTo(base, recordOffset + klen, vlen + 4);
    +    valueRow.pointTo(base, recordOffset + klen, vlen + 8);
    --- End diff --
    
    why add `8` here?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

Posted by kunalkhamar <gi...@git.apache.org>.
Github user kunalkhamar commented on a diff in the pull request:

    https://github.com/apache/spark/pull/18503#discussion_r126028838
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/HDFSBackedStateStoreProvider.scala ---
    @@ -350,20 +350,24 @@ private[state] class HDFSBackedStateStoreProvider extends StateStoreProvider wit
               throw new IOException(
                 s"Error reading delta file $fileToRead of $this: key size cannot be $keySize")
             } else {
    -          val keyRowBuffer = new Array[Byte](keySize)
    +          // If key size in an existing file is not a multiple of 8, round it to multiple of 8
    +          val keyAllocationSize = ((keySize + 7) / 8) * 8
    +          val keyRowBuffer = new Array[Byte](keyAllocationSize)
    --- End diff --
    
    Regen would involve running a query that looks something like:
    
    - set # shuffle partitions to 10
    - add 1,2,3,4
    - processAllAvail
    - add 3,4,5,6
    - processAllAvail
    - stop
    
    The checkpoint generated by this query should do it. This should be generated with Spark 2.1 branch.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:

    https://github.com/apache/spark/pull/18503#discussion_r129228541
  
    --- Diff: sql/core/src/test/scala/org/apache/spark/sql/streaming/StreamSuite.scala ---
    @@ -479,6 +479,61 @@ class StreamSuite extends StreamTest {
           CheckAnswer((1, 2), (2, 2), (3, 2)))
       }
     
    +  testQuietly("store to and recover from a checkpoint") {
    --- End diff --
    
    Yes, I will remove this since the test that @tdas pointed out causes the same assertion failure as my test case expected.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [WIP][SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a m...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:

    https://github.com/apache/spark/pull/18503
  
    LGTM, pending tests


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:

    https://github.com/apache/spark/pull/18503#discussion_r125577457
  
    --- Diff: sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/FixedLengthRowBasedKeyValueBatch.java ---
    @@ -62,7 +62,7 @@ public UnsafeRow appendRow(Object kbase, long koff, int klen,
     
         keyRowId = numRows;
         keyRow.pointTo(base, recordOffset, klen);
    -    valueRow.pointTo(base, recordOffset + klen, vlen + 4);
    +    valueRow.pointTo(base, recordOffset + klen, vlen + 8);
    --- End diff --
    
    Good catch. While `Long` consumes 8 bytes in a page, this is not a part of `UnsafeRow`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

Posted by tdas <gi...@git.apache.org>.
Github user tdas commented on a diff in the pull request:

    https://github.com/apache/spark/pull/18503#discussion_r129465467
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/HDFSBackedStateStoreProvider.scala ---
    @@ -363,7 +363,11 @@ private[state] class HDFSBackedStateStoreProvider extends StateStoreProvider wit
                 val valueRowBuffer = new Array[Byte](valueSize)
                 ByteStreams.readFully(input, valueRowBuffer, 0, valueSize)
                 val valueRow = new UnsafeRow(valueSchema.fields.length)
    -            valueRow.pointTo(valueRowBuffer, valueSize)
    +            // If valueSize in existing file is not multiple of 8, floor it to multiple of 8.
    +            // This is work around for the following.
    +            // Pre-Spark 2.3 mistakenly append 4 bytes to the value row in
    +            // `FixedLengthRowBasedKeyValueBatch`, which gets persisted into the checkpoint data
    +            valueRow.pointTo(valueRowBuffer, (valueSize / 8) * 8)
    --- End diff --
    
    @cloud-fan @kiszk Just to confirm again, are we absolutely sure that the issue is that there 4 extra bytes in checkpointed rows, and therefore this truncation is safe to do?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

Posted by tdas <gi...@git.apache.org>.
Github user tdas commented on a diff in the pull request:

    https://github.com/apache/spark/pull/18503#discussion_r128408166
  
    --- Diff: sql/core/src/test/scala/org/apache/spark/sql/streaming/StreamSuite.scala ---
    @@ -479,6 +479,61 @@ class StreamSuite extends StreamTest {
           CheckAnswer((1, 2), (2, 2), (3, 2)))
       }
     
    +  testQuietly("store to and recover from a checkpoint") {
    --- End diff --
    
    It does not really check it explicitly .. does it? It tests it implicitly by creating checkpoints and then restarting. There are other tests that already do the same thing. E.g. This test is effectively same as 
    https://github.com/apache/spark/blob/master/sql/core/src/test/scala/org/apache/spark/sql/streaming/StreamingAggregationSuite.scala#L88


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on the issue:

    https://github.com/apache/spark/pull/18503
  
    @tdas @marmbrus @zsxwing could you please let us know any impacts by change of `FixedLengthRowBasedKeyValueBatch`? Can we change this structure?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:

    https://github.com/apache/spark/pull/18503#discussion_r126116898
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/HDFSBackedStateStoreProvider.scala ---
    @@ -350,20 +350,24 @@ private[state] class HDFSBackedStateStoreProvider extends StateStoreProvider wit
               throw new IOException(
                 s"Error reading delta file $fileToRead of $this: key size cannot be $keySize")
             } else {
    -          val keyRowBuffer = new Array[Byte](keySize)
    +          // If key size in an existing file is not a multiple of 8, round it to multiple of 8
    +          val keyAllocationSize = ((keySize + 7) / 8) * 8
    +          val keyRowBuffer = new Array[Byte](keyAllocationSize)
    --- End diff --
    
    Should we add a program, which @kunalkhamar pointed out at [here](https://github.com/apache/spark/pull/18503#discussion_r126028838), as a new test case to check whether all of the size in `UnsafeRow` are correct or not?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

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/18503#discussion_r129474900
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/HDFSBackedStateStoreProvider.scala ---
    @@ -363,7 +363,11 @@ private[state] class HDFSBackedStateStoreProvider extends StateStoreProvider wit
                 val valueRowBuffer = new Array[Byte](valueSize)
                 ByteStreams.readFully(input, valueRowBuffer, 0, valueSize)
                 val valueRow = new UnsafeRow(valueSchema.fields.length)
    -            valueRow.pointTo(valueRowBuffer, valueSize)
    +            // If valueSize in existing file is not multiple of 8, floor it to multiple of 8.
    +            // This is work around for the following.
    +            // Pre-Spark 2.3 mistakenly append 4 bytes to the value row in
    +            // `FixedLengthRowBasedKeyValueBatch`, which gets persisted into the checkpoint data
    +            valueRow.pointTo(valueRowBuffer, (valueSize / 8) * 8)
    --- End diff --
    
    yes I'm absolutely sure, as we store aggregate buffers in checkpoint files.
    
    BTW, I run [this test](https://github.com/apache/spark/pull/18503/commits/01597010145f7769506cfa51c0450a2db779fc07#diff-b601a70172e88e90cea0020b978d0b5bL482) without the workaround here, and it can still pass. Which means, after this PR, `RowBasedKeyValueBatch` is fixed, and the generated checkpoint files have corrected row length.
    
    And I also run the test without the fix for `RowBasedKeyValueBatch`, and it fails. I think this proves that `RowBasedKeyValueBatch` is the reason why we have wrong row length in checkpoit files.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

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/18503#discussion_r128133369
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/HDFSBackedStateStoreProvider.scala ---
    @@ -350,20 +350,24 @@ private[state] class HDFSBackedStateStoreProvider extends StateStoreProvider wit
               throw new IOException(
                 s"Error reading delta file $fileToRead of $this: key size cannot be $keySize")
             } else {
    -          val keyRowBuffer = new Array[Byte](keySize)
    +          // If key size in an existing file is not a multiple of 8, round it to multiple of 8
    --- End diff --
    
    BTW, we only need to do this for value, not key.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:

    https://github.com/apache/spark/pull/18503
  
    thanks, merging to master!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

Posted by ooq <gi...@git.apache.org>.
Github user ooq commented on a diff in the pull request:

    https://github.com/apache/spark/pull/18503#discussion_r125992343
  
    --- Diff: sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/FixedLengthRowBasedKeyValueBatch.java ---
    @@ -62,7 +62,7 @@ public UnsafeRow appendRow(Object kbase, long koff, int klen,
     
         keyRowId = numRows;
         keyRow.pointTo(base, recordOffset, klen);
    -    valueRow.pointTo(base, recordOffset + klen, vlen + 4);
    --- End diff --
    
    I recall it being intentional. 
    See discussion [here](https://github.com/apache/spark/pull/14174#discussion-diff-71254165).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/18503
  
    **[Test build #79924 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/79924/testReport)** for PR 18503 at commit [`0159701`](https://github.com/apache/spark/commit/01597010145f7769506cfa51c0450a2db779fc07).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:

    https://github.com/apache/spark/pull/18503#discussion_r128333705
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/HDFSBackedStateStoreProvider.scala ---
    @@ -413,21 +417,25 @@ private[state] class HDFSBackedStateStoreProvider extends StateStoreProvider wit
               throw new IOException(
                 s"Error reading snapshot file $fileToRead of $this: key size cannot be $keySize")
             } else {
    -          val keyRowBuffer = new Array[Byte](keySize)
    +          // If key size in an existing file is not a multiple of 8, round it to multiple of 8
    +          val keyAllocationSize = ((keySize + 7) / 8) * 8
    +          val keyRowBuffer = new Array[Byte](keyAllocationSize)
               ByteStreams.readFully(input, keyRowBuffer, 0, keySize)
     
               val keyRow = new UnsafeRow(keySchema.fields.length)
    -          keyRow.pointTo(keyRowBuffer, keySize)
    +          keyRow.pointTo(keyRowBuffer, keyAllocationSize)
     
               val valueSize = input.readInt()
               if (valueSize < 0) {
                 throw new IOException(
                   s"Error reading snapshot file $fileToRead of $this: value size cannot be $valueSize")
               } else {
    -            val valueRowBuffer = new Array[Byte](valueSize)
    +            // If value size in an existing file is not a multiple of 8, round it to multiple of 8
    +            val valueAllocationSize = ((valueSize + 7) / 8) * 8
    --- End diff --
    
    @cloud-fan what do you think?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

Posted by tdas <gi...@git.apache.org>.
Github user tdas commented on a diff in the pull request:

    https://github.com/apache/spark/pull/18503#discussion_r126038199
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/HDFSBackedStateStoreProvider.scala ---
    @@ -350,20 +350,24 @@ private[state] class HDFSBackedStateStoreProvider extends StateStoreProvider wit
               throw new IOException(
                 s"Error reading delta file $fileToRead of $this: key size cannot be $keySize")
             } else {
    -          val keyRowBuffer = new Array[Byte](keySize)
    +          // If key size in an existing file is not a multiple of 8, round it to multiple of 8
    +          val keyAllocationSize = ((keySize + 7) / 8) * 8
    +          val keyRowBuffer = new Array[Byte](keyAllocationSize)
    --- End diff --
    
    I agree. If we are having to regenerate, then we are breaking something. And we must not since we have been guaranteeing backward compatibility. If @cloud-fan claims that all unsafe rows except RowBasedKeyValueBatch should have a size multiple of 8, then we need to understand what is going on; why does reading the checkpoint files fail.  


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:

    https://github.com/apache/spark/pull/18503#discussion_r128156305
  
    --- Diff: sql/core/src/test/scala/org/apache/spark/sql/streaming/StreamSuite.scala ---
    @@ -479,6 +479,61 @@ class StreamSuite extends StreamTest {
           CheckAnswer((1, 2), (2, 2), (3, 2)))
       }
     
    +  testQuietly("store to and recover from a checkpoint") {
    --- End diff --
    
    This test also checks whether the length of checkpoints is a multiple of 8 or not. Does it make sense? Or, is there any other test suite to check the length?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

Posted by tdas <gi...@git.apache.org>.
Github user tdas commented on a diff in the pull request:

    https://github.com/apache/spark/pull/18503#discussion_r128408410
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/HDFSBackedStateStoreProvider.scala ---
    @@ -363,7 +363,8 @@ private[state] class HDFSBackedStateStoreProvider extends StateStoreProvider wit
                 val valueRowBuffer = new Array[Byte](valueSize)
                 ByteStreams.readFully(input, valueRowBuffer, 0, valueSize)
                 val valueRow = new UnsafeRow(valueSchema.fields.length)
    -            valueRow.pointTo(valueRowBuffer, valueSize)
    +            // If valueSize in existing file is not multiple of 8, round it down to multiple of 8
    +            valueRow.pointTo(valueRowBuffer, (valueSize / 8) * 8)
    --- End diff --
    
    This isnt rounding. This essentially floor to the multiple of 8. 
    @cloud-fan  is this safe to do with ANY row generated in earlier Spark 2.0 - 2.2? I want to be 100% sure.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:

    https://github.com/apache/spark/pull/18503#discussion_r129257673
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/HDFSBackedStateStoreProvider.scala ---
    @@ -363,7 +363,8 @@ private[state] class HDFSBackedStateStoreProvider extends StateStoreProvider wit
                 val valueRowBuffer = new Array[Byte](valueSize)
                 ByteStreams.readFully(input, valueRowBuffer, 0, valueSize)
                 val valueRow = new UnsafeRow(valueSchema.fields.length)
    -            valueRow.pointTo(valueRowBuffer, valueSize)
    +            // If valueSize in existing file is not multiple of 8, round it down to multiple of 8
    +            valueRow.pointTo(valueRowBuffer, (valueSize / 8) * 8)
    --- End diff --
    
    Sure, added more comments.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [WIP][SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a m...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

Posted by tdas <gi...@git.apache.org>.
Github user tdas commented on a diff in the pull request:

    https://github.com/apache/spark/pull/18503#discussion_r128122095
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/HDFSBackedStateStoreProvider.scala ---
    @@ -413,21 +417,25 @@ private[state] class HDFSBackedStateStoreProvider extends StateStoreProvider wit
               throw new IOException(
                 s"Error reading snapshot file $fileToRead of $this: key size cannot be $keySize")
             } else {
    -          val keyRowBuffer = new Array[Byte](keySize)
    +          // If key size in an existing file is not a multiple of 8, round it to multiple of 8
    +          val keyAllocationSize = ((keySize + 7) / 8) * 8
    +          val keyRowBuffer = new Array[Byte](keyAllocationSize)
               ByteStreams.readFully(input, keyRowBuffer, 0, keySize)
     
               val keyRow = new UnsafeRow(keySchema.fields.length)
    -          keyRow.pointTo(keyRowBuffer, keySize)
    +          keyRow.pointTo(keyRowBuffer, keyAllocationSize)
     
               val valueSize = input.readInt()
               if (valueSize < 0) {
                 throw new IOException(
                   s"Error reading snapshot file $fileToRead of $this: value size cannot be $valueSize")
               } else {
    -            val valueRowBuffer = new Array[Byte](valueSize)
    +            // If value size in an existing file is not a multiple of 8, round it to multiple of 8
    +            val valueAllocationSize = ((valueSize + 7) / 8) * 8
    --- End diff --
    
    Can this be made into utility function inside UnsafeRow? Seems like this sort of adjustment should not be a concerns of external users of UnsafeRow.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:

    https://github.com/apache/spark/pull/18503
  
    retest this please


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [WIP][SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a m...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

Posted by zsxwing <gi...@git.apache.org>.
Github user zsxwing commented on a diff in the pull request:

    https://github.com/apache/spark/pull/18503#discussion_r126035228
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/HDFSBackedStateStoreProvider.scala ---
    @@ -350,20 +350,24 @@ private[state] class HDFSBackedStateStoreProvider extends StateStoreProvider wit
               throw new IOException(
                 s"Error reading delta file $fileToRead of $this: key size cannot be $keySize")
             } else {
    -          val keyRowBuffer = new Array[Byte](keySize)
    +          // If key size in an existing file is not a multiple of 8, round it to multiple of 8
    +          val keyAllocationSize = ((keySize + 7) / 8) * 8
    +          val keyRowBuffer = new Array[Byte](keyAllocationSize)
    --- End diff --
    
    > cc @kunalkhamar @zsxwing is it ok if we re-generate these files?
    
    If we need to re-generate these files, it means we probably break backward compatibility. I think we need to support reading from checkpoint files generated by old Spark versions.
    
    /cc @marmbrus 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

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/18503#discussion_r125889190
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/HDFSBackedStateStoreProvider.scala ---
    @@ -350,20 +350,24 @@ private[state] class HDFSBackedStateStoreProvider extends StateStoreProvider wit
               throw new IOException(
                 s"Error reading delta file $fileToRead of $this: key size cannot be $keySize")
             } else {
    -          val keyRowBuffer = new Array[Byte](keySize)
    +          // If key size in an existing file is not a multiple of 8, round it to multiple of 8
    +          val keyAllocationSize = ((keySize + 7) / 8) * 8
    +          val keyRowBuffer = new Array[Byte](keyAllocationSize)
    --- End diff --
    
    I think all unsafe rows have the size of a multiple of 8, except `RowBasedKeyValueBatch` in the previous code. So I'm wondering how the state store can have unsafe rows with wrong size, does state store use `RowBasedKeyValueBatch`?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a...

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark issue #18503: [SPARK-21271][SQL] Ensure Unsafe.sizeInBytes is a multip...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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