Skip to content

[SPARK-11949][SQL] Set field nullable property for GroupingSets to get correct results for null values #10038

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
viirya wants to merge 2 commits into from
Closed

[SPARK-11949][SQL] Set field nullable property for GroupingSets to get correct results for null values #10038

viirya wants to merge 2 commits into from

Conversation

@viirya
Copy link
Member

@viirya viirya commented Nov 30, 2015

JIRA: https://issues.apache.org/jira/browse/SPARK-11949

The result of cube plan uses incorrect schema. The schema of cube result should set nullable property to true because the grouping expressions will have null values.

@SparkQA
Copy link

SparkQA commented Nov 30, 2015

Test build #46876 has finished for PR 10038 at commit 0f335a6.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Nov 30, 2015

Test build #46879 has finished for PR 10038 at commit c3c37fe.

  • This patch fails from timeout after a configured wait of 250m.
  • This patch merges cleanly.
  • This patch adds no public classes.

@viirya
Copy link
Member Author

viirya commented Nov 30, 2015

retest this please.

@yhuai
Copy link
Contributor

yhuai commented Nov 30, 2015

So, the problem is caused by the nullability setting? Does rollup have the same problem?

@viirya
Copy link
Member Author

viirya commented Nov 30, 2015

@yhuai yeah. Because rollup and cube will be transformed to GroupingSets, this patch should solve both of them.

@SparkQA
Copy link

SparkQA commented Nov 30, 2015

Test build #46888 has finished for PR 10038 at commit c3c37fe.

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

yhuai
yhuai reviewed Nov 30, 2015
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
Copy link
Contributor

@yhuai yhuai Nov 30, 2015

Choose a reason for hiding this comment

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

Maybe we need to check the bitmasks? I mean if we something like GROUPING SETS ( (a,b), a), we do not need to change the nullability of a, right?

Copy link
Contributor

@yhuai yhuai Nov 30, 2015

Choose a reason for hiding this comment

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

(this is a minor issue since setting nullable to true does not cause wrong results)

Copy link
Member Author

@viirya viirya Nov 30, 2015

Choose a reason for hiding this comment

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

Indeed, I think you are right. I will update this later. Thanks.

Copy link
Member Author

@viirya viirya Nov 30, 2015

Choose a reason for hiding this comment

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

Yeah, that is right too.

Copy link
Contributor

@yhuai yhuai Dec 1, 2015

Choose a reason for hiding this comment

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

Do you want to update it in this PR?

Copy link
Member Author

@viirya viirya Dec 1, 2015

Choose a reason for hiding this comment

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

This can be merged first. I will add a following one for the issue later. If it is ok for you.

Copy link
Contributor

@yhuai yhuai Dec 1, 2015

Choose a reason for hiding this comment

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

OK. Sounds good. I will add a TODO at here when I merge it.

Copy link
Contributor

@marmbrus marmbrus Dec 1, 2015

Choose a reason for hiding this comment

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

Friendly ping, if we can fix this last comment we can probably include this in 1.6-RC1.

@yhuai
Copy link
Contributor

yhuai commented Nov 30, 2015

@viirya Thank you for the fix. It overall looks good. Can you update the title to make it provide more information about the issue?

@viirya viirya changed the title [SPARK-11949][SQL] Fix wrong cube results [SPARK-11949][SQL] Set field nullable property for GroupingSets to get correct results Nov 30, 2015
@viirya
Copy link
Member Author

viirya commented Nov 30, 2015

@yhuai It is updated.

@viirya viirya changed the title [SPARK-11949][SQL] Set field nullable property for GroupingSets to get correct results [SPARK-11949][SQL] Set field nullable property for GroupingSets to get correct results for null values Nov 30, 2015
@viirya
Copy link
Member Author

viirya commented Dec 1, 2015

@yhuai Is this title ok for you?

@yhuai
Copy link
Contributor

yhuai commented Dec 1, 2015

Merging to master and branch 1.6.

asfgit pushed a commit that referenced this pull request Dec 1, 2015
@viirya @yhuai
[SPARK-11949][SQL] Set field nullable property for GroupingSets to ge…
add4e63 
…t correct results for null values

JIRA: https://issues.apache.org/jira/browse/SPARK-11949

The result of cube plan uses incorrect schema. The schema of cube result should set nullable property to true because the grouping expressions will have null values.

Author: Liang-Chi Hsieh <viirya@appier.com>

Closes #10038 from viirya/fix-cube.

(cherry picked from commit c87531b)
Signed-off-by: Yin Huai <yhuai@databricks.com>
@asfgit asfgit closed this in c87531b Dec 1, 2015
@viirya viirya mentioned this pull request Dec 1, 2015
Closed
asfgit pushed a commit that referenced this pull request Dec 2, 2015
@viirya @yhuai
[SPARK-11949][SQL] Check bitmasks to set nullable property
3c4938e 
Following up #10038.

We can use bitmasks to determine which grouping expressions need to be set as nullable.

cc yhuai

Author: Liang-Chi Hsieh <viirya@appier.com>

Closes #10067 from viirya/fix-cube-following.

(cherry picked from commit 0f37d1d)
Signed-off-by: Yin Huai <yhuai@databricks.com>
asfgit pushed a commit that referenced this pull request Dec 2, 2015
@viirya @yhuai
[SPARK-11949][SQL] Check bitmasks to set nullable property
0f37d1d 
Following up #10038.

We can use bitmasks to determine which grouping expressions need to be set as nullable.

cc yhuai

Author: Liang-Chi Hsieh <viirya@appier.com>

Closes #10067 from viirya/fix-cube-following.
@viirya viirya deleted the fix-cube branch December 27, 2023 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@viirya @SparkQA @yhuai @marmbrus