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

[#5277] improvement(cli): display index information for tables in the Gravitino CLI #5476

Merged
merged 14 commits into from
Nov 14, 2024

Conversation

orenccl
Copy link
Collaborator

@orenccl orenccl commented Nov 5, 2024

What changes were proposed in this pull request?

Add --index option to display index information on Tables.

Fix TableDetails command.

Update cli/README.md for table commands

Why are the changes needed?

This change allows users to retrieve additional index information on Tables, providing more insights.

Fix: #5277

Does this PR introduce any user-facing change?

Yes, it adds the --index option to CommandEntities.TABLE.

How was this patch tested?

  1. Follow the instructions in the cli README to build the CLI sub-project.
  2. Start the Gravitino Playground.

To test, use a command like the following:

gcli -i table details --metalake metalake_demo --name catalog_mysql.db.iceberg_namespace_properties

gcli -i table details --metalake metalake_demo --name catalog_mysql.db.iceberg_namespace_properties --index

gcli -i table details --metalake metalake_demo --name catalog_postgres.hr.employee_performance

Check that the output matches the expected audit information.
image
image
image

@orenccl orenccl changed the title [#5277] improvement: display index infromation for tables in the Gravitino CLI [#5277] improvement(cli): display index infromation for tables in the Gravitino CLI Nov 5, 2024
@justinmclean
Copy link
Member

Thanks for the PR. For it to be consistent with other commands, I would output in CSV format with an index (and its type) on each line.

@orenccl
Copy link
Collaborator Author

orenccl commented Nov 6, 2024

image
Fixed format

@orenccl orenccl marked this pull request as draft November 6, 2024 12:33
@orenccl orenccl force-pushed the improvement/displayIndexInfoInCLI branch from 4cc789c to 30567dd Compare November 6, 2024 12:34
@orenccl orenccl marked this pull request as ready for review November 6, 2024 12:38
@orenccl orenccl force-pushed the improvement/displayIndexInfoInCLI branch from 30567dd to 415b62e Compare November 6, 2024 12:39
@orenccl orenccl marked this pull request as draft November 6, 2024 16:04
@orenccl orenccl force-pushed the improvement/displayIndexInfoInCLI branch from 415b62e to da09889 Compare November 6, 2024 16:27
@orenccl orenccl marked this pull request as ready for review November 6, 2024 16:51
clients/cli/README.md Outdated Show resolved Hide resolved
@orenccl orenccl marked this pull request as draft November 7, 2024 02:08
@orenccl orenccl marked this pull request as ready for review November 7, 2024 02:14
@orenccl orenccl force-pushed the improvement/displayIndexInfoInCLI branch from 26ad68d to 7c47bc3 Compare November 7, 2024 16:49
@orenccl orenccl marked this pull request as draft November 8, 2024 05:25
@orenccl orenccl force-pushed the improvement/displayIndexInfoInCLI branch from 7c47bc3 to 096623b Compare November 8, 2024 05:26
@orenccl orenccl marked this pull request as ready for review November 8, 2024 05:29
@orenccl orenccl marked this pull request as draft November 8, 2024 05:50
@orenccl orenccl force-pushed the improvement/displayIndexInfoInCLI branch from 4d87f93 to 1ee1138 Compare November 8, 2024 05:52
@orenccl orenccl marked this pull request as ready for review November 8, 2024 05:53
@orenccl
Copy link
Collaborator Author

orenccl commented Nov 8, 2024

Hi @justinmclean

This is also done!

docs/cli.md Outdated Show resolved Hide resolved
@jerryshao
Copy link
Contributor

My question from another PR (#5525 (comment)), why do we need a new option to display the index information, can we just display all the table metadata information in one details command?

@orenccl orenccl force-pushed the improvement/displayIndexInfoInCLI branch from 247ac5b to 4ad6d99 Compare November 11, 2024 15:57
@justinmclean
Copy link
Member

See #5525 (comment)

@justinmclean
Copy link
Member

@jerryshao can we move forward on this?

@jerryshao
Copy link
Contributor

@jerryshao can we move forward on this?

I've left the comment in another PR, I'm +0 on the current design choice, but I don't want to block things.

@justinmclean justinmclean changed the title [#5277] improvement(cli): display index infromation for tables in the Gravitino CLI [#5277] improvement(cli): display index information for tables in the Gravitino CLI Nov 14, 2024
Copy link
Member

@justinmclean justinmclean 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 your contribution

@justinmclean
Copy link
Member

@jerryshao can you approve this? I approved this but I can't merge it.

@jerryshao jerryshao merged commit 76f29c9 into apache:main Nov 14, 2024
23 checks passed
@orenccl orenccl deleted the improvement/displayIndexInfoInCLI branch November 19, 2024 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement] Display index infromation for tables in the Gravitino CLI
3 participants