Skip to content

[SPARK-20690][SQL] Subqueries in FROM should have alias names #17935

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

[SPARK-20690][SQL] Subqueries in FROM should have alias names #17935

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c079946
Analyzer shouldn't add missing attributes through subqueries.
viirya May 10, 2017
51aafde
Merge remote-tracking branch 'upstream/master' into SPARK-20690
viirya May 11, 2017
c19cbbb
Fix merging.
viirya May 11, 2017
a904214
Fix test.
viirya May 11, 2017
1e25926
Require alias name for aliasedQuery.
viirya May 16, 2017
307ca1d
Merge remote-tracking branch 'upstream/master' into SPARK-20690
viirya May 16, 2017
1e595b2
Address comments.
viirya May 16, 2017
9ec0414
Fix tests.
viirya May 17, 2017
9b117f4
Address comment.
viirya May 17, 2017
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
2 changes: 1 addition & 1 deletion sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,7 @@ identifierComment

relationPrimary
: tableIdentifier sample? (AS? strictIdentifier)? #tableName
| '(' queryNoWith ')' sample? (AS? strictIdentifier)? #aliasedQuery
| '(' queryNoWith ')' sample? (AS? strictIdentifier) #aliasedQuery

@cloud-fan cloud-fan May 16, 2017

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.

shall we also update AstBuilder for this?

@viirya viirya May 16, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes.

@hvanhovell hvanhovell May 16, 2017

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.

Should we also force an alias for the next line? '(' relation ')' sample? (AS? strictIdentifier)?

@viirya viirya May 16, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I also have this question when I change this.

MySQL supports select 1 from (test); but doesn't support select 1 from (test) as a ;

Postgres doesn't support both syntax.

Hive supports select * from (test); but doesn't support select * from (test) as a;.

Seems aliased relation is not commonly supported. So I leave it untouched.

@hvanhovell hvanhovell May 16, 2017

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.

Yay, unclear semantics. Ok, that is fine for now.

| '(' relation ')' sample? (AS? strictIdentifier)? #aliasedRelation
| inlineTable #inlineTableDefault2
| functionTable #tableValuedFunction
Expand Down
4 changes: 2 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 @@ -1045,7 +1045,7 @@ class Analyzer(
// Skip sort with aggregate. This will be handled in ResolveAggregateFunctions
case sa @ Sort(_, _, child: Aggregate) => sa

case s @ Sort(order, _, child) if child.resolved =>
case s @ Sort(order, _, child) if !s.resolved && child.resolved =>
try {
val newOrder = order.map(resolveExpressionRecursively(_, child).asInstanceOf[SortOrder])
val requiredAttrs = AttributeSet(newOrder).filter(_.resolved)
Expand All @@ -1066,7 +1066,7 @@ class Analyzer(
case ae: AnalysisException => s
}

case f @ Filter(cond, child) if child.resolved =>
case f @ Filter(cond, child) if !f.resolved && child.resolved =>

@gatorsmile gatorsmile May 16, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Only added for Filter? How about Sort in the same rule?

@viirya viirya May 16, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sure.

try {
val newCond = resolveExpressionRecursively(cond, child)
val requiredAttrs = newCond.references.filter(_.resolved)
Expand Down
5 changes: 2 additions & 3 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -745,9 +745,8 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging
* hooks.
*/
override def visitAliasedQuery(ctx: AliasedQueryContext): LogicalPlan = withOrigin(ctx) {
plan(ctx.queryNoWith)
.optionalMap(ctx.sample)(withSample)
.optionalMap(ctx.strictIdentifier)(aliasPlan)
aliasPlan(ctx.strictIdentifier,
plan(ctx.queryNoWith).optionalMap(ctx.sample)(withSample))
}

/**
Expand Down
6 changes: 6 additions & 0 deletions ...st/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -798,6 +798,12 @@ case class LocalLimit(limitExpr: Expression, child: LogicalPlan) extends UnaryNo
}
}

/**
* Aliased subquery.
*
* @param alias the alias name for this subquery.
* @param child the logical plan of this subquery.
*/
case class SubqueryAlias(
alias: String,
child: LogicalPlan)
Expand Down
11 changes: 11 additions & 0 deletions sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,17 @@ class PlanParserSuite extends PlanTest {
| (select id from t0)) as u_1
""".stripMargin,
plan.union(plan).union(plan).as("u_1").select('id))

}

test("aliased subquery") {
assertEqual("select a from (select id as a from t0) tt",
table("t0").select('id.as("a")).as("tt").select('a))
intercept("select a from (select id as a from t0)", "mismatched input")

assertEqual("from (select id as a from t0) tt select a",
table("t0").select('id.as("a")).as("tt").select('a))
intercept("from (select id as a from t0) select a", "extraneous input 'a'")
}

test("scalar sub-query") {
Expand Down
18 changes: 9 additions & 9 deletions sql/core/src/test/scala/org/apache/spark/sql/CachedTableSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -631,13 +631,13 @@ class CachedTableSuite extends QueryTest with SQLTestUtils with SharedSQLContext
val ds2 =
sql(
"""
|SELECT * FROM (SELECT max(c1) FROM t1 GROUP BY c1)
|SELECT * FROM (SELECT max(c1) as c1 FROM t1 GROUP BY c1) t1
|WHERE
|c1 = (SELECT max(c1) FROM t2 GROUP BY c1)
|t1.c1 = (SELECT max(c1) FROM t2 GROUP BY c1)
|OR
|EXISTS (SELECT c1 FROM t3)
|OR
|c1 IN (SELECT c1 FROM t4)
|t1.c1 IN (SELECT c1 FROM t4)
""".stripMargin)
assert(getNumInMemoryRelations(ds2) == 4)
}
Expand Down Expand Up @@ -788,25 +788,25 @@ class CachedTableSuite extends QueryTest with SQLTestUtils with SharedSQLContext
// Scalar subquery and predicate subquery
sql(
"""
|SELECT * FROM (SELECT max(c1) FROM t1 GROUP BY c1)
|SELECT * FROM (SELECT max(c1) as c1 FROM t1 GROUP BY c1) t1
|WHERE
|c1 = (SELECT max(c1) FROM t2 GROUP BY c1)
|t1.c1 = (SELECT max(c1) FROM t2 GROUP BY c1)
|OR
|EXISTS (SELECT c1 FROM t3)
|OR
|c1 IN (SELECT c1 FROM t4)
|t1.c1 IN (SELECT c1 FROM t4)
""".stripMargin).cache()

val cachedDs2 =
sql(
"""
|SELECT * FROM (SELECT max(c1) FROM t1 GROUP BY c1)
|SELECT * FROM (SELECT max(c1) as c1 FROM t1 GROUP BY c1) t1
|WHERE
|c1 = (SELECT max(c1) FROM t2 GROUP BY c1)
|t1.c1 = (SELECT max(c1) FROM t2 GROUP BY c1)
|OR
|EXISTS (SELECT c1 FROM t3)
|OR
|c1 IN (SELECT c1 FROM t4)
|t1.c1 IN (SELECT c1 FROM t4)
""".stripMargin)
assert(getNumInMemoryRelations(cachedDs2) == 1)
}
Expand Down
8 changes: 4 additions & 4 deletions sql/core/src/test/scala/org/apache/spark/sql/SubquerySuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ class SubquerySuite extends QueryTest with SharedSQLContext {
| with t4 as (select 1 as d, 3 as e)
| select * from t4 cross join t2 where t2.b = t4.d
| )
| select a from (select 1 as a union all select 2 as a)
| select a from (select 1 as a union all select 2 as a) t
| where a = (select max(d) from t3)
""".stripMargin),
Array(Row(1))
Expand Down Expand Up @@ -606,8 +606,8 @@ class SubquerySuite extends QueryTest with SharedSQLContext {
| select cntPlusOne + 1 as cntPlusTwo from (
| select cnt + 1 as cntPlusOne from (
| select sum(r.c) s, count(*) cnt from r where l.a = r.c having cnt = 0
| )
| )
| ) t1
| ) t2
|) = 2""".stripMargin),
Row(1) :: Row(1) :: Row(null) :: Row(null) :: Nil)
}
Expand Down Expand Up @@ -655,7 +655,7 @@ class SubquerySuite extends QueryTest with SharedSQLContext {
"""
| select c1 from onerow t1
| where exists (select 1
| from (select 1 from onerow t2 LIMIT 1)
| from (select 1 as c1 from onerow t2 LIMIT 1) t2
| where t1.c1=t2.c1)""".stripMargin),
Row(1) :: Nil)
}
Expand Down