Skip to content

[SPARK-16355] [SPARK-16354] [SQL] Fix Bugs When LIMIT/TABLESAMPLE is Non-foldable, Zero or Negative #14034

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

[SPARK-16355] [SPARK-16354] [SQL] Fix Bugs When LIMIT/TABLESAMPLE is Non-foldable, Zero or Negative #14034

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
1255968
fix
gatorsmile Jul 3, 2016
bdf4e56
reverted
gatorsmile Jul 3, 2016
3c402d3
address comments
gatorsmile Jul 4, 2016
5b36fbc
Detected foldable expression in limit
gatorsmile Jul 5, 2016
a2a828f
address comments.
gatorsmile Jul 5, 2016
f600ba4
address comments.
gatorsmile Jul 8, 2016
8fd72f6
address comments.
gatorsmile Jul 8, 2016
1abdbb9
address comments.
gatorsmile Jul 8, 2016
3036847
address comments.
gatorsmile Jul 8, 2016
d135b77
change another way
gatorsmile Jul 9, 2016
028aa79
revert it back
gatorsmile Jul 9, 2016
01137dc
address comments
gatorsmile Jul 10, 2016
0ebbdfe
Merge remote-tracking branch 'upstream/master' into limit
gatorsmile Jul 11, 2016
dec5ad9
address comments
gatorsmile Jul 11, 2016
2e6f8d8
address comments
gatorsmile Jul 11, 2016
d66870b
fix test case
gatorsmile Jul 11, 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
Prev Previous commit
Next Next commit
change another way
  • Loading branch information
@gatorsmile
gatorsmile committed Jul 9, 2016
commit d135b77b2d3dd2f9f6ae160bfb24b73df34d3995
6 changes: 3 additions & 3 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 @@ -2047,14 +2047,14 @@ object EliminateUnions extends Rule[LogicalPlan] {
}

/**
* Converts foldable numeric expressions to integers of [[GlobalLimit]] and [[LocalLimit]] operators
* Converts foldable numeric expressions to integers in [[GlobalLimit]] and [[LocalLimit]] operators
*/
object ResolveLimits extends Rule[LogicalPlan] {
Copy link
Contributor

@cloud-fan cloud-fan Jul 9, 2016

Choose a reason for hiding this comment

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

Thanks for trying this out! Yea looks like it's doable. Let's remove it first and do it in follow-ups, to make this PR surgical.

Copy link
Contributor

@cloud-fan cloud-fan Jul 9, 2016

Choose a reason for hiding this comment

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

We can create a JIRA first and discuss with others to decide if it's useful to support all integral types in limit.

Copy link
Member Author

@gatorsmile gatorsmile Jul 9, 2016

Choose a reason for hiding this comment

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

Sure, let me revert it back. Thanks!

def apply(plan: LogicalPlan): LogicalPlan = plan transform {
case g @ GlobalLimit(limitExpr, _) if limitExpr.foldable && isNumeric(limitExpr.eval()) =>
g.copy(limitExpr = Literal(limitExpr.eval().asInstanceOf[Number].intValue(), IntegerType))
g.copy(limitExpr = Literal(Cast(limitExpr, IntegerType).eval(), IntegerType))
case l @ LocalLimit(limitExpr, _) if limitExpr.foldable && isNumeric(limitExpr.eval()) =>
l.copy(limitExpr = Literal(limitExpr.eval().asInstanceOf[Number].intValue(), IntegerType))
l.copy(limitExpr = Literal(Cast(limitExpr, IntegerType).eval(), IntegerType))
}

private def isNumeric(value: Any): Boolean =
Expand Down
1 change: 1 addition & 0 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 @@ -52,6 +52,7 @@ trait CheckAnalysis extends PredicateHelper {
"The argument to the LIMIT clause must evaluate to a constant value. " +
s"Limit:${limitExpr.sql}")
}
// Analyzer rule ResolveLimits already converts limitExpr to integers.
limitExpr match {
case IntegerLiteral(limit) if limit >= 0 => // OK
case IntegerLiteral(limit) => failAnalysis(
Expand Down