-
Notifications
You must be signed in to change notification settings - Fork 360
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
Task: Support Azure Unity Catalog export #7554
Conversation
♻️ PR Preview 156c05b has been successfully destroyed since this PR has been closed. 🤖 By surge-preview |
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.
Can you update the export_delta_log
method with the new parameter?
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.
Very cool!
Thanks for the quick fix
(just the docs thingy)
esti/catalog_export_test.go
Outdated
AzureStorageAccount: viper.GetString("azure_storage_account"), | ||
AzureAccessKey: viper.GetString("azure_storage_access_key"), | ||
} | ||
//blockstore := setupCatalogExportTestByStorageType(t, testData) |
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.
delete?
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 good, request only around docs.
I wonder about friction - not sure so it's more of a question:
Unity users will have to use this path transformer, maybe we can make it somehow default?
For example let it be the default in delta_exporter (maybe its a bad idea).
:)
I don't think making it the default behavior for delta export is a good idea. For delta export use cases this might even be a point of friction for users who have set up a Azure Blob Storage (and not an ADLS gen2 storage account) |
docs/howto/hooks/lua.md
Outdated
@@ -739,6 +745,10 @@ The registration will use the following paths to register the table: | |||
`<catalog>.<branch name>.<table_name>` where the branch name will be used as the schema name. | |||
The return value is a table with mapping of table names to registration request status. | |||
|
|||
**Note: (Azure users)** Databricks catalog external locations is supported only for ADLS Gen2 storage accounts. | |||
When exporting Delta tables using the `lakefs/catalogexport/delta_exporter.export_delta_log` function, the `path_transformer` must be | |||
used to convert the paths scheme to `abfss`. the built-in `azure` lua library provides this functionality in `transformPathToAbfss`. |
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.
used to convert the paths scheme to `abfss`. the built-in `azure` lua library provides this functionality in `transformPathToAbfss`. | |
used to convert the paths scheme to `abfss`. The built-in `azure` lua library provides this functionality in `transformPathToAbfss`. |
Just had to...
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.
Awesome (one typo change request that doesn't block)
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.
LGTM!
Thanks - waiting to verify the fix completely before merging |
Closes #7551
Closes #7553
Change Description
Add support for Unity catalog exporter in Azure
Fix delta exporter to support abfss scheme for delta log path and table physical address
Testing Details
Added esti test case
Breaking Change?
No
After fix this is the output of delta_exporter: