[SPARK-29505][SQL] Make DESC EXTENDED <table name> <column name> case insensitive #26927

PavithraRamachandran · 2019-12-17T11:57:16Z

What changes were proposed in this pull request?

While querying using desc , if column name is not entered exactly as per the column name given during the table creation, the colstats are wrong. fetching of col stats has been made case insensitive.

Why are the changes needed?

functions like analyze, etc support case insensitive retrieval of column data.

Does this PR introduce any user-facing change?

NO

How was this patch tested?

maropu · 2019-12-17T12:19:42Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala

-    val colStats = catalogTable.stats.map(_.colStats).getOrElse(Map.empty)
-    val cs = colStats.get(field.name)
+    val colStats = catalogTable.stats.map(_.colStats.map{ case (key, value) => key.toLowerCase -> value}).getOrElse(Map.empty)
+    val cs = colStats.get(field.name.toLowerCase())


Can you link this behaivour to SQLConf.caseSensitiveAnalysis?

@maropu @dongjoon-hyun i have done the changes. could you kindly review?

maropu · 2019-12-17T12:20:06Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

+      sql("insert into customer values(2,'Ana','trujilo','Adva de la','Maxico D.F.',05021,'Maxico')")
+      sql("insert into customer values(3,'Antonio','Antonio Moreno','Mataderos 2312','Maxico D.F.',05023,'Maxico')")
+      sql("analyze table customer compute statistics for columns cname")
+      val expectedData= Seq(


nit: add space befor =

maropu · 2019-12-17T12:20:18Z

ok to test

SparkQA · 2019-12-17T12:23:40Z

Test build #115451 has finished for PR 26927 at commit cfb4939.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2019-12-17T17:48:36Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala

@@ -720,8 +720,8 @@ case class DescribeColumnCommand(
    }

    val catalogTable = catalog.getTempViewOrPermanentTableMetadata(table)
-    val colStats = catalogTable.stats.map(_.colStats).getOrElse(Map.empty)
-    val cs = colStats.get(field.name)
+    val colStats = catalogTable.stats.map(_.colStats.map{ case (key, value) => key.toLowerCase -> value}).getOrElse(Map.empty)


Hi, @PavithraRamachandran .
Could you run dev/scalastyle and fix the errors?

dongjoon-hyun · 2019-12-17T17:49:05Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

+
+  test("SPARK-29505: desc columnname - case insensitive search") {
+    withTable("customer") {
+      sql(s"create table customer(id int, name String, CName String, address String, city String, pin int, country String)")


s" -> ".

Also, please rewrite like CREATE TABLE customer(id INT, ...).

dongjoon-hyun · 2019-12-17T17:50:27Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

+  test("SPARK-29505: desc columnname - case insensitive search") {
+    withTable("customer") {
+      sql(s"create table customer(id int, name String, CName String, address String, city String, pin int, country String)")
+      sql("insert into customer values(1,'Alfred','Maria','Obere Str 57','Berlin',12209,'Germany')")


INSERT INTO customer VALUES

dongjoon-hyun · 2019-12-17T17:50:58Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

+      sql("insert into customer values(1,'Alfred','Maria','Obere Str 57','Berlin',12209,'Germany')")
+      sql("insert into customer values(2,'Ana','trujilo','Adva de la','Maxico D.F.',05021,'Maxico')")
+      sql("insert into customer values(3,'Antonio','Antonio Moreno','Mataderos 2312','Maxico D.F.',05023,'Maxico')")
+      sql("analyze table customer compute statistics for columns cname")


ANALYZE TABLE customer COMPUTE STATISTICS FOR COLUMNS cname

SparkQA · 2019-12-18T03:48:14Z

Test build #115484 has finished for PR 26927 at commit 453e8df.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-12-18T04:45:54Z

Test build #115487 has finished for PR 26927 at commit 4b007e4.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-12-18T04:54:05Z

Test build #115488 has finished for PR 26927 at commit 9557a10.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-12-18T07:40:10Z

Test build #115492 has finished for PR 26927 at commit af0336f.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-12-18T11:43:04Z

Test build #115499 has finished for PR 26927 at commit 84b64fd.

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

SparkQA · 2019-12-18T13:04:20Z

Test build #115501 has finished for PR 26927 at commit f3792d9.

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

dongjoon-hyun · 2019-12-18T19:46:52Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala

+      catalogTable.stats.map(_.colStats.map {
+        case (key, value) => key.toLowerCase(Locale.ROOT) -> value
+      }).getOrElse(Map.empty)
+


Shall we remove this empty line?

dongjoon-hyun · 2019-12-18T19:55:10Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala

+    } else {
+      fieldName.toLowerCase(Locale.ROOT)
+    }
+  }


Shall we use the following format? (We use the following style in SessionCatalog.scala and Analyzer.scala.)

protected def formatColumnName(name: String): String = { if (conf.caseSensitiveAnalysis) name else name.toLowerCase(Locale.ROOT) }

dongjoon-hyun · 2019-12-19T00:23:02Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

+        sql("INSERT INTO customer VALUES(2,'Ana','trujilo','Adva de la'," +
+          "'Maxico D.F.',05021,'Maxico')")
+        sql("INSERT INTO customer VALUES(3,'Antonio','Antonio Moreno','Mataderos 2312'," +
+          "'Maxico D.F.',05023,'Maxico')")


Do we need to populate all data? It seems that we only use CName and the other are ignored completely.

dongjoon-hyun · 2019-12-19T00:41:26Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

@@ -3336,6 +3336,35 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession {
      checkAnswer(df5, Array.empty[Row])
    }
  }
+
+  test("SPARK-29505: desc columnname - case insensitive search") {


Could you try to move this test to describe-table-column.sql? You can use SQL there. Please refer SQLQueryTestSuite for how to use.

+1 for the move.

@dongjoon-hyun and @maropu i have addressed the review comments could you kindly verify?

SparkQA · 2019-12-19T14:45:39Z

Test build #115564 has finished for PR 26927 at commit 27d51c3.

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

SparkQA · 2019-12-19T16:00:10Z

Test build #115567 has finished for PR 26927 at commit 7e85a34.

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

maropu · 2019-12-20T02:16:11Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala

-    val colStats = catalogTable.stats.map(_.colStats).getOrElse(Map.empty)
-    val cs = colStats.get(field.name)
+    val colStats = getColStats(catalogTable)
+    val cs = colStats.get(getColumnName(field.name))



How about just simply writing it like this?

val catalogStats = catalog.getTempViewOrPermanentTableMetadata(table).stats val cs = if (conf.caseSensitiveAnalysis) { catalogStats.flatMap { cs => cs.colStats.get(field.name) } } else { catalogStats.flatMap { cs => cs.colStats.map { case (k, v) => (k.toLowerCase(Locale.ROOT), v) } .get(field.name.toLowerCase(Locale.ROOT)) } }

I have reworked on the change suggested. Could you review?

SparkQA · 2019-12-20T06:47:01Z

Test build #115597 has finished for PR 26927 at commit afe659c.

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

PavithraRamachandran · 2019-12-23T01:43:22Z

@maropu @dongjoon-hyun could you kindly review the changes? I have reworked on the comments

HyukjinKwon · 2019-12-24T09:43:27Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala

+        case (key, value) => key.toLowerCase(Locale.ROOT) -> value
+      }).getOrElse(Map.empty)
+    }
+    val cs = colStats.get(getColumnName(field.name))


Can we just do:

val colStatsMap = catalogTable.stats.map(_.colStats).getOrElse(Map.empty) val colStats = if (conf.caseSensitiveAnalysis) colStatsMap else CaseInsensitiveMap(colStatsMap)

? I think this will fix all problems here.

Ur, I totally forgot CaseInsensitiveMap... nice suggestion.

SparkQA · 2019-12-24T11:15:48Z

Test build #115734 has finished for PR 26927 at commit ac5b097.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

…ive manner.Converting the column name which is the key to lower case and searching using lower case whatever may be the input value.

HyukjinKwon

Looks good if tests pass

SparkQA · 2019-12-24T15:36:51Z

Test build #115736 has finished for PR 26927 at commit 096d2e3.

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

SparkQA · 2019-12-24T16:21:32Z

Test build #115738 has finished for PR 26927 at commit 5ff17ae.

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

maropu · 2019-12-24T23:58:10Z

Thanks! Merged to master.

… insensitive ### What changes were proposed in this pull request? While querying using **desc** , if column name is not entered exactly as per the column name given during the table creation, the colstats are wrong. fetching of col stats has been made case insensitive. ### Why are the changes needed? functions like **analyze**, etc support case insensitive retrieval of column data. ### Does this PR introduce any user-facing change? NO ### How was this patch tested? <!-- Unit test has been rewritten and tested. Closes apache#26927 from PavithraRamachandran/desc_caseinsensitive. Authored-by: Pavithra Ramachandran <pavi.rams@gmail.com> Signed-off-by: Takeshi Yamamuro <yamamuro@apache.org>

maropu reviewed Dec 17, 2019

View reviewed changes

dongjoon-hyun reviewed Dec 17, 2019

View reviewed changes

PavithraRamachandran force-pushed the desc_caseinsensitive branch from 453e8df to 4b007e4 Compare December 18, 2019 04:42

PavithraRamachandran force-pushed the desc_caseinsensitive branch from af0336f to 84b64fd Compare December 18, 2019 08:56

dongjoon-hyun changed the title ~~[SPARK-29505][SQL] desc extended <table name> <column name> to be case insensitive~~ [SPARK-29505][SQL] Make DESC EXTENDED <table name> <column name> case insensitive Dec 18, 2019

dongjoon-hyun reviewed Dec 18, 2019

View reviewed changes

dongjoon-hyun added the SQL label Dec 18, 2019

dongjoon-hyun reviewed Dec 18, 2019

View reviewed changes

dongjoon-hyun reviewed Dec 19, 2019

View reviewed changes

PavithraRamachandran force-pushed the desc_caseinsensitive branch from 6f7c873 to 27d51c3 Compare December 19, 2019 10:39

maropu reviewed Dec 20, 2019

View reviewed changes

HyukjinKwon reviewed Dec 24, 2019

View reviewed changes

PavithraRamachandran force-pushed the desc_caseinsensitive branch from ac5b097 to 096d2e3 Compare December 24, 2019 11:38

the column statistics received from hive is compared in a case sensit…

5ff17ae

…ive manner.Converting the column name which is the key to lower case and searching using lower case whatever may be the input value.

PavithraRamachandran force-pushed the desc_caseinsensitive branch from 096d2e3 to 5ff17ae Compare December 24, 2019 12:28

HyukjinKwon approved these changes Dec 24, 2019

View reviewed changes

maropu approved these changes Dec 24, 2019

View reviewed changes

maropu closed this in 57ca952 Dec 24, 2019

[SPARK-29505][SQL] Make DESC EXTENDED <table name> <column name> case insensitive #26927

[SPARK-29505][SQL] Make DESC EXTENDED <table name> <column name> case insensitive #26927

Conversation

PavithraRamachandran commented Dec 17, 2019

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maropu commented Dec 17, 2019

SparkQA commented Dec 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Dec 18, 2019

SparkQA commented Dec 18, 2019

SparkQA commented Dec 18, 2019

SparkQA commented Dec 18, 2019

SparkQA commented Dec 18, 2019

SparkQA commented Dec 18, 2019

Choose a reason for hiding this comment

dongjoon-hyun Dec 18, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Dec 19, 2019

SparkQA commented Dec 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Dec 20, 2019

PavithraRamachandran commented Dec 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Dec 24, 2019

HyukjinKwon left a comment

Choose a reason for hiding this comment

SparkQA commented Dec 24, 2019

SparkQA commented Dec 24, 2019

maropu commented Dec 24, 2019

dongjoon-hyun Dec 18, 2019 •

edited