[SPARK-19912][SQL] String literals should be escaped for Hive metastore partition pruning #17266

dongjoon-hyun · 2017-03-12T10:15:29Z

What changes were proposed in this pull request?

Since current HiveShim's convertFilters does not escape the string literals. There exists the following correctness issues. This PR aims to return the correct result and also shows the more clear exception message.

BEFORE

scala> Seq((1, "p1", "q1"), (2, "p1\" and q=\"q1", "q2")).toDF("a", "p", "q").write.partitionBy("p", "q").saveAsTable("t1")
                                                                                
scala> spark.table("t1").filter($"p" === "p1\" and q=\"q1").select($"a").show
+---+
|  a|
+---+
+---+

scala> spark.table("t1").filter($"p" === "'\"").select($"a").show
java.lang.RuntimeException: Caught Hive MetaException attempting to get partition metadata by filter from ...

AFTER

scala> spark.table("t1").filter($"p" === "p1\" and q=\"q1").select($"a").show
+---+
|  a|
+---+
|  2|
+---+

scala> spark.table("t1").filter($"p" === "'\"").select($"a").show
java.lang.UnsupportedOperationException: Partition filter cannot have both `"` and `'` characters

How was this patch tested?

Pass the Jenkins test with new test cases.

…re partition pruning

dongjoon-hyun · 2017-03-12T10:19:41Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveShim.scala

+      s"""'$str'"""
+    } else {
+      throw new UnsupportedOperationException(
+        """Partition filter cannot have both `"` and `'` characters""")


The current master also raise exception with this mixed case.

scala> spark.table("t1").filter($"p" === "'\"").select($"a").show java.lang.RuntimeException: Caught Hive MetaException attempting to get partition metadata by filter from ... ... Caused by: java.lang.reflect.InvocationTargetException: org.apache.hadoop.hive.metastore.api.MetaException: Error parsing partition filter : line 1:8 mismatched character '<EOF>' expecting '"'

table.filter($"p" === """a"'b""").select($"a")

Does that return the correct result?

From the current master branch, Hive does not accept that.

scala> Seq((1, "a\"'b")).toDF("a", "p").write.partitionBy("p").saveAsTable("t1") scala> spark.table("t1").show() +---+----+ | a| p| +---+----+ | 1|a"'b| +---+----+ scala> spark.table("t1").filter($"p" === """a"'b""").select($"a").show java.lang.RuntimeException:

We got the exception from Hive, because they are not escaped.

SparkQA · 2017-03-12T11:47:18Z

Test build #74400 has finished for PR 17266 at commit 37bff5d.

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

gatorsmile · 2017-03-12T23:27:01Z

We should add the escape, instead of adding quotes. Right?

gatorsmile · 2017-03-12T23:32:25Z

Based on Hive' doc (https://cwiki.apache.org/confluence/display/Hive/LanguageManual+Types):

String literals can be expressed with either single quotes (') or double quotes ("). Hive uses C-style escaping within the strings.

dongjoon-hyun · 2017-03-12T23:38:28Z

Yep. I tried escaping first, but it doesn't work inside Hive side. I mean for the mixed cases.
Also, I tried concatenation style '"'"'" for "' because it works in Hive. But, for filter parsing, it fails at Hive Filter Lexer layer.

dongjoon-hyun · 2017-03-12T23:49:17Z

BTW, I forgot to thank you. :) Thank you for review.
For the non-mixed cases, I think we don't need to escape.

gatorsmile · 2017-03-13T03:01:10Z

Have you tried C-style escaping? "' -> \"\'

dongjoon-hyun · 2017-03-13T04:20:17Z

Yes. What I meant was that is not supported correctly from Hive (as described in its documentation).
For the mixed case, all combinations of the escaping does not work as indented.

You can test by changing here with the escaped string.

gatorsmile · 2017-03-13T04:23:23Z

Could you try Hive to double check it? Is this a bug in Hive?

dongjoon-hyun · 2017-03-13T04:23:48Z

It's not bug of Hive CLI, it seems a limitation of that API, getPartitionsByFilterMethod.

dongjoon-hyun · 2017-03-13T04:25:10Z

If you want, I will make some other failure test cases in this PR to make it sure for you and the others.
Maybe, about getPartitionsByFilterMethod.

gatorsmile · 2017-03-13T04:27:00Z

Ok, how about submitting a separate PR by escaping the string? You can show the reviewers the failure cases there.

dongjoon-hyun · 2017-03-13T04:27:44Z

Yep. I see. Thank for the guide, @gatorsmile !

dongjoon-hyun · 2017-03-13T06:30:28Z

#17271 failed as expected. Hive API does not handle the filters with escaped string, e.g. two escaped chars like \"\".

gatorsmile · 2017-03-13T18:30:54Z

Could you do more investigation about the impact of the following two Hive JIRAs?

https://issues.apache.org/jira/browse/HIVE-11723
https://issues.apache.org/jira/browse/HIVE-2943

Thank you!

dongjoon-hyun · 2017-03-13T18:40:09Z

Sure!

dongjoon-hyun · 2017-03-13T19:05:20Z

For HIVE-11723, it resolved it in SemanticAnalyzer.

I think it's possible to bring that into our @JoshRosen's repo.

Let's me backport that to see if this is enough.

cloud-fan · 2017-03-15T01:23:53Z

can we say something more in the error message? We should explain that it's a hive bug and put the hive jira in it

dongjoon-hyun · 2017-03-15T02:28:07Z

The following is the error message. Since we are not escaping in the spark master, the behavior (incorrect filtering or the error message) is the same from the master branch Spark.

java.lang.RuntimeException: Caught Hive MetaException attempting to get partition metadata by filter from Hive. You can set the Spark configuration setting spark.sql.hive.manageFilesourcePartitions to false to work around this problem, however this will result in degraded performance. Please report a bug: https://issues.apache.org/jira/browse/SPARK
  at org.apache.spark.sql.hive.client.Shim_v0_13.getPartitionsByFilter(HiveShim.scala:612)
...
Caused by: java.lang.reflect.InvocationTargetException: org.apache.hadoop.hive.metastore.api.MetaException: Error parsing partition filter : line 1:8 mismatched character '<EOF>' expecting '"'
  at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
...
  at org.apache.spark.sql.hive.client.Shim_v0_13.getPartitionsByFilter(HiveShim.scala:599)
  ... 103 more
Caused by: org.apache.hadoop.hive.metastore.api.MetaException: Error parsing partition filter : line 1:8 mismatched character '<EOF>' expecting '"'
  at org.apache.hadoop.hive.metastore.ObjectStore.getFilterParser(ObjectStore.java:2569)
  at org.apache.hadoop.hive.metastore.ObjectStore.getPartitionsByFilterInternal(ObjectStore.java:2512)
  at org.apache.hadoop.hive.metastore.ObjectStore.getPartitionsByFilter(ObjectStore.java:2335)
...
org.apache.hadoop.hive.metastore.HiveMetaStore$HMSHandler.get_partitions_by_filter(HiveMetaStore.java:4442)
...
org.apache.hadoop.hive.metastore.HiveMetaStoreClient.listPartitionsByFilter(HiveMetaStoreClient.java:1103)
...
  at org.apache.hadoop.hive.ql.metadata.Hive.getPartitionsByFilter(Hive.java:2254)
  ... 108 more

HIVE-11723 seems to resove that in SemanticAnalyzer. So, I need to try that soon.

dongjoon-hyun · 2017-03-15T02:38:01Z

For non-error message cases, incorrect result is also a problem in this issue.

…re partition pruning ## What changes were proposed in this pull request? Since current `HiveShim`'s `convertFilters` does not escape the string literals. There exists the following correctness issues. This PR aims to return the correct result and also shows the more clear exception message. **BEFORE** ```scala scala> Seq((1, "p1", "q1"), (2, "p1\" and q=\"q1", "q2")).toDF("a", "p", "q").write.partitionBy("p", "q").saveAsTable("t1") scala> spark.table("t1").filter($"p" === "p1\" and q=\"q1").select($"a").show +---+ | a| +---+ +---+ scala> spark.table("t1").filter($"p" === "'\"").select($"a").show java.lang.RuntimeException: Caught Hive MetaException attempting to get partition metadata by filter from ... ``` **AFTER** ```scala scala> spark.table("t1").filter($"p" === "p1\" and q=\"q1").select($"a").show +---+ | a| +---+ | 2| +---+ scala> spark.table("t1").filter($"p" === "'\"").select($"a").show java.lang.UnsupportedOperationException: Partition filter cannot have both `"` and `'` characters ``` ## How was this patch tested? Pass the Jenkins test with new test cases. Author: Dongjoon Hyun <dongjoon@apache.org> Closes #17266 from dongjoon-hyun/SPARK-19912. (cherry picked from commit 21e366a) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

cloud-fan · 2017-03-21T04:18:03Z

thanks, merging to master/2.1!

dongjoon-hyun · 2017-03-21T15:24:15Z

Oh, thank you, @cloud-fan !

[SPARK-19912][SQL] String literals should be escaped for Hive metasto…

37bff5d

…re partition pruning

dongjoon-hyun commented Mar 12, 2017

View reviewed changes

dongjoon-hyun mentioned this pull request Mar 13, 2017

[SPARK-19912][SQL][NOT FOR MERGE] String literals should be escaped for Hive metastore partition pruning #17271

Closed

asfgit closed this in 21e366a Mar 21, 2017

dongjoon-hyun deleted the SPARK-19912 branch January 7, 2019 07:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-19912][SQL] String literals should be escaped for Hive metastore partition pruning #17266

[SPARK-19912][SQL] String literals should be escaped for Hive metastore partition pruning #17266

dongjoon-hyun commented Mar 12, 2017 •

edited

Loading

dongjoon-hyun Mar 12, 2017

gatorsmile Mar 12, 2017

dongjoon-hyun Mar 12, 2017

dongjoon-hyun Mar 12, 2017 •

edited

Loading

gatorsmile Mar 13, 2017

SparkQA commented Mar 12, 2017

gatorsmile commented Mar 12, 2017

gatorsmile commented Mar 12, 2017

dongjoon-hyun commented Mar 12, 2017 •

edited

Loading

dongjoon-hyun commented Mar 12, 2017

gatorsmile commented Mar 13, 2017

dongjoon-hyun commented Mar 13, 2017 •

edited

Loading

gatorsmile commented Mar 13, 2017

dongjoon-hyun commented Mar 13, 2017 •

edited

Loading

dongjoon-hyun commented Mar 13, 2017 •

edited

Loading

gatorsmile commented Mar 13, 2017

dongjoon-hyun commented Mar 13, 2017

dongjoon-hyun commented Mar 13, 2017

gatorsmile commented Mar 13, 2017

dongjoon-hyun commented Mar 13, 2017

dongjoon-hyun commented Mar 13, 2017 •

edited

Loading

cloud-fan commented Mar 15, 2017 •

edited

Loading

dongjoon-hyun commented Mar 15, 2017

dongjoon-hyun commented Mar 15, 2017

cloud-fan commented Mar 21, 2017

dongjoon-hyun commented Mar 21, 2017

[SPARK-19912][SQL] String literals should be escaped for Hive metastore partition pruning #17266

[SPARK-19912][SQL] String literals should be escaped for Hive metastore partition pruning #17266

Conversation

dongjoon-hyun commented Mar 12, 2017 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

dongjoon-hyun Mar 12, 2017

Choose a reason for hiding this comment

gatorsmile Mar 12, 2017

Choose a reason for hiding this comment

dongjoon-hyun Mar 12, 2017

Choose a reason for hiding this comment

dongjoon-hyun Mar 12, 2017 • edited Loading

Choose a reason for hiding this comment

gatorsmile Mar 13, 2017

Choose a reason for hiding this comment

SparkQA commented Mar 12, 2017

gatorsmile commented Mar 12, 2017

gatorsmile commented Mar 12, 2017

dongjoon-hyun commented Mar 12, 2017 • edited Loading

dongjoon-hyun commented Mar 12, 2017

gatorsmile commented Mar 13, 2017

dongjoon-hyun commented Mar 13, 2017 • edited Loading

gatorsmile commented Mar 13, 2017

dongjoon-hyun commented Mar 13, 2017 • edited Loading

dongjoon-hyun commented Mar 13, 2017 • edited Loading

gatorsmile commented Mar 13, 2017

dongjoon-hyun commented Mar 13, 2017

dongjoon-hyun commented Mar 13, 2017

gatorsmile commented Mar 13, 2017

dongjoon-hyun commented Mar 13, 2017

dongjoon-hyun commented Mar 13, 2017 • edited Loading

cloud-fan commented Mar 15, 2017 • edited Loading

dongjoon-hyun commented Mar 15, 2017

dongjoon-hyun commented Mar 15, 2017

cloud-fan commented Mar 21, 2017

dongjoon-hyun commented Mar 21, 2017

dongjoon-hyun commented Mar 12, 2017 •

edited

Loading

dongjoon-hyun Mar 12, 2017 •

edited

Loading

dongjoon-hyun commented Mar 12, 2017 •

edited

Loading

dongjoon-hyun commented Mar 13, 2017 •

edited

Loading

dongjoon-hyun commented Mar 13, 2017 •

edited

Loading

dongjoon-hyun commented Mar 13, 2017 •

edited

Loading

dongjoon-hyun commented Mar 13, 2017 •

edited

Loading

cloud-fan commented Mar 15, 2017 •

edited

Loading