Skip to content

Conversation

@wajda
Copy link
Contributor

@wajda wajda commented May 22, 2018

What changes were proposed in this pull request?

Fixes a scala.MatchError in the element_at operation - SPARK-24348

When calling element_at with a wrong first operand type an AnalysisException should be thrown instead of scala.MatchError

Example:

select element_at('foo', 1)

results in:

scala.MatchError: StringType (of class org.apache.spark.sql.types.StringType$)
	at org.apache.spark.sql.catalyst.expressions.ElementAt.inputTypes(collectionOperations.scala:1469)
	at org.apache.spark.sql.catalyst.expressions.ExpectsInputTypes$class.checkInputDataTypes(ExpectsInputTypes.scala:44)
	at org.apache.spark.sql.catalyst.expressions.ElementAt.checkInputDataTypes(collectionOperations.scala:1478)
	at org.apache.spark.sql.catalyst.expressions.Expression.resolved$lzycompute(Expression.scala:168)
	at org.apache.spark.sql.catalyst.expressions.Expression.resolved(Expression.scala:168)
	at org.apache.spark.sql.catalyst.analysis.Analyzer$ResolveAliases$$anonfun$org$apache$spark$sql$catalyst$analysis$Analyzer$ResolveAliases$$assignAliases$1$$anonfun$apply$3.applyOrElse(Analyzer.scala:256)
	at org.apache.spark.sql.catalyst.analysis.Analyzer$ResolveAliases$$anonfun$org$apache$spark$sql$catalyst$analysis$Analyzer$ResolveAliases$$assignAliases$1$$anonfun$apply$3.applyOrElse(Analyzer.scala:252)
	at org.apache.spark.sql.catalyst.trees.TreeNode$$anonfun$transformUp$1.apply(TreeNode.scala:289)
	at org.apache.spark.sql.catalyst.trees.TreeNode$$anonfun$transformUp$1.apply(TreeNode.scala:289)
	at org.apache.spark.sql.catalyst.trees.CurrentOrigin$.withOrigin(TreeNode.scala:70)
	at org.apache.spark.sql.catalyst.trees.TreeNode.transformUp(TreeNode.scala:288)

How was this patch tested?

unit tests

@kiszk
Copy link
Member

kiszk commented May 22, 2018

cc @ueshin

@wajda wajda changed the title SPARK-24348 "element_at" error fix [SPARK-24348][SQL] "element_at" error fix May 22, 2018
@ueshin
Copy link
Member

ueshin commented May 22, 2018

Good catch!
I guess we should check other functions while we are here. I found ArrayPosition has similar problem for now. Could you fix it as well?

@ueshin
Copy link
Member

ueshin commented May 22, 2018

ok to test.

@wajda
Copy link
Contributor Author

wajda commented May 22, 2018

Yep, just have fixed that one as well :)
#21401

)

intercept[AnalysisException] {
Seq(("a string element", 1)).toDF().selectExpr("element_at(_1, _2)")
Copy link
Member

Choose a reason for hiding this comment

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

Please check the error message, too.

@SparkQA
Copy link

SparkQA commented May 22, 2018

Test build #90974 has finished for PR 21395 at commit ec2e4a6.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Member

@gatorsmile gatorsmile left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks, Merged to master!

left.dataType match {
case _: ArrayType => IntegerType
case _: MapType => left.dataType.asInstanceOf[MapType].keyType
case _ => AnyDataType // no match for a wrong 'left' expression type
Copy link
Member

Choose a reason for hiding this comment

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

BTW, normally, we prefer to

  override def inputTypes: Seq[AbstractDataType] = {
    val rightInputType = left.dataType match {
      case _: ArrayType => IntegerType
      case _: MapType => left.dataType.asInstanceOf[MapType].keyType
      case _ => AnyDataType // no match for a wrong 'left' expression type
    }
    Seq(TypeCollection(ArrayType, MapType), rightInputType)
  }

@asfgit asfgit closed this in bc6ea61 May 22, 2018
@SparkQA
Copy link

SparkQA commented May 22, 2018

Test build #90986 has finished for PR 21395 at commit 045307b.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants