eric-maynard commented on code in PR #2798:
URL: https://github.com/apache/polaris/pull/2798#discussion_r2521603755


##########
client/python/cli/command/catalogs.py:
##########
@@ -268,6 +273,14 @@ def _build_connection_config_info(self):
                 service_identity=service_identity,
                 remote_catalog_name=self.iceberg_remote_catalog_name
             )
+        elif self.catalog_connection_type == CatalogConnectionType.HIVE.value:
+            config = HiveConnectionConfigInfo(
+                connection_type=self.catalog_connection_type.upper(),
+                uri=self.catalog_uri,
+                authentication_parameters=auth_params,
+                service_identity=service_identity,
+                warehouse=self.hadoop_warehouse

Review Comment:
   The tentative convention I had in mind was to prefix each argument with the 
federation type it's specific to -- namely, `ICEBERG_REMOTE_CATALOG_NAME` is 
specific to `iceberg` federation type, and `HADOOP_WAREHOUSE` is specific to 
`HADOOP`. I thought it might be unclear what just `REMOTE_CATALOG_NAME` or 
`HADOOP_WAREHOUSE` meant.



##########
client/python/cli/command/catalogs.py:
##########
@@ -268,6 +273,14 @@ def _build_connection_config_info(self):
                 service_identity=service_identity,
                 remote_catalog_name=self.iceberg_remote_catalog_name
             )
+        elif self.catalog_connection_type == CatalogConnectionType.HIVE.value:
+            config = HiveConnectionConfigInfo(
+                connection_type=self.catalog_connection_type.upper(),
+                uri=self.catalog_uri,
+                authentication_parameters=auth_params,
+                service_identity=service_identity,
+                warehouse=self.hadoop_warehouse

Review Comment:
   The tentative convention I had in mind was to prefix each argument with the 
federation type it's specific to -- namely, `ICEBERG_REMOTE_CATALOG_NAME` is 
specific to `iceberg` federation type, and `HADOOP_WAREHOUSE` is specific to 
`HADOOP`. I thought it might be unclear what just `REMOTE_CATALOG_NAME` or 
`WAREHOUSE` meant.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to