-
Notifications
You must be signed in to change notification settings - Fork 334
CLI: Add Hive federation option #2798
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,7 +29,7 @@ | |
| StorageConfigInfo, ExternalCatalog, AwsStorageConfigInfo, AzureStorageConfigInfo, GcpStorageConfigInfo, \ | ||
| PolarisCatalog, CatalogProperties, BearerAuthenticationParameters, ImplicitAuthenticationParameters, \ | ||
| OAuthClientCredentialsParameters, SigV4AuthenticationParameters, HadoopConnectionConfigInfo, \ | ||
| IcebergRestConnectionConfigInfo, AwsIamServiceIdentityInfo | ||
| IcebergRestConnectionConfigInfo, HiveConnectionConfigInfo, AwsIamServiceIdentityInfo | ||
|
|
||
|
|
||
| @dataclass | ||
|
|
@@ -114,6 +114,11 @@ def validate(self): | |
| if self.catalog_connection_type == CatalogConnectionType.HADOOP.value: | ||
| if not self.hadoop_warehouse or not self.catalog_uri: | ||
| raise Exception(f"Missing required argument for connection type 'HADOOP':" | ||
| f" {Argument.to_flag_name(Arguments.HADOOP_WAREHOUSE)}" | ||
| f" and {Argument.to_flag_name(Arguments.CATALOG_URI)}") | ||
| elif self.catalog_connection_type == CatalogConnectionType.HIVE.value: | ||
| if not self.hadoop_warehouse or not self.catalog_uri: | ||
| raise Exception(f"Missing required argument for connection type 'HIVE':" | ||
| f" {Argument.to_flag_name(Arguments.HADOOP_WAREHOUSE)}" | ||
| f" and {Argument.to_flag_name(Arguments.CATALOG_URI)}") | ||
| if self.catalog_service_identity_type == ServiceIdentityType.AWS_IAM.value: | ||
|
|
@@ -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 | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's weird to use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thoughts, suggestions? @HonahX @eric-maynard @MonkeyCanCode
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sry for the late response. I was away for couple of days. I don't have a strong preference over this naming. Maybe ask @eric-maynard as he initially added it in eb6b6ad.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The tentative convention I had in mind was to prefix each argument with the federation type it's specific to -- namely,
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. to be clear I'm supportive of re-using these flags across federations types (e.g. across Hive / Hadoop) and indeed I think if we ever flesh out federation in the way that I envisioned at this time we would need to. There would be a way to federate to a Hive catalog both for Iceberg and non-Iceberg tables, and these would surely share arguments. My only hesitation would be that the CLI 's method of handling arguments is a bit brittle and if we re-use them we should just make sure the parsing and the |
||
| ) | ||
| elif self.catalog_connection_type is not None: | ||
| raise Exception("Unknown catalog connection type:", self.catalog_connection_type) | ||
| return config | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
--helpwas updated to listiceberg-rest, hadoop, hiveso these logs (and the tests) should follow that same convention