-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-28227][SQL] Support projection, aggregate/window functions, and lateral view in the TRANSFORM clause #29087
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.
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
Changes from 1 commit
d51c0dc
5d85160
dbb4d04
2b8912e
d89afa9
b1cc739
f2a640b
b04909c
671711b
1a4262b
8df104b
1b4e0c1
a85753f
3eb8d11
2e146c3
ca5a032
614f8f9
ee16a2f
9ef73b6
707f1e6
327566f
fa293cd
a8233d4
c3d423a
cf4085a
1278705
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -155,6 +155,8 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging | |
| ctx, | ||
| ctx.transformClause, | ||
| ctx.whereClause, | ||
| ctx.aggregationClause, | ||
| ctx.havingClause, | ||
| plan | ||
| ) | ||
| } else { | ||
|
|
@@ -583,7 +585,13 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging | |
| val from = OneRowRelation().optional(ctx.fromClause) { | ||
| visitFromClause(ctx.fromClause) | ||
| } | ||
| withTransformQuerySpecification(ctx, ctx.transformClause, ctx.whereClause, from) | ||
| withTransformQuerySpecification( | ||
| ctx, | ||
| ctx.transformClause, | ||
| ctx.whereClause, | ||
| ctx.aggregationClause, | ||
| ctx.havingClause, from | ||
|
AngersZhuuuu marked this conversation as resolved.
Outdated
|
||
| ) | ||
| } | ||
|
|
||
| override def visitRegularQuerySpecification( | ||
|
|
@@ -638,7 +646,9 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging | |
| ctx: ParserRuleContext, | ||
| transformClause: TransformClauseContext, | ||
| whereClause: WhereClauseContext, | ||
| relation: LogicalPlan): LogicalPlan = withOrigin(ctx) { | ||
| aggregationClause: AggregationClauseContext, | ||
| havingClause: HavingClauseContext, | ||
| relation: LogicalPlan): LogicalPlan = withOrigin(ctx) { | ||
| // Add where. | ||
| val withFilter = relation.optionalMap(whereClause)(withWhereClause) | ||
|
|
||
|
|
@@ -660,12 +670,39 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging | |
| AttributeReference("value", StringType)()), true) | ||
| } | ||
|
|
||
| val namedExpressions = expressions.map { | ||
| case e: NamedExpression => e | ||
| case e: Expression => UnresolvedAlias(e) | ||
| } | ||
|
|
||
| def createProject() = if (namedExpressions.nonEmpty) { | ||
| Project(namedExpressions, withFilter) | ||
| } else { | ||
| withFilter | ||
| } | ||
|
|
||
| val withProject = if (aggregationClause == null && havingClause != null) { | ||
| if (conf.getConf(SQLConf.LEGACY_HAVING_WITHOUT_GROUP_BY_AS_WHERE)) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this is a new feature, we don't need to follow this config here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We can follow this, why not, but I need to add test case about this. |
||
| // If the legacy conf is set, treat HAVING without GROUP BY as WHERE. | ||
| withHavingClause(havingClause, createProject()) | ||
| } else { | ||
| // According to SQL standard, HAVING without GROUP BY means global aggregate. | ||
| withHavingClause(havingClause, Aggregate(Nil, namedExpressions, withFilter)) | ||
| } | ||
| } else if (aggregationClause != null) { | ||
| val aggregate = withAggregationClause(aggregationClause, namedExpressions, withFilter) | ||
| aggregate.optionalMap(havingClause)(withHavingClause) | ||
| } else { | ||
| // When hitting this branch, `having` must be null. | ||
| createProject() | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this part just copied from
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Done |
||
|
|
||
| // Create the transform. | ||
| ScriptTransformation( | ||
| expressions, | ||
| Seq(UnresolvedStar(None)), | ||
|
maropu marked this conversation as resolved.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hm, on second thought, we cannot remove this param
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It looks like this. We can replace
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice. Just removing the param causes a big diff? I'd like to remove the current the weird Analyzer code to handle the unresolved star in this PR though.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yea, will change a lot of file. Compared to the origin code here(the current the weird Analyzer code) ,current change seem s not so weird, but for the whole process, really weird. Create a ticket for this https://issues.apache.org/jira/browse/SPARK-34035
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
How many changed lines of codes there?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Nearly 100 lines, most of changes are about UT
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. okay, I don't have a strong opinion on it, so please follow other reviewer's comment.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yea |
||
| string(transformClause.script), | ||
| attributes, | ||
| withFilter, | ||
| withProject, | ||
| withScriptIOSchema( | ||
| ctx, | ||
| transformClause.inRowFormat, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2558,6 +2558,131 @@ abstract class SQLQuerySuiteBase extends QueryTest with SQLTestUtils with TestHi | |
| } | ||
| } | ||
| } | ||
|
|
||
| test("SPARK-28227: test script transform with aggregation") { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you move the tests into
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This should wait for #29085, since currently we can't use script transform in sql/core
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I see.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @maropu |
||
| assume(TestUtils.testCommandAvailable("/bin/bash")) | ||
| val data1 = (1 to 1000).map { i => (i, i, i) } | ||
| data1.toDF("d1", "d2", "d3").createOrReplaceTempView("script_trans") | ||
|
|
||
| // without aggregation | ||
| assert(0 === sql( | ||
| s"""SELECT TRANSFORM ( d2, d1, CAST(d3 AS STRING) ) | ||
| |USING 'cat 1>&2' AS (a,b,c) | ||
| |FROM script_trans | ||
| |WHERE d1 <= 100""".stripMargin).count()) | ||
|
|
||
| // number as column | ||
| assert(0 === sql( | ||
| s"""SELECT TRANSFORM ( 1,2,3 ) | ||
| |USING 'cat 1>&2' AS (a,b,c) | ||
| |FROM script_trans | ||
| |WHERE d1 <= 100""".stripMargin).count()) | ||
|
|
||
| // number as column output | ||
| checkAnswer(sql( | ||
| """ | ||
| |SELECT TRANSFORM(1,2) | ||
| |USING 'cat' AS (a INT,b INT) | ||
| |FROM script_trans | ||
| |LIMIT 1 | ||
| """.stripMargin), Row(1, 2)) | ||
|
|
||
| // without aggregation with complex function | ||
| assert(0 === sql( | ||
| s"""SELECT TRANSFORM ( | ||
| |d2 AS d5, | ||
| |d1, | ||
| |CASE | ||
| | WHEN d3 > 100 THEN 1 | ||
| | WHEN d3 < 100 THEN 2 | ||
| |ELSE 3 END ) | ||
| |USING 'cat 1>&2' AS (a,b,c) | ||
| |FROM script_trans | ||
| |WHERE d1 <= 100""".stripMargin).count()) | ||
|
|
||
| // binary operator function | ||
| assert(0 === sql( | ||
| s"""SELECT TRANSFORM ( d2, d1, d3 + 1 ) | ||
| |USING 'cat 1>&2' AS (a,b,c) | ||
| |FROM script_trans | ||
| |WHERE d1 <= 100""".stripMargin).count()) | ||
|
|
||
| // test all start | ||
| assert(100 === sql( | ||
| s"""SELECT TRANSFORM (*) | ||
| |USING 'cat' AS (a,b,c) | ||
| |FROM script_trans | ||
| |WHERE d1 <= 100""".stripMargin).count()) | ||
|
|
||
| // with aggregation and complex function | ||
| assert(100 === sql( | ||
| s"""SELECT TRANSFORM ( d2 AS d4, MAX(d1) as maxd1, CAST(SUM(d3) AS STRING)) | ||
| |USING 'cat' AS (a,b,c) | ||
| |FROM script_trans | ||
| |WHERE d1 <= 100 | ||
| |GROUP BY d2""".stripMargin).count()) | ||
|
|
||
|
|
||
| assert(100 === sql( | ||
| s"""SELECT TRANSFORM (d2, MAX(d1) as maxd1, CAST(sum(d3) AS STRING)) | ||
| |USING 'cat' AS (a,b,c) | ||
| |FROM script_trans | ||
| |WHERE d1 <= 100 | ||
| |GROUP BY d2""".stripMargin).count()) | ||
|
|
||
| assert(0 === sql( | ||
| s"""SELECT TRANSFORM (d2, MAX(d1) as maxd1, CAST(SUM(d3) AS STRING)) | ||
| |USING 'cat 1>&2' AS (a,b,c) | ||
| |FROM script_trans | ||
| |WHERE d1 <= 100 | ||
| |GROUP BY d2 | ||
| |HAVING maxd1 > 0""".stripMargin).count()) | ||
|
|
||
| assert(90 === sql( | ||
| s"""SELECT TRANSFORM (d2, MAX(d1) as maxd1, CAST(SUM(d3) AS STRING)) | ||
| |USING 'cat' AS (a,b,c) | ||
| |FROM script_trans | ||
| |WHERE d1 <= 100 | ||
| |GROUP BY d2 | ||
| |HAVING max(d1) > 10""".stripMargin).count()) | ||
|
|
||
| val data2 = (1 to 5).map { i => (i, i) } | ||
| data2.toDF("key", "value").createOrReplaceTempView("test") | ||
| checkAnswer( | ||
| sql( | ||
| """FROM | ||
| |(FROM test SELECT TRANSFORM(key, value) USING 'cat' AS (`thing1` INT, thing2 STRING)) t | ||
| |SELECT thing1 + 1 | ||
| """.stripMargin), (2 to 6).map(i => Row(i))) | ||
|
|
||
| val data3 = (1 to 5).map { i => (i % 2, i) } | ||
| data3.toDF("key", "value").createOrReplaceTempView("test") | ||
| checkAnswer( | ||
| sql( | ||
| """FROM | ||
| |(SELECT TRANSFORM(key, SUM(value) value) | ||
| |USING 'cat' AS (`thing1` INT, thing2 STRING) | ||
| |FROM test | ||
| |GROUP BY key | ||
| |) t | ||
| |SELECT (thing2 + 1) AS result | ||
| """.stripMargin).sort("result"), Array(7, 10).map(i => Row(i))) | ||
|
|
||
| checkAnswer( | ||
| sql( | ||
| """ | ||
| |MAP k / 10 USING 'cat' AS (one) FROM (SELECT 10 AS k) | ||
| """.stripMargin | ||
| ), Row("1.0")) | ||
|
|
||
| checkAnswer( | ||
| sql( | ||
| """ | ||
| |FROM (SELECT 1 AS key, 100 AS value) src | ||
| |MAP src.*, src.key, CAST(src.key / 10 AS INT), CAST(src.key % 10 AS INT), src.value | ||
| |USING 'cat' AS (k, v, tkey, ten, one, tvalue) | ||
| """.stripMargin), Row("1", "100", "1", "0", "1", "100")) | ||
| } | ||
| } | ||
|
|
||
| @SlowHiveTest | ||
|
|
||

Uh oh!
There was an error while loading. Please reload this page.