[SPARK-17495] [SQL] Support Decimal type in Hive-hash #17056

tejasapatil · 2017-02-24T07:38:47Z

What changes were proposed in this pull request?

Hive hash to support Decimal datatype. Hive internally normalises decimals and I have ported that logic as-is to HiveHash.

How was this patch tested?

Added unit tests

tejasapatil · 2017-02-24T07:39:20Z

ok to test

SparkQA · 2017-02-24T07:42:37Z

Test build #73410 has started for PR 17056 at commit a378b3e.

## What changes were proposed in this pull request? This PR adds tests hive-hash by comparing the outputs generated against Hive 1.2.1. Following datatypes are covered by this PR: - null - boolean - byte - short - int - long - float - double - string - array - map - struct Datatypes that I have _NOT_ covered but I will work on separately are: - Decimal (handled separately in #17056) - TimestampType - DateType - CalendarIntervalType ## How was this patch tested? NA Author: Tejas Patil <[email protected]> Closes #17049 from tejasapatil/SPARK-17495_remaining_types.

tejasapatil · 2017-02-25T02:42:50Z

retest this please

SparkQA · 2017-02-25T04:46:05Z

Test build #73460 has finished for PR 17056 at commit 8595305.

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

tejasapatil · 2017-02-26T00:06:35Z

@cloud-fan @gatorsmile : can you please review this PR ?

gatorsmile · 2017-02-26T06:17:36Z

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/HashExpressionsSuite.scala

A quick question: these expected values are got from Hive?

I did a quick check. Most are right, but some of them do not match

These were generated over Hive 1.2.1.

gatorsmile · 2017-02-26T07:53:44Z

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/HashExpressionsSuite.scala

hive> select HASH(CAST("-18446744073709001000" AS DECIMAL(38,19))); OK 0 Time taken: 0.035 seconds, Fetched: 1 row(s)

I figured out the problem was with the test case being not looking at the result of decimal.changePrecision. Fixed.

## What changes were proposed in this pull request? This PR adds tests hive-hash by comparing the outputs generated against Hive 1.2.1. Following datatypes are covered by this PR: - null - boolean - byte - short - int - long - float - double - string - array - map - struct Datatypes that I have _NOT_ covered but I will work on separately are: - Decimal (handled separately in apache#17056) - TimestampType - DateType - CalendarIntervalType ## How was this patch tested? NA Author: Tejas Patil <[email protected]> Closes apache#17049 from tejasapatil/SPARK-17495_remaining_types.

tejasapatil · 2017-02-28T17:02:27Z

Jenkins test this please

SparkQA · 2017-02-28T18:59:58Z

Test build #73595 has finished for PR 17056 at commit 2387515.

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

tejasapatil · 2017-02-28T19:01:09Z

cc @cloud-fan @gatorsmile

SparkQA · 2017-02-28T19:11:54Z

Test build #73596 has finished for PR 17056 at commit 2387515.

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

cloud-fan · 2017-02-28T19:49:12Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/hash.scala

where do we use ctx and d?

They both aren't used but are a part of the method signature since the default impl in abstract class needs those :

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/hash.scala

Line 321 in 3e40f6c

protected def genHashDecimal(

cloud-fan · 2017-02-28T19:50:50Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/hash.scala

${classOf[HiveHashFunction].getName}.normalizeDecimal

HiveHashFunction is an object so cannot do classOf[]. Tried HiveHashFunction.getClass.getName as per other places in the codebase

cloud-fan · 2017-02-28T19:53:48Z

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/HashExpressionsSuite.scala

hmm it's hard to guarantee that we can produce same hash value as hive, can we run hive in the test and compare the result with spark?

The expected values are generated using hive 1.2.1. My original approach was to depend on Hive for generating expected values but as per discussion in a related PR, I was suggested to hardcode expected values. The main point being reduce dependency on Hive

SparkQA · 2017-02-28T23:40:58Z

Test build #73610 has finished for PR 17056 at commit 428a9a4.

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

gatorsmile · 2017-03-01T06:09:33Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/hash.scala

gatorsmile · 2017-03-01T06:10:05Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/hash.scala

Nit: if (input == null) return null

gatorsmile · 2017-03-01T06:13:23Z

Let me manually check whether the results are consistent with Hive.

tejasapatil · 2017-03-01T07:06:17Z

@gatorsmile : I really appreciate your help in reviewing this PR to the extent that you are manually checking the hashes over Hive. If you haven't already embarked on that, here is the set of hive queries corresponding to the test case in the PR which you can easily copy paste:

SELECT HASH(CAST(18BD AS DECIMAL(38, 0)));
SELECT HASH(CAST(-18BD AS DECIMAL(38, 0)));
SELECT HASH(CAST(-18BD AS DECIMAL(38, 12)));
SELECT HASH(CAST(18446744073709001000BD AS DECIMAL(38, 19)));
SELECT HASH(CAST(-18446744073709001000BD AS DECIMAL(38, 22)));
SELECT HASH(CAST(-18446744073709001000BD AS DECIMAL(38, 3)));
SELECT HASH(CAST(18446744073709001000BD AS DECIMAL(38, 4)));
SELECT HASH(CAST(9223372036854775807BD AS DECIMAL(38, 4)));
SELECT HASH(CAST(-9223372036854775807BD AS DECIMAL(38, 5)));
SELECT HASH(CAST(00000.00000000000BD AS DECIMAL(38, 34)));
SELECT HASH(CAST(-00000.00000000000BD AS DECIMAL(38, 11)));
SELECT HASH(CAST(123456.1234567890BD AS DECIMAL(38, 2)));
SELECT HASH(CAST(123456.1234567890BD AS DECIMAL(38, 20)));
SELECT HASH(CAST(123456.1234567890BD AS DECIMAL(38, 10)));
SELECT HASH(CAST(-123456.1234567890BD AS DECIMAL(38, 10)));
SELECT HASH(CAST(123456.1234567890BD AS DECIMAL(38, 0)));
SELECT HASH(CAST(-123456.1234567890BD AS DECIMAL(38, 0)));
SELECT HASH(CAST(123456.1234567890BD AS DECIMAL(38, 20)));
SELECT HASH(CAST(-123456.1234567890BD AS DECIMAL(38, 20)));
SELECT HASH(CAST(123456.123456789012345678901234567890BD AS DECIMAL(38, 0)));
SELECT HASH(CAST(-123456.123456789012345678901234567890BD AS DECIMAL(38, 0)));
SELECT HASH(CAST(123456.123456789012345678901234567890BD AS DECIMAL(38, 10)));
SELECT HASH(CAST(-123456.123456789012345678901234567890BD AS DECIMAL(38, 10)));
SELECT HASH(CAST(123456.123456789012345678901234567890BD AS DECIMAL(38, 20)));
SELECT HASH(CAST(-123456.123456789012345678901234567890BD AS DECIMAL(38, 20)));
SELECT HASH(CAST(123456.123456789012345678901234567890BD AS DECIMAL(38, 30)));
SELECT HASH(CAST(-123456.123456789012345678901234567890BD AS DECIMAL(38, 30)));
SELECT HASH(CAST(123456.123456789012345678901234567890BD AS DECIMAL(38, 31)));

gatorsmile · 2017-03-01T07:38:54Z

Thank you! I checked Hive 2.1. It has the exactly same hash values.

gatorsmile · 2017-03-01T07:50:55Z

LGTM. cc @cloud-fan for final signing-off

SparkQA · 2017-03-01T08:41:41Z

Test build #73670 has finished for PR 17056 at commit c0c8390.

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

tejasapatil · 2017-03-03T18:27:27Z

@cloud-fan ping !!

cloud-fan · 2017-03-03T19:10:13Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/hash.scala

+  private val HiveDecimalMaxScale = 38
+
+  // Mimics normalization done for decimals in Hive at HiveDecimalV1.normalize()
+  def normalizeDecimal(input: BigDecimal, allowRounding: Boolean): BigDecimal = {


allowRounding will never be false?

removed that param

cloud-fan · 2017-03-03T19:12:19Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/hash.scala

    HiveHasher.hashUnsafeBytes(base, offset, len)
  }

+  private val HiveDecimalMaxPrecision = 38


nit: HIVE_DECIMAL_MAX_PRECISION

SparkQA · 2017-03-06T00:31:22Z

Test build #73945 has finished for PR 17056 at commit 65a09e9.

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

cloud-fan · 2017-03-06T08:20:32Z

LGTM(if tests pass)

SparkQA · 2017-03-06T17:30:30Z

Test build #74018 has finished for PR 17056 at commit 7c0b6c8.

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

cloud-fan · 2017-03-06T18:18:48Z

thanks, merging to master!

tejasapatil mentioned this pull request Feb 24, 2017

[SPARK-17495] [SQL] Add more tests for hive hash #17049

Closed

tejasapatil force-pushed the SPARK-17495_decimal branch from a378b3e to 8595305 Compare February 25, 2017 02:42

gatorsmile reviewed Feb 26, 2017

View reviewed changes

cloud-fan reviewed Feb 28, 2017

View reviewed changes

gatorsmile reviewed Mar 1, 2017

View reviewed changes

tejasapatil added 4 commits February 28, 2017 22:26

[SPARK-17495] [SQL] Support Decimal type

d16ae0f

Handle un-successfull precision change in test cases

7bc4eab

review comment apache#2

2e4c333

review round apache#3

c0c8390

tejasapatil force-pushed the SPARK-17495_decimal branch from 428a9a4 to c0c8390 Compare March 1, 2017 06:32

cloud-fan reviewed Mar 3, 2017

View reviewed changes

tejasapatil added 2 commits March 5, 2017 14:57

review apache#4

f60ffb9

checkstlye

65a09e9

fix generated code

7c0b6c8

asfgit closed this in 2a0bc86 Mar 6, 2017

tejasapatil deleted the SPARK-17495_decimal branch March 6, 2017 19:54

[SPARK-17495] [SQL] Support Decimal type in Hive-hash #17056

[SPARK-17495] [SQL] Support Decimal type in Hive-hash #17056

Uh oh!

Conversation

tejasapatil commented Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

tejasapatil commented Feb 24, 2017

Uh oh!

SparkQA commented Feb 24, 2017

Uh oh!

tejasapatil commented Feb 25, 2017

Uh oh!

SparkQA commented Feb 25, 2017

Uh oh!

tejasapatil commented Feb 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tejasapatil commented Feb 28, 2017

Uh oh!

SparkQA commented Feb 28, 2017

Uh oh!

tejasapatil commented Feb 28, 2017

Uh oh!

SparkQA commented Feb 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tejasapatil Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Mar 1, 2017

Uh oh!

tejasapatil commented Mar 1, 2017

Uh oh!

gatorsmile commented Mar 1, 2017

Uh oh!

gatorsmile commented Mar 1, 2017

Uh oh!

SparkQA commented Mar 1, 2017

Uh oh!

tejasapatil commented Mar 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

tejasapatil commented Feb 24, 2017 •

edited

Loading

tejasapatil Feb 28, 2017 •

edited

Loading