Skip to content

[SPARK-25457][SQL] IntegralDivide returns data type of the operands#22465

Closed
mgaido91 wants to merge 2 commits intoapache:masterfrom
mgaido91:SPARK-25457
Closed

[SPARK-25457][SQL] IntegralDivide returns data type of the operands#22465
mgaido91 wants to merge 2 commits intoapache:masterfrom
mgaido91:SPARK-25457

Conversation

@mgaido91
Copy link
Contributor

What changes were proposed in this pull request?

The PR proposes to return the data type of the operands as a result for the div operator. Before the PR, bigint is always returned. It introduces also a spark.sql.legacy.integralDivide.returnBigint config in order to let the users restore the legacy behavior.

How was this patch tested?

added UTs

select null div 5;

set spark.sql.legacy.integralDivide.returnBigint=false;

Copy link
Member

Choose a reason for hiding this comment

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

Can we move these tests into the end of this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to keep them close to the other div tests, but I am fine moving them. Let me know.

Copy link
Member

Choose a reason for hiding this comment

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

Too many unrelated changes in the output file, so IMO it'd be better to do so. But, its ok to wait for other reviews.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok thanks, I see your point. cc @cloud-fan @dongjoon-hyun @viirya for their opinion on this. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems it's getting more complicated, can we create a new file for testing div?

@SparkQA
Copy link

SparkQA commented Sep 19, 2018

Test build #96243 has finished for PR 22465 at commit 19ff368.

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

@SparkQA
Copy link

SparkQA commented Sep 19, 2018

Test build #96263 has finished for PR 22465 at commit 3f045c0.

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

@mgaido91
Copy link
Contributor Author

retest this please

.booleanConf
.createWithDefault(false)

val LEGACY_INTEGRALDIVIDE_RETURN_LONG = buildConf("spark.sql.legacy.integralDivide.returnBigint")
Copy link
Member

Choose a reason for hiding this comment

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

returnBigint or returnLong? I think we use "long" instead of "big int" as type name?

Copy link
Contributor

Choose a reason for hiding this comment

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

big int is more commonly used in the SQL world

@SparkQA
Copy link

SparkQA commented Sep 20, 2018

Test build #96287 has finished for PR 22465 at commit 3f045c0.

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

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@asfgit asfgit closed this in 47d6e80 Sep 20, 2018
@mgaido91
Copy link
Contributor Author

thank you

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.

5 participants