Skip to content

[SPARK-24536] Validate that an evaluated limit clause cannot be null #21807

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

[SPARK-24536] Validate that an evaluated limit clause cannot be null #21807

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
91a5fda
Validate that limit clause cannot have a nullable expression
mauropalsgraaf Jul 18, 2018
f84ff23
Make sure that evaluated limit expressions cannot be null
mauropalsgraaf Jul 28, 2018
5ce97b4
Changed evalledExpression to alias e
mauropalsgraaf Jul 28, 2018
7253c55
Merge branch 'master' of github.com:apache/spark into SPARK-24536
mauropalsgraaf Jul 28, 2018
5235f32
Remove merge conflict missing bracket
mauropalsgraaf Jul 28, 2018
c0eb690
Remove double white line
mauropalsgraaf Jul 28, 2018
60b9af3
Include match statement inline of checking the limit
mauropalsgraaf Jul 30, 2018
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
12 changes: 8 additions & 4 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,14 @@ trait CheckAnalysis extends PredicateHelper {
case e if e.dataType != IntegerType => failAnalysis(
s"The limit expression must be integer type, but got " +
e.dataType.catalogString)
case e if e.eval().asInstanceOf[Int] < 0 => failAnalysis(
Copy link
Member

@HyukjinKwon HyukjinKwon Jul 30, 2018

Choose a reason for hiding this comment

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

How about this:

case e => e.eval() match {
  case null => failAnalysis(
    s"The evaluated limit expression must not be null, but got ${limitExpr.sql}")
  case v: Int if v < 0 => failAnalysis(
    s"The limit expression must be equal to or greater than 0, but got $v")
  case _ => // OK
}

Copy link
Contributor Author

@mauropalsgraaf mauropalsgraaf Jul 30, 2018

Choose a reason for hiding this comment

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

I'm personally not a fan of this because of the nesting, but I guess it's a matter of taste. Is there any guidelines for this regarding to styling? I don't have a strong opinion about this

Copy link
Member

@HyukjinKwon HyukjinKwon Jul 30, 2018

Choose a reason for hiding this comment

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

nope but it's shorter and I don't think this code makes less readable.

Copy link
Contributor Author

@mauropalsgraaf mauropalsgraaf Jul 30, 2018

Choose a reason for hiding this comment

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

fixed

"The limit expression must be equal to or greater than 0, but got " +
e.eval().asInstanceOf[Int])
case e => // OK
case e =>
e.eval() match {
case null => failAnalysis(
s"The evaluated limit expression must not be null, but got ${limitExpr.sql}")
case v: Int if v < 0 => failAnalysis(
s"The limit expression must be equal to or greater than 0, but got $v")
case _ => // OK
}
}
}

Expand Down
6 changes: 6 additions & 0 deletions sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisErrorSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,12 @@ class AnalysisErrorSuite extends AnalysisTest {
"Generators are not supported outside the SELECT clause, but got: Sort" :: Nil
)

errorTest(
"an evaluated limit class must not be null",
testRelation.limit(Literal(null, IntegerType)),
"The evaluated limit expression must not be null, but got " :: Nil
)

errorTest(
"num_rows in limit clause must be equal to or greater than 0",
listRelation.limit(-1),
Expand Down
5 changes: 5 additions & 0 deletions sql/core/src/test/resources/sql-tests/inputs/limit.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ SELECT * FROM testdata LIMIT CAST(1 AS int);
SELECT * FROM testdata LIMIT -1;
SELECT * FROM testData TABLESAMPLE (-1 ROWS);


SELECT * FROM testdata LIMIT CAST(1 AS INT);
-- evaluated limit must not be null
SELECT * FROM testdata LIMIT CAST(NULL AS INT);

-- limit must be foldable
SELECT * FROM testdata LIMIT key > 3;

Expand Down
45 changes: 31 additions & 14 deletions sql/core/src/test/resources/sql-tests/results/limit.sql.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
-- Automatically generated by SQLQueryTestSuite
-- Number of queries: 12
-- Number of queries: 14


-- !query 0
Expand Down Expand Up @@ -66,44 +66,61 @@ The limit expression must be equal to or greater than 0, but got -1;


-- !query 7
SELECT * FROM testdata LIMIT key > 3
SELECT * FROM testdata LIMIT CAST(1 AS INT)
-- !query 7 schema
struct<>
struct<key:int,value:string>
-- !query 7 output
org.apache.spark.sql.AnalysisException
The limit expression must evaluate to a constant value, but got (testdata.`key` > 3);
1 1


-- !query 8
SELECT * FROM testdata LIMIT true
SELECT * FROM testdata LIMIT CAST(NULL AS INT)
-- !query 8 schema
struct<>
-- !query 8 output
org.apache.spark.sql.AnalysisException
The limit expression must be integer type, but got boolean;
The evaluated limit expression must not be null, but got CAST(NULL AS INT);


-- !query 9
SELECT * FROM testdata LIMIT 'a'
SELECT * FROM testdata LIMIT key > 3
-- !query 9 schema
struct<>
-- !query 9 output
org.apache.spark.sql.AnalysisException
The limit expression must be integer type, but got string;
The limit expression must evaluate to a constant value, but got (testdata.`key` > 3);


-- !query 10
SELECT * FROM (SELECT * FROM range(10) LIMIT 5) WHERE id > 3
SELECT * FROM testdata LIMIT true
-- !query 10 schema
struct<id:bigint>
struct<>
-- !query 10 output
4
org.apache.spark.sql.AnalysisException
The limit expression must be integer type, but got boolean;


-- !query 11
SELECT * FROM testdata WHERE key < 3 LIMIT ALL
SELECT * FROM testdata LIMIT 'a'
-- !query 11 schema
struct<key:int,value:string>
struct<>
-- !query 11 output
org.apache.spark.sql.AnalysisException
The limit expression must be integer type, but got string;


-- !query 12
SELECT * FROM (SELECT * FROM range(10) LIMIT 5) WHERE id > 3
-- !query 12 schema
struct<id:bigint>
-- !query 12 output
4


-- !query 13
SELECT * FROM testdata WHERE key < 3 LIMIT ALL
-- !query 13 schema
struct<key:int,value:string>
-- !query 13 output
1 1
2 2