Skip to content

[SPARK-24870][SQL]Cache can't work normally if there are case letters in SQL #21823

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

[SPARK-24870][SQL]Cache can't work normally if there are case letters in SQL #21823

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2b2a5a3
Cache can't work normally if there are case letters in SQL
eatoncys Jul 20, 2018
c01cf89
erase the attribute name
eatoncys Jul 20, 2018
1aefcb3
erase the attribute name
eatoncys Jul 20, 2018
b5b2a1b
using canonicalized
eatoncys Jul 20, 2018
86c7ed6
using canonicalized
eatoncys Jul 20, 2018
f2091a4
project
eatoncys Jul 21, 2018
f3a7963
Canonicalized result is case-insensitive
eatoncys Jul 24, 2018
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
8 changes: 5 additions & 3 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/QueryPlan.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

package org.apache.spark.sql.catalyst.plans

import java.util.Locale

import org.apache.spark.sql.catalyst.expressions._
import org.apache.spark.sql.catalyst.trees.{CurrentOrigin, TreeNode}
import org.apache.spark.sql.internal.SQLConf
Expand Down Expand Up @@ -237,7 +239,7 @@ abstract class QueryPlan[PlanType <: QueryPlan[PlanType]] extends TreeNode[PlanT
// Top level `AttributeReference` may also be used for output like `Alias`, we should
// normalize the epxrId too.
id += 1
ar.withExprId(ExprId(id)).canonicalized
ar.withExprId(ExprId(id)).withName(ar.name.toLowerCase(Locale.ROOT)).canonicalized

@cloud-fan cloud-fan Jul 20, 2018

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 just erase the attribute name like alias?

@eatoncys eatoncys Jul 20, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think it is Ok, and it erase the attribute name in spark version 2.0.2


case other => QueryPlan.normalizeExprId(other, allAttributes)
}.withNewChildren(canonicalizedChildren)
Expand Down Expand Up @@ -282,9 +284,9 @@ object QueryPlan extends PredicateHelper {
case ar: AttributeReference =>
val ordinal = input.indexOf(ar.exprId)
if (ordinal == -1) {
ar
ar.withName(ar.name.toLowerCase(Locale.ROOT))
} else {
ar.withExprId(ExprId(ordinal))
ar.withExprId(ExprId(ordinal)).withName(ar.name.toLowerCase(Locale.ROOT))
}
}.canonicalized.asInstanceOf[T]
}
Expand Down
30 changes: 29 additions & 1 deletion ...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CanonicalizeSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@
package org.apache.spark.sql.catalyst.expressions

import org.apache.spark.SparkFunSuite
import org.apache.spark.sql.catalyst.dsl.expressions._
import org.apache.spark.sql.catalyst.dsl.plans._
import org.apache.spark.sql.catalyst.plans.logical.Range
import org.apache.spark.sql.catalyst.plans.logical.{Aggregate, LocalRelation, Range}
import org.apache.spark.sql.types.LongType

class CanonicalizeSuite extends SparkFunSuite {

Expand Down Expand Up @@ -50,4 +52,30 @@ class CanonicalizeSuite extends SparkFunSuite {
assert(range.where(arrays1).sameResult(range.where(arrays2)))
assert(!range.where(arrays1).sameResult(range.where(arrays3)))
}

test("Canonicalized result is not case-insensitive") {

@cloud-fan cloud-fan Jul 20, 2018

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.

let's move it to SameResultSuite, also let's pick a simpler test, like using a Project with one columns instead of Aggregate.

@eatoncys eatoncys Jul 20, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok,modified,thanks.

val u1 = 'A.string.at(0)
val u2 = 'B.string.at(1)
val u3 = 'C.string.at(2)
val caseA = CaseWhen(Seq((u1, u2)), u3)
val otherA = AttributeReference("D", LongType)(exprId = ExprId(3))
val aliases = Alias(sum(caseA), "E")() :: Nil
val planUppercase = Aggregate(
Nil,
aliases,
LocalRelation(otherA))

val l1 = 'a.string.at(0)
val l2 = 'b.string.at(1)
val l3 = 'c.string.at(2)
val caseALower = CaseWhen(Seq((l1, l2)), l3)
val otherALower = AttributeReference("d", LongType)(exprId = ExprId(3))
val aliasesLower = Alias(sum(caseALower), "e")() :: Nil
val planLowercase = Aggregate(
Nil,
aliasesLower,
LocalRelation(otherALower))

assert(planUppercase.canonicalized == planLowercase.canonicalized)
}
}