[SPARK-27160][SQL] Fix DecimalType when building orc filters #24092

da-liii · 2019-03-14T13:34:45Z

What changes were proposed in this pull request?

DecimalType Literal should not be casted to Long.

eg. For df.filter("x < 3.14"), assuming df (x in DecimalType) reads from a ORC table and uses the native ORC reader with predicate push down enabled, we will push down the x < 3.14 predicate to the ORC reader via a SearchArgument.

OrcFilters will construct the SearchArgument, but not handle the DecimalType correctly.

The previous impl will construct x < 3 from x < 3.14.

How was this patch tested?

$ sbt
> sql/testOnly *OrcFilterSuite
> sql/testOnly *OrcQuerySuite -- -z "27160"

da-liii · 2019-03-14T13:36:52Z

@dongjoon-hyun

Please review! I do think this should be backported to 2.4.x. And I will add unit tests later.

cloud-fan · 2019-03-14T13:43:24Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/orc/OrcFilters.scala

@@ -136,10 +137,7 @@ private[sql] object OrcFilters {
    case FloatType | DoubleType =>
      value.asInstanceOf[Number].doubleValue()
    case _: DecimalType =>
-      val decimal = value.asInstanceOf[java.math.BigDecimal]
-      val decimalWritable = new HiveDecimalWritable(decimal.longValue)
-      decimalWritable.mutateEnforcePrecisionScale(decimal.precision, decimal.scale)


why do we remove this line?

This will potentially result in an RuntimeException in the hashCode method of HiveDecimalWritabble.

Can you provide more details? I'm not convinced that we can skip it.

https://github.com/apache/orc/blob/d1d2a37842087501933d594a71cd1f5f5c4a5a9e/java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java#L784

There are similar conversions in the ORC project.

@dongjoon-hyun Why do we need mutateEnforcePrecisionScale ?

@cloud-fan I have an ORC file to reproduce the RuntimeException with mutateEnforcePrecisionScale. But have not come up with a nice unit test yet.

@sadhen and @cloud-fan .
Yes, Line 140 was the bug and mutateEnforcePrecisionScale just amended the scale and precision for the HiveDecimalWriter(long) case. We can remove this in the new code.

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/orc/OrcFilters.scala

SparkQA · 2019-03-14T17:33:46Z

Test build #103505 has finished for PR 24092 at commit 591c3f4.

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

SparkQA · 2019-03-14T18:23:16Z

Test build #103493 has finished for PR 24092 at commit 4be6d82.

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

dongjoon-hyun · 2019-03-14T21:45:38Z

Thank you for pinging me, @sadhen . I'll take a look today!

dongjoon-hyun · 2019-03-14T21:50:12Z

Retest this please.

dongjoon-hyun · 2019-03-14T22:31:13Z

cc @dbtsai since he is a release manager.

cloud-fan · 2019-03-15T01:50:38Z

Can we add an end-to-end test to demonstrate the correctness bug?

SparkQA · 2019-03-15T02:04:30Z

Test build #103517 has finished for PR 24092 at commit 591c3f4.

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

dongjoon-hyun · 2019-03-15T02:45:58Z

+1 for @cloud-fan 's opinion.
@sadhen , could you add another test case for that?

da-liii · 2019-03-15T03:19:56Z

@dongjoon-hyun @cloud-fan

Do you mean generating an ORC file with DecimalType, and read it using the native reader with predicate push down?

dongjoon-hyun · 2019-03-15T03:29:34Z

Yes, right, @sadhen .

da-liii · 2019-03-19T12:06:20Z

@cloud-fan @dongjoon-hyun Please review again.

sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/orc/OrcQuerySuite.scala

dongjoon-hyun · 2019-03-19T16:54:28Z

Retest this please.

dongjoon-hyun

+1, LGTM (Pending Jenkins)

SparkQA · 2019-03-19T18:09:02Z

Test build #103681 has started for PR 24092 at commit 08e158a.

dongjoon-hyun · 2019-03-19T22:26:27Z

Retest this please.

SparkQA · 2019-03-20T02:41:09Z

Test build #103696 has finished for PR 24092 at commit 08e158a.

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

dongjoon-hyun · 2019-03-20T03:28:10Z

Merged to master.

dongjoon-hyun · 2019-03-20T03:30:05Z

Hi, @sadhen . There is a conflict at branch-2.4. Could you make a PR against branch-2.4 please?

DecimalType Literal should not be casted to Long. eg. For `df.filter("x < 3.14")`, assuming df (x in DecimalType) reads from a ORC table and uses the native ORC reader with predicate push down enabled, we will push down the `x < 3.14` predicate to the ORC reader via a SearchArgument. OrcFilters will construct the SearchArgument, but not handle the DecimalType correctly. The previous impl will construct `x < 3` from `x < 3.14`. ``` $ sbt > sql/testOnly *OrcFilterSuite > sql/testOnly *OrcQuerySuite -- -z "27160" ``` Closes #24092 from sadhen/spark27160. Authored-by: Darcy Shen <sadhen@zoho.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

dongjoon-hyun · 2019-03-23T17:35:36Z

Since @sadhen looks busy and RC8 fails, I resolved the conflicts and back port this to branch-2.4. I built and tested locally.

cc @dbtsai since he is a release manager for 2.4.1.

DecimalType Literal should not be casted to Long. eg. For `df.filter("x < 3.14")`, assuming df (x in DecimalType) reads from a ORC table and uses the native ORC reader with predicate push down enabled, we will push down the `x < 3.14` predicate to the ORC reader via a SearchArgument. OrcFilters will construct the SearchArgument, but not handle the DecimalType correctly. The previous impl will construct `x < 3` from `x < 3.14`. ``` $ sbt > sql/testOnly *OrcFilterSuite > sql/testOnly *OrcQuerySuite -- -z "27160" ``` Closes #24092 from sadhen/spark27160. Authored-by: Darcy Shen <sadhen@zoho.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com> (cherry picked from commit f3ba73a) Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

dongjoon-hyun · 2019-03-23T17:59:22Z

Also, I tested and landed this on branch-2.3 for 2.3.4.

DecimalType Literal should not be casted to Long. eg. For `df.filter("x < 3.14")`, assuming df (x in DecimalType) reads from a ORC table and uses the native ORC reader with predicate push down enabled, we will push down the `x < 3.14` predicate to the ORC reader via a SearchArgument. OrcFilters will construct the SearchArgument, but not handle the DecimalType correctly. The previous impl will construct `x < 3` from `x < 3.14`. ``` $ sbt > sql/testOnly *OrcFilterSuite > sql/testOnly *OrcQuerySuite -- -z "27160" ``` Closes apache#24092 from sadhen/spark27160. Authored-by: Darcy Shen <sadhen@zoho.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com> Conflicts: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/orc/OrcFilters.scala

DecimalType Literal should not be casted to Long. eg. For `df.filter("x < 3.14")`, assuming df (x in DecimalType) reads from a ORC table and uses the native ORC reader with predicate push down enabled, we will push down the `x < 3.14` predicate to the ORC reader via a SearchArgument. OrcFilters will construct the SearchArgument, but not handle the DecimalType correctly. The previous impl will construct `x < 3` from `x < 3.14`. ``` $ sbt > sql/testOnly *OrcFilterSuite > sql/testOnly *OrcQuerySuite -- -z "27160" ``` Closes apache#24092 from sadhen/spark27160. Authored-by: Darcy Shen <sadhen@zoho.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

DecimalType Literal should not be casted to Long. eg. For `df.filter("x < 3.14")`, assuming df (x in DecimalType) reads from a ORC table and uses the native ORC reader with predicate push down enabled, we will push down the `x < 3.14` predicate to the ORC reader via a SearchArgument. OrcFilters will construct the SearchArgument, but not handle the DecimalType correctly. The previous impl will construct `x < 3` from `x < 3.14`. ``` $ sbt > sql/testOnly *OrcFilterSuite > sql/testOnly *OrcQuerySuite -- -z "27160" ``` Closes apache#24092 from sadhen/spark27160. Authored-by: Darcy Shen <sadhen@zoho.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com> (cherry picked from commit f3ba73a) Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

Fix DecimalType literal casting

4be6d82

cloud-fan reviewed Mar 14, 2019

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/orc/OrcFilters.scala Show resolved Hide resolved

da-liii changed the title ~~[SPARK-27160][SQL] Fix DecimalType literal casting~~ [WIP][SPARK-27160][SQL] Fix DecimalType literal casting Mar 14, 2019

Add unit tests

591c3f4

da-liii changed the title ~~[WIP][SPARK-27160][SQL] Fix DecimalType literal casting~~ [SPARK-27160][SQL] Fix DecimalType literal casting Mar 14, 2019

da-liii added 2 commits March 19, 2019 19:48

Add end to end unit tests

69551cc

More cases

f6089c9

cloud-fan reviewed Mar 19, 2019

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/orc/OrcQuerySuite.scala Outdated Show resolved Hide resolved

cloud-fan changed the title ~~[SPARK-27160][SQL] Fix DecimalType literal casting~~ [SPARK-27160][SQL] Fix DecimalType when building orc filters Mar 19, 2019

da-liii added 2 commits March 19, 2019 20:35

Minor improve

8621980

Remove unused import

08e158a

dongjoon-hyun approved these changes Mar 19, 2019

View reviewed changes

dongjoon-hyun closed this in 9a43852 Mar 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-27160][SQL] Fix DecimalType when building orc filters #24092

[SPARK-27160][SQL] Fix DecimalType when building orc filters #24092

da-liii commented Mar 14, 2019 •

edited

Loading

da-liii commented Mar 14, 2019

cloud-fan Mar 14, 2019

da-liii Mar 14, 2019 •

edited

Loading

cloud-fan Mar 14, 2019

da-liii Mar 14, 2019

da-liii Mar 14, 2019

dongjoon-hyun Mar 14, 2019 •

edited

Loading

SparkQA commented Mar 14, 2019

SparkQA commented Mar 14, 2019

dongjoon-hyun commented Mar 14, 2019

dongjoon-hyun commented Mar 14, 2019

dongjoon-hyun commented Mar 14, 2019

cloud-fan commented Mar 15, 2019

SparkQA commented Mar 15, 2019

dongjoon-hyun commented Mar 15, 2019

da-liii commented Mar 15, 2019

dongjoon-hyun commented Mar 15, 2019

da-liii commented Mar 19, 2019

dongjoon-hyun commented Mar 19, 2019

dongjoon-hyun left a comment •

edited

Loading

SparkQA commented Mar 19, 2019

dongjoon-hyun commented Mar 19, 2019

SparkQA commented Mar 20, 2019

dongjoon-hyun commented Mar 20, 2019

dongjoon-hyun commented Mar 20, 2019

dongjoon-hyun commented Mar 23, 2019 •

edited

Loading

dongjoon-hyun commented Mar 23, 2019

[SPARK-27160][SQL] Fix DecimalType when building orc filters #24092

[SPARK-27160][SQL] Fix DecimalType when building orc filters #24092

Conversation

da-liii commented Mar 14, 2019 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

da-liii commented Mar 14, 2019

cloud-fan Mar 14, 2019

Choose a reason for hiding this comment

da-liii Mar 14, 2019 • edited Loading

Choose a reason for hiding this comment

cloud-fan Mar 14, 2019

Choose a reason for hiding this comment

da-liii Mar 14, 2019

Choose a reason for hiding this comment

da-liii Mar 14, 2019

Choose a reason for hiding this comment

dongjoon-hyun Mar 14, 2019 • edited Loading

Choose a reason for hiding this comment

SparkQA commented Mar 14, 2019

SparkQA commented Mar 14, 2019

dongjoon-hyun commented Mar 14, 2019

dongjoon-hyun commented Mar 14, 2019

dongjoon-hyun commented Mar 14, 2019

cloud-fan commented Mar 15, 2019

SparkQA commented Mar 15, 2019

dongjoon-hyun commented Mar 15, 2019

da-liii commented Mar 15, 2019

dongjoon-hyun commented Mar 15, 2019

da-liii commented Mar 19, 2019

dongjoon-hyun commented Mar 19, 2019

dongjoon-hyun left a comment • edited Loading

Choose a reason for hiding this comment

SparkQA commented Mar 19, 2019

dongjoon-hyun commented Mar 19, 2019

SparkQA commented Mar 20, 2019

dongjoon-hyun commented Mar 20, 2019

dongjoon-hyun commented Mar 20, 2019

dongjoon-hyun commented Mar 23, 2019 • edited Loading

dongjoon-hyun commented Mar 23, 2019

da-liii commented Mar 14, 2019 •

edited

Loading

da-liii Mar 14, 2019 •

edited

Loading

dongjoon-hyun Mar 14, 2019 •

edited

Loading

dongjoon-hyun left a comment •

edited

Loading

dongjoon-hyun commented Mar 23, 2019 •

edited

Loading