-
Notifications
You must be signed in to change notification settings - Fork 313
change catalog default warehouse location to not use hive-style warehouse location #2059
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
change catalog default warehouse location to not use hive-style warehouse location #2059
Conversation
could do something fancy with |
217d912
to
f393c38
Compare
.db
suffix in warehouse location.db
suffix in warehouse location for dynamo/hive/glue catalogs
@jayceslesar made it so aligns with the java implementation |
@@ -652,6 +654,19 @@ def _convert_dynamo_table_item_to_iceberg_table(self, dynamo_table_item: Dict[st | |||
catalog=self, | |||
) | |||
|
|||
def _get_default_warehouse_location(self, database_name: str, table_name: str) -> str: |
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.
Looks like we're repeating this a couple of times, should we move this into pyiceberg/catalog/__init__.py
?
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.
good call, done
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.
Generally, I think this is a good change since we don't want to leak Hive legacy until eternity :)
.db
suffix in warehouse location for dynamo/hive/glue catalogs
Closes #2052
Rationale for this change
Aligns catalog behavior with the java reference implementation.
HiveCatalog, DynamoDbCatalog, and GlueCatalog all use
.db
suffix in warehouse locationJdbcCatalog, HadoopCatalog, and InMemoryCatalog do not use
.db
suffix in warehouse locationAre these changes tested?
Yes tests for sql catalog are modified to remove
.db
Are there any user-facing changes?