[GitHub] [spark] kevincmchen commented on a change in pull request #33746: [SPARK-36514][SQL] Support to set the meta conf in HiveMetastoreClient
kevincmchen commented on a change in pull request #33746: URL: https://github.com/apache/spark/pull/33746#discussion_r689373076 ## File path: sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala ## @@ -265,6 +265,10 @@ private[hive] class HiveClientImpl( clientLoader.cachedHive.asInstanceOf[Hive] } else { val c = getHive(conf) + val metaConfPrefix = SQLConf.get.getConf(HiveUtils.HIVE_METASTORE_METACONF_PREFIX) + conf.getAllProperties.asScala +.filterKeys(_.startsWith(metaConfPrefix)) +.foreach{ case(key, value) => c.setMetaConf(key.substring(metaConfPrefix.length), value)} Review comment: i have add the description, pls check -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] kevincmchen commented on a change in pull request #33746: [SPARK-36514][SQL] Support to set the meta conf in HiveMetastoreClient
kevincmchen commented on a change in pull request #33746: URL: https://github.com/apache/spark/pull/33746#discussion_r689373891 ## File path: sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala ## @@ -265,6 +265,10 @@ private[hive] class HiveClientImpl( clientLoader.cachedHive.asInstanceOf[Hive] } else { val c = getHive(conf) + val metaConfPrefix = SQLConf.get.getConf(HiveUtils.HIVE_METASTORE_METACONF_PREFIX) + conf.getAllProperties.asScala +.filterKeys(_.startsWith(metaConfPrefix)) +.foreach{ case(key, value) => c.setMetaConf(key.substring(metaConfPrefix.length), value)} Review comment: in our case, we pass tenant info to an external multi-tenant metasotre by utilizing the meta conf, such as tenant name, auth info etc. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] kevincmchen commented on a change in pull request #33746: [SPARK-36514][SQL] Support to set the meta conf in HiveMetastoreClient
kevincmchen commented on a change in pull request #33746: URL: https://github.com/apache/spark/pull/33746#discussion_r689380411 ## File path: sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveUtils.scala ## @@ -118,6 +118,13 @@ private[spark] object HiveUtils extends Logging { .toSequence .createWithDefault(Nil) + val HIVE_METASTORE_METACONF_PREFIX = buildStaticConf("spark.sql.hive.metastore.metaconf.prefix") +.doc("the prefix of the meta conf in HiveMetastoreClient, which is used to extract " + Review comment: I want to update the description to "prefix of the meta conf that could be used to set the HiveMetastoreClient.", do you think it's better? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] kevincmchen commented on a change in pull request #33746: [SPARK-36514][SQL] Support to set the meta conf in HiveMetastoreClient
kevincmchen commented on a change in pull request #33746: URL: https://github.com/apache/spark/pull/33746#discussion_r689373076 ## File path: sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala ## @@ -265,6 +265,10 @@ private[hive] class HiveClientImpl( clientLoader.cachedHive.asInstanceOf[Hive] } else { val c = getHive(conf) + val metaConfPrefix = SQLConf.get.getConf(HiveUtils.HIVE_METASTORE_METACONF_PREFIX) + conf.getAllProperties.asScala +.filterKeys(_.startsWith(metaConfPrefix)) +.foreach{ case(key, value) => c.setMetaConf(key.substring(metaConfPrefix.length), value)} Review comment: i have added the description, pls check -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] kevincmchen commented on a change in pull request #33746: [SPARK-36514][SQL] Support to set the meta conf in HiveMetastoreClient
kevincmchen commented on a change in pull request #33746: URL: https://github.com/apache/spark/pull/33746#discussion_r689380411 ## File path: sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveUtils.scala ## @@ -118,6 +118,13 @@ private[spark] object HiveUtils extends Logging { .toSequence .createWithDefault(Nil) + val HIVE_METASTORE_METACONF_PREFIX = buildStaticConf("spark.sql.hive.metastore.metaconf.prefix") +.doc("the prefix of the meta conf in HiveMetastoreClient, which is used to extract " + Review comment: I want to update the description to "prefix of the meta conf that could be used to set the HiveMetastoreClient.", do you think it's better? @dongjoon-hyun -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] kevincmchen commented on a change in pull request #33746: [SPARK-36514][SQL] Support to set the meta conf in HiveMetastoreClient
kevincmchen commented on a change in pull request #33746: URL: https://github.com/apache/spark/pull/33746#discussion_r689373891 ## File path: sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala ## @@ -265,6 +265,10 @@ private[hive] class HiveClientImpl( clientLoader.cachedHive.asInstanceOf[Hive] } else { val c = getHive(conf) + val metaConfPrefix = SQLConf.get.getConf(HiveUtils.HIVE_METASTORE_METACONF_PREFIX) + conf.getAllProperties.asScala +.filterKeys(_.startsWith(metaConfPrefix)) +.foreach{ case(key, value) => c.setMetaConf(key.substring(metaConfPrefix.length), value)} Review comment: in our case, we pass tenant info to an external multi-tenant metasotre by utilizing the meta conf, such as tenant name, auth info etc. @dongjoon-hyun -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] kevincmchen commented on a change in pull request #33746: [SPARK-36514][SQL] Support to set the meta conf in HiveMetastoreClient
kevincmchen commented on a change in pull request #33746: URL: https://github.com/apache/spark/pull/33746#discussion_r689380411 ## File path: sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveUtils.scala ## @@ -118,6 +118,13 @@ private[spark] object HiveUtils extends Logging { .toSequence .createWithDefault(Nil) + val HIVE_METASTORE_METACONF_PREFIX = buildStaticConf("spark.sql.hive.metastore.metaconf.prefix") +.doc("the prefix of the meta conf in HiveMetastoreClient, which is used to extract " + Review comment: I want to update the description to "prefix of the meta conf that could be used to set the HiveMetastoreClient.", do you think it's better? @dongjoon-hyun -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] kevincmchen commented on a change in pull request #33746: [SPARK-36514][SQL] Support to set the meta conf in HiveMetastoreClient
kevincmchen commented on a change in pull request #33746: URL: https://github.com/apache/spark/pull/33746#discussion_r689492375 ## File path: sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveUtils.scala ## @@ -118,6 +118,13 @@ private[spark] object HiveUtils extends Logging { .toSequence .createWithDefault(Nil) + val HIVE_METASTORE_METACONF_PREFIX = buildStaticConf("spark.sql.hive.metastore.metaconf.prefix") +.doc("the prefix of the meta conf in HiveMetastoreClient, which is used to extract " + Review comment: > This sounds misleading because this PR calls `c.setMetaConf` instead of extracting. Can we use more direct terms to describe what this conf aims? cause we couldn't get the meta conf directly and the prefix is used to extract meta conf from HiveConf. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] kevincmchen commented on a change in pull request #33746: [SPARK-36514][SQL] Support to set the meta conf in HiveMetastoreClient
kevincmchen commented on a change in pull request #33746: URL: https://github.com/apache/spark/pull/33746#discussion_r689492375 ## File path: sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveUtils.scala ## @@ -118,6 +118,13 @@ private[spark] object HiveUtils extends Logging { .toSequence .createWithDefault(Nil) + val HIVE_METASTORE_METACONF_PREFIX = buildStaticConf("spark.sql.hive.metastore.metaconf.prefix") +.doc("the prefix of the meta conf in HiveMetastoreClient, which is used to extract " + Review comment: > This sounds misleading because this PR calls `c.setMetaConf` instead of extracting. Can we use more direct terms to describe what this conf aims? 1. the prefix is used to extract meta conf from HiveConf, cause we couldn't get the meta conf directly. 2. we should get the meta conf before set it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] kevincmchen commented on a change in pull request #33746: [SPARK-36514][SQL] Support to set the meta conf in HiveMetastoreClient
kevincmchen commented on a change in pull request #33746: URL: https://github.com/apache/spark/pull/33746#discussion_r689524588 ## File path: sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveUtils.scala ## @@ -118,6 +118,13 @@ private[spark] object HiveUtils extends Logging { .toSequence .createWithDefault(Nil) + val HIVE_METASTORE_METACONF_PREFIX = buildStaticConf("spark.sql.hive.metastore.metaconf.prefix") +.doc("the prefix of the meta conf in HiveMetastoreClient, which is used to extract " + Review comment: I will add detailed description for the configuration -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] kevincmchen commented on a change in pull request #33746: [SPARK-36514][SQL] Support to set the meta conf in HiveMetastoreClient
kevincmchen commented on a change in pull request #33746: URL: https://github.com/apache/spark/pull/33746#discussion_r690018846 ## File path: sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveUtils.scala ## @@ -118,6 +118,13 @@ private[spark] object HiveUtils extends Logging { .toSequence .createWithDefault(Nil) + val HIVE_METASTORE_METACONF_PREFIX = buildStaticConf("spark.sql.hive.metastore.metaconf.prefix") +.doc("the prefix of the meta conf in HiveMetastoreClient, which is used to extract " + Review comment: ok, i add a example about how to use the additional conf in this PR. pls check -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] kevincmchen commented on a change in pull request #33746: [SPARK-36514][SQL] Support to set the meta conf in HiveMetastoreClient
kevincmchen commented on a change in pull request #33746: URL: https://github.com/apache/spark/pull/33746#discussion_r690018846 ## File path: sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveUtils.scala ## @@ -118,6 +118,13 @@ private[spark] object HiveUtils extends Logging { .toSequence .createWithDefault(Nil) + val HIVE_METASTORE_METACONF_PREFIX = buildStaticConf("spark.sql.hive.metastore.metaconf.prefix") +.doc("the prefix of the meta conf in HiveMetastoreClient, which is used to extract " + Review comment: ok, i add a example about how to use the additional conf in this PR. pls check @HyukjinKwon -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] kevincmchen commented on a change in pull request #33746: [SPARK-36514][SQL] Support to set the meta conf in HiveMetastoreClient
kevincmchen commented on a change in pull request #33746: URL: https://github.com/apache/spark/pull/33746#discussion_r690018846 ## File path: sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveUtils.scala ## @@ -118,6 +118,13 @@ private[spark] object HiveUtils extends Logging { .toSequence .createWithDefault(Nil) + val HIVE_METASTORE_METACONF_PREFIX = buildStaticConf("spark.sql.hive.metastore.metaconf.prefix") +.doc("the prefix of the meta conf in HiveMetastoreClient, which is used to extract " + Review comment: ok, i have added a example about how to use the additional conf in this PR. pls check @HyukjinKwon -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] kevincmchen commented on a change in pull request #33746: [SPARK-36514][SQL] Support to set the meta conf in HiveMetastoreClient
kevincmchen commented on a change in pull request #33746: URL: https://github.com/apache/spark/pull/33746#discussion_r690018846 ## File path: sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveUtils.scala ## @@ -118,6 +118,13 @@ private[spark] object HiveUtils extends Logging { .toSequence .createWithDefault(Nil) + val HIVE_METASTORE_METACONF_PREFIX = buildStaticConf("spark.sql.hive.metastore.metaconf.prefix") +.doc("the prefix of the meta conf in HiveMetastoreClient, which is used to extract " + Review comment: ok, i have added an example about how to use the additional conf in this PR. pls check @HyukjinKwon -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] kevincmchen commented on a change in pull request #33746: [SPARK-36514][SQL] Support to set the meta conf in HiveMetastoreClient
kevincmchen commented on a change in pull request #33746: URL: https://github.com/apache/spark/pull/33746#discussion_r690018846 ## File path: sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveUtils.scala ## @@ -118,6 +118,13 @@ private[spark] object HiveUtils extends Logging { .toSequence .createWithDefault(Nil) + val HIVE_METASTORE_METACONF_PREFIX = buildStaticConf("spark.sql.hive.metastore.metaconf.prefix") +.doc("the prefix of the meta conf in HiveMetastoreClient, which is used to extract " + Review comment: ok, i have added a example about how to use the additional conf in this PR. pls check @HyukjinKwon ## File path: sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveUtils.scala ## @@ -118,6 +118,13 @@ private[spark] object HiveUtils extends Logging { .toSequence .createWithDefault(Nil) + val HIVE_METASTORE_METACONF_PREFIX = buildStaticConf("spark.sql.hive.metastore.metaconf.prefix") +.doc("the prefix of the meta conf in HiveMetastoreClient, which is used to extract " + Review comment: ok, i have added an example about how to use the additional conf in this PR. pls check @HyukjinKwon -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org