Skip to content

[SPARK-22822][TEST] Basic tests for WindowFrameCoercion and DecimalPrecision #20008

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 8 commits into from
Closed

[SPARK-22822][TEST] Basic tests for WindowFrameCoercion and DecimalPrecision #20008

wants to merge 8 commits into from

Conversation

wangyum
Copy link
Member

@wangyum wangyum commented Dec 18, 2017 •
edited
Loading

What changes were proposed in this pull request?

Test Coverage for WindowFrameCoercion and DecimalPrecision, this is a Sub-tasks for SPARK-22722.

How was this patch tested?

N/A

@SparkQA
Copy link

SparkQA commented Dec 18, 2017

Test build #85058 has finished for PR 20008 at commit 05acae3.

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

gatorsmile
gatorsmile reviewed Dec 18, 2017
View reviewed changes
sql/core/src/test/resources/sql-tests/inputs/typeCoercion/native/decimalPrecision.sql Outdated
SELECT cast(1 as tinyint) + cast(1 as decimal(20, 1)) FROM t;
SELECT cast(1 as tinyint) + cast(1 as decimal(21, 1)) FROM t;
SELECT cast(1 as tinyint) + cast(1 as decimal(38, 1)) FROM t;
SELECT cast(1 as tinyint) + cast(1 as decimal(39, 1)) FROM t;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need all these cases? Could you simplify them?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about only these 4 decimals: DECIMAL(3, 0), DECIMAL(5, 0), DECIMAL(10, 0) and DECIMAL(20, 0).

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/DecimalPrecision.scala

Lines 54 to 57 in 00d176d

* - BYTE gets turned into DECIMAL(3, 0)
* - SHORT gets turned into DECIMAL(5, 0)
* - INT gets turned into DECIMAL(10, 0)
* - LONG gets turned into DECIMAL(20, 0)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

@gatorsmile
Copy link
Member

gatorsmile commented Dec 19, 2017 •
edited
Loading

functionArgumentConversion.sql has too many combinations. Let us get rid of this one.

@SparkQA
Copy link

SparkQA commented Dec 19, 2017

Test build #85111 has finished for PR 20008 at commit 3ff5c9b.

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

wangyum
wangyum commented Dec 20, 2017
View reviewed changes
sql/core/src/test/resources/sql-tests/inputs/typeCoercion/native/functionArgumentConversion.sql Outdated
@@ -25,7 +25,7 @@ SELECT array(cast(1 as tinyint), cast(1 as float)) FROM t;
SELECT array(cast(1 as tinyint), cast(1 as double)) FROM t;
SELECT array(cast(1 as tinyint), cast(1 as decimal(10, 0))) FROM t;
SELECT array(cast(1 as tinyint), cast(1 as string)) FROM t;
SELECT array(cast(1 as tinyint), cast('1' as binary)) FROM t;
SELECT size(array(cast(1 as tinyint), cast('1' as binary))) FROM t;
Copy link
Member Author

@wangyum wangyum Dec 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace array(cast(1 as tinyint), cast('1' as binary)) with size(array(cast(1 as tinyint), cast('1' as binary))) to avoid binary type with collection error.

@SparkQA
Copy link

SparkQA commented Dec 20, 2017

Test build #85143 has finished for PR 20008 at commit 16f0a99.

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

@gatorsmile
Copy link
Member

functionArgumentConversion.sql has too many combinations. Let us get rid of this one?

@wangyum wangyum changed the title [SPARK-22822][TEST] Basic tests for FunctionArgumentConversion and DecimalPrecision [SPARK-22822][TEST] Basic tests for WindowFrameCoercion and DecimalPrecision Dec 20, 2017
@SparkQA
Copy link

SparkQA commented Dec 20, 2017

Test build #85156 has finished for PR 20008 at commit b4c5339.

  • 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 Dec 20, 2017

retest this, please.

@SparkQA
Copy link

SparkQA commented Dec 20, 2017

Test build #85162 has finished for PR 20008 at commit b4c5339.

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

wangyum
wangyum commented Dec 20, 2017
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/windowExpressions.scala Outdated
@@ -252,7 +252,7 @@ case class SpecifiedWindowFrame(
case e: Expression if !frameType.inputType.acceptsType(e.dataType) =>
TypeCheckFailure(
s"The data type of the $location bound '${e.dataType} does not match " +
s"the expected data type '${frameType.inputType}'.")
s"the expected data type '${frameType.inputType.simpleString}'.")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise the result is:

cannot resolve 'RANGE BETWEEN CURRENT ROW AND CAST(1 AS STRING) FOLLOWING' due to data type mismatch: The data type of the upper bound 'StringType does not match the expected data type 'org.apache.spark.sql.types.TypeCollection@7ff36201'.; line 1 pos 21

@SparkQA
Copy link

SparkQA commented Dec 20, 2017

Test build #85186 has finished for PR 20008 at commit 19bcca1.

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

@SparkQA
Copy link

SparkQA commented Dec 20, 2017

Test build #85187 has finished for PR 20008 at commit 3291339.

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

@SparkQA
Copy link

SparkQA commented Dec 21, 2017

Test build #85215 has finished for PR 20008 at commit a49d1cc.

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

@SparkQA
Copy link

SparkQA commented Dec 21, 2017

Test build #85217 has finished for PR 20008 at commit ec07bc2.

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

@wangyum
Copy link
Member Author

wangyum commented Dec 21, 2017

retest this please

@SparkQA
Copy link

SparkQA commented Dec 21, 2017

Test build #85233 has finished for PR 20008 at commit ec07bc2.

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

@gatorsmile
Copy link
Member

The test failure is not related to this PR.

LGTM

Thanks! Merged to master.

@asfgit asfgit closed this in 4e107fd Dec 21, 2017
@gatorsmile gatorsmile mentioned this pull request Dec 24, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gatorsmile gatorsmile gatorsmile left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wangyum @SparkQA @gatorsmile