Skip to content

Conversation

@wangyum
Copy link
Member

@wangyum wangyum commented Oct 5, 2018

What changes were proposed in this pull request?

Only test these 4 cases is enough:

// Standard mode, 1 <= precision <= 9, writes as INT32
case false if precision <= Decimal.MAX_INT_DIGITS => int32Writer
// Standard mode, 10 <= precision <= 18, writes as INT64
case false if precision <= Decimal.MAX_LONG_DIGITS => int64Writer
// Legacy mode, 1 <= precision <= 18, writes as FIXED_LEN_BYTE_ARRAY
case true if precision <= Decimal.MAX_LONG_DIGITS => binaryWriterUsingUnscaledLong
// Either standard or legacy mode, 19 <= precision <= 38, writes as FIXED_LEN_BYTE_ARRAY
case _ => binaryWriterUsingUnscaledBytes

How was this patch tested?

Manual tests on my local machine.
before:

- filter pushdown - decimal (13 seconds, 683 milliseconds)

after:

- filter pushdown - decimal (9 seconds, 713 milliseconds)

@SparkQA
Copy link

SparkQA commented Oct 5, 2018

Test build #96962 has finished for PR 22636 at commit e40d79c.

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

@wangyum
Copy link
Member Author

wangyum commented Oct 5, 2018

retest this please

@SparkQA
Copy link

SparkQA commented Oct 5, 2018

Test build #96965 has finished for PR 22636 at commit e40d79c.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@wangyum
Copy link
Member Author

wangyum commented Oct 5, 2018

retest this please

@SparkQA
Copy link

SparkQA commented Oct 5, 2018

Test build #96976 has finished for PR 22636 at commit e40d79c.

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

@gatorsmile
Copy link
Member

The time reduction is not obvious. Let us keep this unchanged?

@HyukjinKwon
Copy link
Member

Yea, it is not obvious and only few seconds - might not be so worth. But looks improvement because it fixes the test cases to test what the previous PR targeted. Wouldn't it be better just to go ahead rather then close this since the PR is already open?

@wangyum wangyum closed this Oct 12, 2018
@HyukjinKwon
Copy link
Member

@wangyum, can you reopen this? I don't think we should just close this. Fixing tests to actually test what the previous change targeted sounds definitely an improvement although it's minor. I wouldn't encourage to fix those but at least I would go ahead for this one since the fix is already proposed.

@wangyum wangyum reopened this Oct 14, 2018
@SparkQA
Copy link

SparkQA commented Oct 14, 2018

Test build #97362 has finished for PR 22636 at commit e40d79c.

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

@HyukjinKwon
Copy link
Member

Merged to master.

@asfgit asfgit closed this in 5c7f6b6 Oct 16, 2018
jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
…ime costs in Jenkins

## What changes were proposed in this pull request?

Only test these 4 cases is enough:
https://github.com/apache/spark/blob/be2238fb502b0f49a8a1baa6da9bc3e99540b40e/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetWriteSupport.scala#L269-L279

## How was this patch tested?

Manual tests on my local machine.
before:
```
- filter pushdown - decimal (13 seconds, 683 milliseconds)
```
after:
```
- filter pushdown - decimal (9 seconds, 713 milliseconds)
```

Closes apache#22636 from wangyum/SPARK-25629.

Authored-by: Yuming Wang <yumwang@ebay.com>
Signed-off-by: hyukjinkwon <gurwls223@apache.org>
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