[GitHub] spark pull request #22614: [SPARK-25561][SQL] Implement a new config to cont...

[mallman <gi...@git.apache.org>]

Github user mallman commented on a diff in the pull request:

    https://github.com/apache/spark/pull/22614#discussion_r223415835
  
    --- Diff: sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveShim.scala ---
    @@ -754,26 +755,38 @@ private[client] class Shim_v0_13 extends Shim_v0_12 {
               tryDirectSqlConfVar.defaultBoolVal.toString).toBoolean
             try {
               // Hive may throw an exception when calling this method in some circumstances, such as
    -          // when filtering on a non-string partition column when the hive config key
    -          // hive.metastore.try.direct.sql is false
    +          // when filtering on a non-string partition column.
    --- End diff --
    
    To me your revised comment suggests that if we try to filter on a non-string partition column we can expect the call to `getPartitionedByFilter` to fail. That's not true.
    
    Although I wrote the original comment, I can see how making an assumption about Hive's behavior when calling this method is rather specious and unwise. I suggest we just remove this comment entirely. To simply state that
    
    ```
    Hive may throw an exception when calling this method in some circumstances.
    ```
    
    only states the obvious—of course any method call may throw an exception in some circumstances.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org