Skip to content

[SPARK-17849] [SQL] Fix NPE problem when using grouping sets #15416

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

Closed
yangw1234 wants to merge 8 commits into from
Closed

[SPARK-17849] [SQL] Fix NPE problem when using grouping sets #15416

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a6d4651
fix grouping id bug
Oct 10, 2016
ab82b21
add test
Oct 10, 2016
c849b6a
add test
Oct 10, 2016
42f7a63
mv test to SQLQueryTestSuit
Oct 10, 2016
9d7f33a
add comment
Oct 11, 2016
9893c03
address davies' comments
Oct 11, 2016
69f6e4f
refine name
Oct 11, 2016
0ad7aba
fix scala style
Oct 14, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,10 +298,11 @@ class Analyzer(
case other => Alias(other, other.toString)()
}

val nonNullBitmask = x.bitmasks.reduce(_ & _)
val nonNullBitmask = ~ x.bitmasks.reduce(_ | _)

@hvanhovell hvanhovell Oct 10, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bit manipulation magic is hard to follow. This is should be documented better. Could you add a line or two to explain how the bitmasks are structured?

@rxin rxin Oct 11, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1

@yangw1234 yangw1234 Oct 11, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ok, I'll do it.

@yangw1234 yangw1234 Oct 11, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@hvanhovell @rxin comments are added

@davies davies Oct 11, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you remove the '~' here, and use (nonNullBitmask & (1 << (attrLength - idx - 1))) == 1?

@yangw1234 yangw1234 Oct 11, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Do you mean ((nonNullBitmask >> (attrLength - idx - 1)) & 1) == 1? We can only test on 0 if we left shift 1, right? @davies


val attrLength = groupByAliases.length
val expandedAttributes = groupByAliases.zipWithIndex.map { case (a, idx) =>
a.toAttribute.withNullability((nonNullBitmask & 1 << idx) == 0)
a.toAttribute.withNullability((nonNullBitmask & 1 << (attrLength - idx - 1)) == 0)
}

val expand = Expand(x.bitmasks, groupByAliases, expandedAttributes, gid, x.child)
Expand Down
18 changes: 18 additions & 0 deletions sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2189,6 +2189,24 @@ class SQLQuerySuite extends QueryTest with SharedSQLContext {
}
}

test("SPARK-17849: grouping set throws NPE") {

@rxin rxin Oct 10, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe we can move this into SQLQueryTestSuite, by creating a new grouping_set.q file??

@yangw1234 yangw1234 Oct 10, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@rxin done

val data = Seq(
("1", "2", "3", 1),
("4", "5", "6", 1),
("7", "8", "9", 1)
)
data.toDF("a", "b", "c", "d").createOrReplaceTempView("point_table")
checkAnswer(
sql("select a, b, c, count(d) from point_table group by a, b, c GROUPING SETS (())"),
Row(null, null, null, 3) :: Nil)
checkAnswer(
sql("select a, b, c, count(d) from point_table group by a, b, c GROUPING SETS ((a))"),
Row("1", null, null, 1) ::
Row("4", null, null, 1) ::
Row("7", null, null, 1) :: Nil)
}


test("hash function") {
val df = Seq(1 -> "a", 2 -> "b").toDF("i", "j")
withTempView("tbl") {
Expand Down