Skip to content
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

Unity table registration docs: table metadata #7531

Merged
merged 1 commit into from
Mar 7, 2024

Conversation

Jonathan-Rosenberg
Copy link
Contributor

No description provided.

@Jonathan-Rosenberg Jonathan-Rosenberg added exclude-changelog PR description should not be included in next release changelog minor-change Used for PRs that don't require issue attached labels Mar 6, 2024
@Jonathan-Rosenberg Jonathan-Rosenberg requested a review from N-o-Z March 6, 2024 12:45
Copy link

github-actions bot commented Mar 6, 2024

😭 Deploy PR Preview a274046 failed. Build logs

🤖 By surge-preview

Copy link

github-actions bot commented Mar 6, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

10 passed

@@ -741,7 +743,7 @@ Parameters:

- `action(table)`: The global action table
- `table_descriptors_path(string)`: The path under which the table descriptors of the provided `table_paths` reside.
- `delta_table_paths(table)`: Table names to physical paths mapping (e.g. `{ table1 = "s3://mybucket/mytable1", table2 = "s3://mybucket/mytable2" }`)
- `delta_table_details(table)`: Table names to physical paths mapping and table metadata (e.g. `{table1 = {path = "s3://mybucket/mytable1", metadata = {id = "table_1_id", name = "table1", ...}}, table2 = {path = "s3://mybucket/mytable2", metadata = {id = "table_2_id", name = "table2", ...}}}`.)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope I don't understand this: right now it looks like we broke backwards compatibility of Lua scripts earlier, and now we're documenting it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You understand correctly.
The change was made at #7527.
What would you suggest on doing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a minor version bump, I would expect the older form (string that is a path) to remain valid, maybe deprecated, and the new form also to be allowed. Once that happened documentation would pretty much solve itself, something like:

Suggested change
- `delta_table_details(table)`: Table names to physical paths mapping and table metadata (e.g. `{table1 = {path = "s3://mybucket/mytable1", metadata = {id = "table_1_id", name = "table1", ...}}, table2 = {path = "s3://mybucket/mytable2", metadata = {id = "table_2_id", name = "table2", ...}}}`.)
- `delta_table_details(table)`: Map of table names to physical paths as a string, or to physical paths and table metadata as a JSON object (e.g. `{table1 = {path = "s3://mybucket/mytable1", metadata = {id = "table_1_id", name = "table1", ...}}, table2 = "s3://mybucket/mytable2"}}`.)

It might be water under the bridge, particularly if this feature is new enough and has not had many users.

Copy link
Contributor Author

@Jonathan-Rosenberg Jonathan-Rosenberg Mar 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be water under the bridge, particularly if this feature is new enough and has not had many users.

I'm very positive that this is the case. If anything, the usage of this script is mainly as part of the Unity Catalog registration flow, and it won't be necessary to change the existing code (not breaking the flow of using the output of the delta exporter)

@Jonathan-Rosenberg Jonathan-Rosenberg added include-changelog PR description should be included in next release changelog exclude-changelog PR description should not be included in next release changelog and removed exclude-changelog PR description should not be included in next release changelog include-changelog PR description should be included in next release changelog labels Mar 6, 2024
@Jonathan-Rosenberg Jonathan-Rosenberg enabled auto-merge (squash) March 6, 2024 16:41
Copy link
Contributor

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explaining!

@Jonathan-Rosenberg Jonathan-Rosenberg merged commit df02f94 into master Mar 7, 2024
48 checks passed
@Jonathan-Rosenberg Jonathan-Rosenberg deleted the docs/unity-table-metadata branch March 7, 2024 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exclude-changelog PR description should not be included in next release changelog minor-change Used for PRs that don't require issue attached
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants