-
Notifications
You must be signed in to change notification settings - Fork 367
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
Delta Lake +Unity Catalog exporter: Delta Lake table metadata #7527
Delta Lake +Unity Catalog exporter: Delta Lake table metadata #7527
Conversation
♻️ PR Preview 5e9af10 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.
Overall looks very good!
I have some comments about the documentation.
Also - can we please add an esti test?
pkg/actions/lua/databricks/client.go
Outdated
luautil "github.com/treeverse/lakefs/pkg/actions/lua/util" | ||
|
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.
luautil "github.com/treeverse/lakefs/pkg/actions/lua/util" | |
luautil "github.com/treeverse/lakefs/pkg/actions/lua/util" |
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.
don't mess with goimports
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.
Don't mess with it, just remove the redundant newline 📏
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.
sure thing sure thing
pkg/actions/lua/formats/delta.go
Outdated
"github.com/csimplestring/delta-go/action" | ||
|
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.
"github.com/csimplestring/delta-go/action" | |
"github.com/csimplestring/delta-go/action" |
docs/howto/hooks/lua.md
Outdated
- the first is a table of the format `{number, {string}}` where `number` is a version in the Delta Log, and the mapped `{string}` | ||
table (list) contains JSON strings of the different Delta Lake log operations listed in the mapped version entry. |
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.
Sentences are not very clear. table? list?JSON strings?? I'm not sure what I'm getting here
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.
I'll clarify
docs/howto/hooks/lua.md
Outdated
The format of the response is two tables: | ||
- the first is a table of the format `{number, {string}}` where `number` is a version in the Delta Log, and the mapped `{string}` | ||
table (list) contains JSON strings of the different Delta Lake log operations listed in the mapped version entry. | ||
- the second is a table of the metadata of the table, it consists of the following fields: |
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.
Isn't the metadata versioned as well?
I find it confusing that on the one hand we provide a list of versions for the table and on the other hand we provide only the metadata of the current snapshot. Perhaps a clarification is needed
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.
It's the latest version's metadata.
The intention is to make it easier to get the current state of the table to initialize it in other systems faster.
I'll clarify
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.
Thanks for adding the tests!
Closes #7302
Change Description
FYI @talSofer