[SPARK-16947][SQL] Improve type coercion for inline tables. #14539

hvanhovell · 2016-08-08T11:25:04Z

What changes were proposed in this pull request?

Inline tables were added in to Spark SQL in 2.0, e.g.: select * from values (1, 'A'), (2, 'B') as tbl(a, b)

This is currently implemented using a LocalRelation and this relation is created during parsing. This has a weakness: type coercion is based on the first row in the relation, and all subsequent values are cast in to this type. The latter violates the principle of least surprise.

This PR fixes this by creating a dedicated InlineTable node. Type coercion now follows the rules for Union, which is similar to other systems like PostgreSQL. In order to retain optimal speed, I have extended the ConvertToLocalRelation, which makes sure the table gets rewritten into a LocalRelation during optimization.

The following SQL statement:

select * from values (1, exp(1)), (2.00, 'b'), (3.0, 'tt'), (40.0001, 4) x(a, b)

... now yields the following plan:

== Parsed Logical Plan ==
'Project [*]
+- 'SubqueryAlias x
   +- 'InlineTable [ArrayBuffer(1 AS a#0, 'exp(1) AS b#1), ArrayBuffer(2.00 AS a#2, b AS b#3), ArrayBuffer(3.0 AS a#4, tt AS b#5), ArrayBuffer(40.0001 AS a#6, 4 AS b#7)]

== Analyzed Logical Plan ==
1 AS a#0: decimal(14,4), EXP(cast(1 as double)) AS b#1: string
Project [1 AS a#0#18, EXP(cast(1 as double)) AS b#1#19]
+- SubqueryAlias x
   +- InlineTable [ArrayBuffer(cast(1 as decimal(14,4)) AS 1 AS a#0#18, cast(EXP(cast(1 as double)) as string) AS EXP(cast(1 as double)) AS b#1#19), ArrayBuffer(cast(2.00 as decimal(14,4)) AS 2.00 AS a#2#20, b AS b#3), ArrayBuffer(cast(3.0 as decimal(14,4)) AS 3.0 AS a#4#21, tt AS b#5), ArrayBuffer(cast(40.0001 as decimal(14,4)) AS 40.0001 AS a#6#22, cast(4 as string) AS 4 AS b#7#23)]

== Optimized Logical Plan ==
LocalRelation [1 AS a#0#18, EXP(cast(1 as double)) AS b#1#19]

== Physical Plan ==
LocalTableScan [1 AS a#0#18, EXP(cast(1 as double)) AS b#1#19]

... and the following result:

+-------+-----------------+
|      a|                b|
+-------+-----------------+
| 1.0000|2.718281828459045|
| 2.0000|                b|
| 3.0000|               tt|
|40.0001|                4|
+-------+-----------------+

How was this patch tested?

I have updated the PlanParseSuite to test the parsers the new output, and I have added tests to the ConvertToLocalRelationSuite to tests if inline table (a-like) structures are converted into LocalRelations. I still need to add tests for the TypeCoercion and CheckAnalysis rules.

hvanhovell · 2016-08-08T11:26:03Z

cc @cloud-fan @eyalfa

eyalfa · 2016-08-08T12:24:13Z

@hvanhovell in case of validation error, how will the error message look like? will it mention inline-tables in any way, or is it just going to complain about Union's requirements?

SparkQA · 2016-08-08T12:40:27Z

Test build #63358 has finished for PR 14539 at commit 3b0a28b.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

hvanhovell · 2016-08-08T12:41:44Z

@eyalfa It is currently going to complain about Unions (if type coercion fails).

eyalfa · 2016-08-08T13:04:43Z

@hvanhovell don't you think that if you're already taking a stab at it, it's better to introduce something like UnresolvedInlineTable with its own resolution logic and type coercion, once it's resolved it can be transformed into a Union of simple projects.

hvanhovell · 2016-08-08T13:17:05Z

@eyalfa I am a bit hesitant to add yet another almost pointless LogicalPlan node to Catalyst, and certainly not one that is functionally exactly the same as a Union. This would require us to add another rule to the analyzer/optimizer and we would have to add a separate case to type coercion.

I do think your point has merit and that errors should be as concise as possible, but I would (if we were to change this) rather add some sort of an alias which encodes this information or just add a flag to Union.

eyalfa · 2016-08-08T13:29:15Z

fair enough, I think it's worth adding a negative test just to see what we're dealing with.
btw, will the error message/location somehow point back to the inline table?

SparkQA · 2016-08-08T14:39:12Z

Test build #63360 has finished for PR 14539 at commit 3f3aa93.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-08-08T17:54:33Z

Test build #63366 has finished for PR 14539 at commit 9a827ce.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

rxin · 2016-08-08T22:19:03Z

Can we create some common function used by both union and this? It seems like a pretty complicated plan to do this via union.

SparkQA · 2016-08-08T23:05:15Z

Test build #63378 has finished for PR 14539 at commit 392eb0a.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- abstract class AbstractOneRowRelation extends LeafNode

cloud-fan · 2016-08-09T00:48:52Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

+    val numExpectedColumns = rows.head.size
+    val aliases = if (ctx.identifierList != null) {
+      val names = visitIdentifierList(ctx.identifierList)
+      assert(names.size == numExpectedColumns,


This is an error case users can hit, should we throw ParserException instead of assert?

It uses a parser only version of assert that throws a ParseException: https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/ParserUtils.scala#L81

Come to think of it, we might need to rename it because people expect that assert calls can be elided. That is for a different PR though.

hvanhovell · 2016-08-09T13:58:42Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala

- * another LocalRelation.
- *
- * This is relatively simple as it currently handles only a single case: Project.
+ * Converts local operations (i.e. ones that don't require data exchange) on LocalRelation or


Update comment.

cloud-fan · 2016-08-09T14:28:04Z

@hvanhovell , I may miss something, why do we create this new InlineTable instead of using Union? I think we can create a special OneRowRelation(e.g. UnfoldableOneRowRelarion) in test scope and use it in ExpressionEvalHelper.

SparkQA · 2016-08-09T15:12:25Z

Test build #63442 has finished for PR 14539 at commit daa01a2.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class InlineTable(rows: Seq[Seq[NamedExpression]]) extends LeafNode

hvanhovell · 2016-08-09T16:35:20Z

@cloud-fan I had an offline discussion with @rxin about this. His main point was that a larger inline table would create an extremely unreadable plan. So I came up with this.

cloud-fan · 2016-08-10T07:07:06Z

@hvanhovell how about we make InlineTable an unresolved plan and resolve it to LocalRelation in analyzer? It's weird that we don't have planner rules for InlineTable and depend on optimizer to convert it to LocalRelation. Besides, we can put alias names as a field in InlineTable instead of alias every expression in every row, because we don't need to define output for InlineTable it it's an unresolved plan.

petermaxlee · 2016-08-16T19:57:26Z

@hvanhovell do you mind me taking a look at this? I am running into an issue in which I cannot use array() function to construct an array in inline tables (only literals are allowed). I can try fix the type coercion issue there too.

hvanhovell · 2016-08-16T20:09:45Z

@petermaxlee sure go ahead!

petermaxlee · 2016-08-16T20:22:36Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

+    // Create expressions.
+    val rows = ctx.expression.asScala.map { e =>
+      expression(e) match {
+        case CreateStruct(children) => children


@hvanhovell what's this about? Why do we need to expand struct?

Actually I think I understand what's happening here now.

The parser creates rows by issuing CreateStruct commands. Inline table takes a Seq[Expression] per row. So we need to extracts the children from the CreateStruct.

gatorsmile · 2016-08-17T05:02:31Z

How to specify null when we creating inline table?

    sql(
      """
        |create temporary view src as select * from values
        |(201, null),
        |(86, "val_86")
        |as data(key, value)
      """.stripMargin)

Is that supported?

hvanhovell · 2016-08-17T07:35:11Z

@gatorsmile we should support this, but you might have to add an explicit cast.

hvanhovell · 2016-08-17T07:35:36Z

closing in favor of #14676

Improve inline table processing.

3b0a28b

Use project instead of direct expression evaluation.

3f3aa93

Fix tests using Union with foldable children.

9a827ce

Add fixed one row relation

392eb0a

cloud-fan reviewed Aug 9, 2016
View reviewed changes

Create an InlineTable LogicalPlan

daa01a2

hvanhovell reviewed Aug 9, 2016
View reviewed changes

rxin mentioned this pull request Aug 10, 2016

[SPARK-16866][SQL] Infrastructure for file-based SQL end-to-end tests #14472

Closed

petermaxlee reviewed Aug 16, 2016
View reviewed changes

hvanhovell closed this Aug 17, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-16947][SQL] Improve type coercion for inline tables. #14539

[SPARK-16947][SQL] Improve type coercion for inline tables. #14539

hvanhovell commented Aug 8, 2016 •

edited

Loading

hvanhovell commented Aug 8, 2016

eyalfa commented Aug 8, 2016

SparkQA commented Aug 8, 2016

hvanhovell commented Aug 8, 2016

eyalfa commented Aug 8, 2016

hvanhovell commented Aug 8, 2016

eyalfa commented Aug 8, 2016 •

edited

Loading

SparkQA commented Aug 8, 2016

SparkQA commented Aug 8, 2016

rxin commented Aug 8, 2016

SparkQA commented Aug 8, 2016

cloud-fan Aug 9, 2016

hvanhovell Aug 9, 2016

rxin Aug 12, 2016

hvanhovell Aug 9, 2016

cloud-fan commented Aug 9, 2016

SparkQA commented Aug 9, 2016

hvanhovell commented Aug 9, 2016

cloud-fan commented Aug 10, 2016

petermaxlee commented Aug 16, 2016

hvanhovell commented Aug 16, 2016

petermaxlee Aug 16, 2016

petermaxlee Aug 16, 2016

hvanhovell Aug 16, 2016

gatorsmile commented Aug 17, 2016 •

edited

Loading

hvanhovell commented Aug 17, 2016

hvanhovell commented Aug 17, 2016

[SPARK-16947][SQL] Improve type coercion for inline tables. #14539

[SPARK-16947][SQL] Improve type coercion for inline tables. #14539

Conversation

hvanhovell commented Aug 8, 2016 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

hvanhovell commented Aug 8, 2016

eyalfa commented Aug 8, 2016

SparkQA commented Aug 8, 2016

hvanhovell commented Aug 8, 2016

eyalfa commented Aug 8, 2016

hvanhovell commented Aug 8, 2016

eyalfa commented Aug 8, 2016 • edited Loading

SparkQA commented Aug 8, 2016

SparkQA commented Aug 8, 2016

rxin commented Aug 8, 2016

SparkQA commented Aug 8, 2016

cloud-fan Aug 9, 2016

Choose a reason for hiding this comment

hvanhovell Aug 9, 2016

Choose a reason for hiding this comment

rxin Aug 12, 2016

Choose a reason for hiding this comment

hvanhovell Aug 9, 2016

Choose a reason for hiding this comment

cloud-fan commented Aug 9, 2016

SparkQA commented Aug 9, 2016

hvanhovell commented Aug 9, 2016

cloud-fan commented Aug 10, 2016

petermaxlee commented Aug 16, 2016

hvanhovell commented Aug 16, 2016

petermaxlee Aug 16, 2016

Choose a reason for hiding this comment

petermaxlee Aug 16, 2016

Choose a reason for hiding this comment

hvanhovell Aug 16, 2016

Choose a reason for hiding this comment

gatorsmile commented Aug 17, 2016 • edited Loading

hvanhovell commented Aug 17, 2016

hvanhovell commented Aug 17, 2016

hvanhovell commented Aug 8, 2016 •

edited

Loading

eyalfa commented Aug 8, 2016 •

edited

Loading

gatorsmile commented Aug 17, 2016 •

edited

Loading