Skip to content
Closed
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ class Analyzer(
GlobalAggregates ::
ResolveAggregateFunctions ::
TimeWindowing ::
ResolveInlineTables ::
TypeCoercion.typeCoercionRules ++
extendedResolutionRules : _*),
Batch("Nondeterministic", Once,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.spark.sql.catalyst.analysis

import org.apache.spark.sql.catalyst.InternalRow
import org.apache.spark.sql.catalyst.expressions.Cast
import org.apache.spark.sql.catalyst.plans.logical.{LocalRelation, LogicalPlan}
import org.apache.spark.sql.catalyst.rules.Rule
import org.apache.spark.sql.types.{StructField, StructType}

/**
* An analyzer rule that replaces [[UnresolvedInlineTable]] with [[LocalRelation]].
*/
object ResolveInlineTables extends Rule[LogicalPlan] {
override def apply(plan: LogicalPlan): LogicalPlan = plan transformUp {
case table: UnresolvedInlineTable if table.expressionsResolved =>
validateInputDimension(table)
validateInputFoldable(table)
convert(table)
}

/**
* Validates that all inline table data are foldable expressions.
*
* This is publicly visible for unit testing.
Copy link
Contributor

Choose a reason for hiding this comment

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

The test is located in the same package, so you could reduce visibility to protected/package.

*/
def validateInputFoldable(table: UnresolvedInlineTable): Unit = {
table.rows.foreach { row =>
row.foreach { e =>
if (!e.resolved || !e.foldable) {
e.failAnalysis(s"cannot evaluate expression ${e.sql} in inline table definition")
Copy link
Contributor

Choose a reason for hiding this comment

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

You could also mention the row and the column for better UX.

}
}
}
}

/**
* Validates the input data dimension:
* 1. All rows have the same cardinality.
* 2. The number of column aliases defined is consistent with the number of columns in data.
*
* This is publicly visible for unit testing.
*/
def validateInputDimension(table: UnresolvedInlineTable): Unit = {
if (table.rows.nonEmpty) {
val numCols = table.rows.head.size
table.rows.zipWithIndex.foreach { case (row, ri) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we just get the table.names.size first and iterate the rows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea. Let me do that.

if (row.size != numCols) {
table.failAnalysis(s"expected $numCols columns but found ${row.size} columns in row $ri")
}
}

if (table.names.size != numCols) {
table.failAnalysis(s"expected ${table.names.size} columns but found $numCols in first row")
}
}
}

/**
* Convert a valid (with right shape and foldable inputs) [[UnresolvedInlineTable]]
* into a [[LocalRelation]].
*
* This function attempts to coerce inputs into consistent types.
*
* This is publicly visible for unit testing.
*/
def convert(table: UnresolvedInlineTable): LocalRelation = {
val numCols = table.rows.head.size

// For each column, traverse all the values and find a common data type.
val targetTypes = Seq.tabulate(numCols) { ci =>
Copy link
Contributor

@hvanhovell hvanhovell Aug 17, 2016

Choose a reason for hiding this comment

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

It would be nice if we would create the entire schema here (including nullability).

You could also transpose the Seq[Seq[Expression]] to do this. Something like: table.rows.transpose.zip(table.names).map { case (column, name) => ... }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't check for nullability, since I don't think it actually matters much for the purpose we are using this feature. What would be useful is to be able to define the data type explicitly, and then we can do controlled tests for nullability.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this might be ok for most testing scenario's. However we are currently uncovering all sorts of tricky nullable bugs in the optimizer; it is useful in such a case to be able to control nullability. The other thing is that these features tend to be used in production, and then these things start to matter. Finally it is literally a one-liner column.exists(_.nullable) (if you combine this with creating the schema).

val inputTypes = table.rows.map(_(ci).dataType)
TypeCoercion.findWiderTypeWithoutStringPromotion(inputTypes).getOrElse {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you check with other databases? Should we do string promotion for inline table? FYI expressions in Union can promote to string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Postgres doesn't allow it. We can choose to be consistent with union though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a strong preference, cc @hvanhovell

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets go with the Postgres semantics.

table.failAnalysis(s"incompatible types found in column $ci for inline table")
}
}
assert(targetTypes.size == table.names.size)
Copy link
Contributor

Choose a reason for hiding this comment

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

it's duplicated, validateInputDimension already guarantees table.names.size is equal to number of column

Copy link
Contributor Author

Choose a reason for hiding this comment

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

asserts are not meant to be user facing. They are meant to be defensive against programming errors (i.e. bugs in Spark).


val newRows: Seq[InternalRow] = table.rows.map { row =>
InternalRow.fromSeq(row.zipWithIndex.map { case (e, ci) =>
val targetType = targetTypes(ci)
if (e.dataType.sameType(targetType)) {
e.eval()
Copy link
Contributor

Choose a reason for hiding this comment

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

Try this with a rand() or any other nondeterministic expression and it will fail. In order to support these you have to create an InterpretedProjection per row and use this once, see the following code for a similar situation: https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala#L1575-L1576

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do we determine if something is valid here? If we don't do a foldable check and want to support nondeterministic functions, how do we rule out something like count(1)?

Copy link
Contributor

Choose a reason for hiding this comment

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

You should be fine if you rule out Unevaluable expressions. Count is one of those.

} else {
Cast(e, targetType).eval()
}
})
}

val attributes = StructType(targetTypes.zip(table.names)
.map { case (typ, name) => StructField(name, typ) }).toAttributes
Copy link
Contributor

@hvanhovell hvanhovell Aug 17, 2016

Choose a reason for hiding this comment

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

The fields are created with nullable = true, it would be easy and valuable to infer nullability from the expressions.

LocalRelation(attributes, newRows)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ object TypeCoercion {
* [[findTightestCommonType]], but can handle decimal types. If the wider decimal type exceeds
* system limitation, this rule will truncate the decimal type before return it.
*/
private def findWiderTypeWithoutStringPromotion(types: Seq[DataType]): Option[DataType] = {
def findWiderTypeWithoutStringPromotion(types: Seq[DataType]): Option[DataType] = {
types.foldLeft[Option[DataType]](Some(NullType))((r, c) => r match {
case Some(d) => findTightestCommonTypeOfTwo(d, c).orElse((d, c) match {
case (t1: DecimalType, t2: DecimalType) =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,23 @@ case class UnresolvedRelation(
override lazy val resolved = false
}

/**
* An inline table that has not been resolved yet. Once resolved, it is turned by the analyzer into
* a [[org.apache.spark.sql.catalyst.plans.logical.LocalRelation]].
*
* @param names list of column names
* @param rows expressions for the data
*/
case class UnresolvedInlineTable(
names: Seq[String],
rows: Seq[Seq[Expression]])
extends LeafNode {

lazy val expressionsResolved: Boolean = rows.forall(_.forall(_.resolved))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is used only once. Lets move this code into that location.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do want this memoized, so a lazy val is better here.

override def output: Seq[Attribute] = Nil
override lazy val resolved = false
}

/**
* Holds the name of an attribute that has yet to be resolved.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -662,39 +662,21 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with Logging {
*/
override def visitInlineTable(ctx: InlineTableContext): LogicalPlan = withOrigin(ctx) {
// Get the backing expressions.
val expressions = ctx.expression.asScala.map { eCtx =>
val e = expression(eCtx)
validate(e.foldable, "All expressions in an inline table must be constants.", eCtx)
e
}

// Validate and evaluate the rows.
val (structType, structConstructor) = expressions.head.dataType match {
case st: StructType =>
(st, (e: Expression) => e)
case dt =>
val st = CreateStruct(Seq(expressions.head)).dataType
(st, (e: Expression) => CreateStruct(Seq(e)))
}
val rows = expressions.map {
case expression =>
val safe = Cast(structConstructor(expression), structType)
safe.eval().asInstanceOf[InternalRow]
val rows = ctx.expression.asScala.map { e =>
expression(e) match {
case CreateStruct(children) => children
case child => Seq(child)
}
}

// Construct attributes.
val baseAttributes = structType.toAttributes.map(_.withNullability(true))
val attributes = if (ctx.identifierList != null) {
val aliases = visitIdentifierList(ctx.identifierList)
validate(aliases.size == baseAttributes.size,
"Number of aliases must match the number of fields in an inline table.", ctx)
baseAttributes.zip(aliases).map(p => p._1.withName(p._2))
val aliases = if (ctx.identifierList != null) {
visitIdentifierList(ctx.identifierList)
} else {
baseAttributes
Seq.tabulate(rows.head.size)(i => s"col${i + 1}")
}

// Create plan and add an alias if a name has been defined.
LocalRelation(attributes, rows).optionalMap(ctx.identifier)(aliasPlan)
val table = UnresolvedInlineTable(aliases, rows)
table.optionalMap(ctx.identifier)(aliasPlan)
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.spark.sql.catalyst.analysis

import org.scalatest.BeforeAndAfter

import org.apache.spark.sql.AnalysisException
import org.apache.spark.sql.catalyst.expressions.{Literal, Rand}
import org.apache.spark.sql.catalyst.plans.PlanTest
import org.apache.spark.sql.types.LongType

/**
* Unit tests for [[ResolveInlineTables]]. Note that there are also test cases defined in
* end-to-end tests (in sql/core module) for verifying the correct error messages are shown
* in negative cases.
*/
class ResolveInlineTablesSuite extends PlanTest with BeforeAndAfter {

private def lit(v: Any): Literal = Literal(v)

test("validate inputs are foldable") {
ResolveInlineTables.validateInputFoldable(
UnresolvedInlineTable(Seq("c1", "c2"), Seq(Seq(lit(1)))))

// nondeterministic (rand)
intercept[AnalysisException] {
ResolveInlineTables.validateInputFoldable(
UnresolvedInlineTable(Seq("c1", "c2"), Seq(Seq(Rand(1)))))
}

// unresolved attribute
intercept[AnalysisException] {
ResolveInlineTables.validateInputFoldable(
UnresolvedInlineTable(Seq("c1", "c2"), Seq(Seq(UnresolvedAttribute("A")))))
Copy link
Contributor

Choose a reason for hiding this comment

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

how about UnresolvedInlineTable(Seq("c1", "c2"), Seq(Seq(AttributeReference("A") + 1)))?

Copy link
Contributor

Choose a reason for hiding this comment

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

the Add will be resolved and evaluable, but not foldable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But how would a user construct an AttributeReference?

}
}

test("validate input dimensions") {
ResolveInlineTables.validateInputDimension(
UnresolvedInlineTable(Seq("c1"), Seq(Seq(lit(1)), Seq(lit(2)))))

// num alias != data dimension
intercept[AnalysisException] {
ResolveInlineTables.validateInputDimension(
UnresolvedInlineTable(Seq("c1", "c2"), Seq(Seq(lit(1)), Seq(lit(2)))))
}

// num alias == data dimension, but data themselves are inconsistent
intercept[AnalysisException] {
ResolveInlineTables.validateInputDimension(
UnresolvedInlineTable(Seq("c1"), Seq(Seq(lit(1)), Seq(lit(21), lit(22)))))
}
}

test("do not fire the rule if not all expressions are resolved") {
val table = UnresolvedInlineTable(Seq("c1", "c2"), Seq(Seq(UnresolvedAttribute("A"))))
assert(ResolveInlineTables(table) == table)
}

test("convert") {
val table = UnresolvedInlineTable(Seq("c1"), Seq(Seq(lit(1)), Seq(lit(2L))))
val converted = ResolveInlineTables.convert(table)

assert(converted.output.map(_.dataType) == Seq(LongType))
assert(converted.data.size == 2)
assert(converted.data(0).getLong(0) == 1L)
assert(converted.data(1).getLong(0) == 2L)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ package org.apache.spark.sql.catalyst.parser

import org.apache.spark.sql.Row
import org.apache.spark.sql.catalyst.FunctionIdentifier
import org.apache.spark.sql.catalyst.analysis.UnresolvedGenerator
import org.apache.spark.sql.catalyst.analysis.{UnresolvedGenerator, UnresolvedInlineTable}
import org.apache.spark.sql.catalyst.expressions._
import org.apache.spark.sql.catalyst.plans._
import org.apache.spark.sql.catalyst.plans.logical._
Expand Down Expand Up @@ -427,19 +427,14 @@ class PlanParserSuite extends PlanTest {
}

test("inline table") {
assertEqual("values 1, 2, 3, 4", LocalRelation.fromExternalRows(
Seq('col1.int),
Seq(1, 2, 3, 4).map(x => Row(x))))
assertEqual("values 1, 2, 3, 4",
UnresolvedInlineTable(Seq("col1"), Seq(1, 2, 3, 4).map(x => Seq(Literal(x)))))

assertEqual(
"values (1, 'a'), (2, 'b'), (3, 'c') as tbl(a, b)",
LocalRelation.fromExternalRows(
Seq('a.int, 'b.string),
Seq((1, "a"), (2, "b"), (3, "c")).map(x => Row(x._1, x._2))).as("tbl"))
intercept("values (a, 'a'), (b, 'b')",
"All expressions in an inline table must be constants.")
intercept("values (1, 'a'), (2, 'b') as tbl(a, b, c)",
"Number of aliases must match the number of fields in an inline table.")
intercept[ArrayIndexOutOfBoundsException](parsePlan("values (1, 'a'), (2, 'b', 5Y)"))
"values (1, 'a'), (2, 'b') as tbl(a, b)",
UnresolvedInlineTable(
Seq("a", "b"),
Seq(Literal(1), Literal("a")) :: Seq(Literal(2), Literal("b")) :: Nil).as("tbl"))
}

test("simple select query with !> and !<") {
Expand Down
39 changes: 39 additions & 0 deletions sql/core/src/test/resources/sql-tests/inputs/inline-table.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@

-- single row, without table and column alias
select * from values ("one", 1);

-- single row, without column alias
select * from values ("one", 1) as data;

-- single row
select * from values ("one", 1) as data(a, b);
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a case for NULL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added


-- two rows
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: 3 rows

select * from values ("one", 1), ("two", 2) as data(a, b);

-- int and long coercion
select * from values ("one", 1), ("two", 2L) as data(a, b);

-- foldable expressions
select * from values ("one", 1 + 0), ("two", 1 + 3L) as data(a, b);

-- complex types
select * from values ("one", array(0, 1)), ("two", array(2, 3)) as data(a, b);

-- decimal and double coercion
select * from values ("one", 2.0), ("two", 3.0D) as data(a, b);

-- error reporting: different number of columns
select * from values ("one", 2.0), ("two") as data(a, b);

-- error reporting: types that are incompatible
select * from values ("one", array(0, 1)), ("two", struct(1, 2)) as data(a, b);

-- error reporting: number aliases different from number data values
select * from values ("one"), ("two") as data(a, b);

-- error reporting: unresolved expression
select * from values ("one", random_not_exist_func(1)), ("two", 2) as data(a, b);

-- error reporting: aggregate expression
select * from values ("one", count(1)), ("two", 2) as data(a, b);
Loading