Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[SPARK-23882][Core] UTF8StringSuite.writeToOutputStreamUnderflow() is not expected to be supported #20995

Closed
wants to merge 3 commits into from

Conversation

kiszk
Copy link
Member

@kiszk kiszk commented Apr 6, 2018

What changes were proposed in this pull request?

This PR excludes an existing UT writeToOutputStreamUnderflow() in UTF8StringSuite.

As discussed here, the behavior of this test looks surprising. This test seems to access metadata area of the JVM object where is reserved by Platform.BYTE_ARRAY_OFFSET.

This test is introduced thru #16089 by @NathanHowell. More specifically, the commit Improve test coverage of UTFString.write introduced this UT. However, I cannot find any discussion about this UT.

I think that it would be good to exclude this UT.

  public void writeToOutputStreamUnderflow() throws IOException {
    // offset underflow is apparently supported?
    final ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
    final byte[] test = "01234567".getBytes(StandardCharsets.UTF_8);

    for (int i = 1; i <= Platform.BYTE_ARRAY_OFFSET; ++i) {
      new UTF8String(
        new ByteArrayMemoryBlock(test, Platform.BYTE_ARRAY_OFFSET - i, test.length + i))
          .writeTo(outputStream);
      final ByteBuffer buffer = ByteBuffer.wrap(outputStream.toByteArray(), i, test.length);
      assertEquals("01234567", StandardCharsets.UTF_8.decode(buffer).toString());
      outputStream.reset();
    }
  }

How was this patch tested?

Existing UTs

@kiszk kiszk changed the title UTF8StringSuite.writeToOutputStreamUnderflow() is not expected to be supported [SPARK-23882][Core] UTF8StringSuite.writeToOutputStreamUnderflow() is not expected to be supported Apr 6, 2018
@kiszk
Copy link
Member Author

kiszk commented Apr 6, 2018

cc @NathanHowell, @cloud-fan, @viirya

@SparkQA
Copy link

SparkQA commented Apr 6, 2018

Test build #88978 has finished for PR 20995 at commit b47b861.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@hvanhovell
Copy link
Contributor

@kiszk let's just remove the test. It should have never been added in the first place.

@SparkQA
Copy link

SparkQA commented Apr 6, 2018

Test build #88979 has finished for PR 20995 at commit 04e9d5f.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Apr 6, 2018

Test build #88984 has finished for PR 20995 at commit d2dc70f.

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

@hvanhovell
Copy link
Contributor

LGTM - merging to master. Thanks!

@asfgit asfgit closed this in c926acf Apr 6, 2018
@ueshin
Copy link
Member

ueshin commented Apr 6, 2018

Seems like this broke lint-java.

[ERROR] src/test/java/org/apache/spark/unsafe/types/UTF8StringSuite.java:[28,8] (imports) UnusedImports: Unused import - org.apache.spark.unsafe.Platform.

@kiszk Could you fix it? Thanks!

@kiszk
Copy link
Member Author

kiszk commented Apr 7, 2018

@ueshin, sorry for my mistake again. I will fix this at #20871

robert3005 pushed a commit to palantir/spark that referenced this pull request Apr 7, 2018
… not expected to be supported

## What changes were proposed in this pull request?

This PR excludes an existing UT [`writeToOutputStreamUnderflow()`](https://github.com/apache/spark/blob/master/common/unsafe/src/test/java/org/apache/spark/unsafe/types/UTF8StringSuite.java#L519-L532) in `UTF8StringSuite`.

As discussed [here](apache#19222 (comment)), the behavior of this test looks surprising. This test seems to access metadata area of the JVM object where is reserved by `Platform.BYTE_ARRAY_OFFSET`.

This test is introduced thru apache#16089 by NathanHowell. More specifically, [the commit](apache@27c102d) `Improve test coverage of UTFString.write` introduced this UT. However, I cannot find any discussion about this UT.

I think that it would be good to exclude this UT.

```java
  public void writeToOutputStreamUnderflow() throws IOException {
    // offset underflow is apparently supported?
    final ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
    final byte[] test = "01234567".getBytes(StandardCharsets.UTF_8);

    for (int i = 1; i <= Platform.BYTE_ARRAY_OFFSET; ++i) {
      new UTF8String(
        new ByteArrayMemoryBlock(test, Platform.BYTE_ARRAY_OFFSET - i, test.length + i))
          .writeTo(outputStream);
      final ByteBuffer buffer = ByteBuffer.wrap(outputStream.toByteArray(), i, test.length);
      assertEquals("01234567", StandardCharsets.UTF_8.decode(buffer).toString());
      outputStream.reset();
    }
  }
```

## How was this patch tested?

Existing UTs

Author: Kazuaki Ishizaki <[email protected]>

Closes apache#20995 from kiszk/SPARK-23882.
@kiszk
Copy link
Member Author

kiszk commented Apr 7, 2018

@ueshin Finally, I found better PR to fix this issue.

ghost pushed a commit to dbtsai/spark that referenced this pull request Apr 8, 2018
…-related tests

## What changes were proposed in this pull request?

This PR improves test coverage in `UTF8StringSuite` and code efficiency in `UTF8StringPropertyCheckSuite`.

This PR also fixes lint-java issue in `UTF8StringSuite` reported at [here](apache#20995 (comment))

```[ERROR] src/test/java/org/apache/spark/unsafe/types/UTF8StringSuite.java:[28,8] (imports) UnusedImports: Unused import - org.apache.spark.unsafe.Platform.```

## How was this patch tested?

Existing UT

Author: Kazuaki Ishizaki <[email protected]>

Closes apache#21000 from kiszk/SPARK-23892.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants