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.

Sign up for GitHub

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

Jump to bottom

[#5745] feat(CLI): Table format output for ListCatalogs command #5759

Merged
merged 11 commits into from
Dec 16, 2024
Merged

[#5745] feat(CLI): Table format output for ListCatalogs command #5759

merged 11 commits into from
Dec 16, 2024

Conversation

waukin
Copy link
Contributor

@waukin waukin commented Dec 4, 2024

What changes were proposed in this pull request?

Support table format output for ListCatalogs command.

Why are the changes needed?

Issue: #5745

Does this PR introduce any user-facing change?

No.

How was this patch tested?

gcli catalog list -m <metalake_name>
gcli catalog list -m <metalake_name> --output plain
gcli catalog list -m <metalake_name> --output table

@waukin
Copy link
Contributor Author

waukin commented Dec 4, 2024

Hi @xunliu, This PR is ready for review. Please take a look.

xunliu
xunliu reviewed Dec 5, 2024
View reviewed changes
clients/cli/src/main/java/org/apache/gravitino/cli/outputs/PlainFormat.java Outdated
@@ -33,6 +33,8 @@ public static void output(Object object) {
new MetalakesStringFormat().output((Metalake[]) object);
} else if (object instanceof Catalog) {
new CatalogStringFormat().output((Catalog) object);
} else if (object instanceof Catalog[]) {
new CatalogsStringFormat().output((Catalog[]) object);
Copy link
Member

Choose a reason for hiding this comment

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

Please change CatalogsStringFormat to CatalogsPlainFormat, and MetalakesStringFormat , CatalogStringFormat and other *StringFormat class

Copy link
Member

@justinmclean justinmclean Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I would probably call them CatalogPlainFormat, MetalakePlainFormat etc, but it is probably OK to leave the "s" in.

Copy link
Member

Choose a reason for hiding this comment

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

The CatalogPlainFormat output Catalog object.
The CatalogsPlainFormat output Catalog[] objects.
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in this commit: f96173d

clients/cli/src/main/java/org/apache/gravitino/cli/outputs/TableFormat.java Show resolved Hide resolved
tengqm
tengqm reviewed Dec 6, 2024
View reviewed changes
clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java Outdated
@@ -201,7 +201,7 @@ private void handleCatalogCommand() {
String outputFormat = line.getOptionValue(GravitinoOptions.OUTPUT);

if (CommandActions.LIST.equals(command)) {
newListCatalogs(url, ignore, metalake).handle();
newListCatalogs(url, ignore, metalake, outputFormat).handle();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest we come up with a ListOptions wrapper for this case.
In the foreseeable future, you may want to add filter, sorting, field selector, paging options. The list of parameters to pass would grow and you have to change all these places again and again. This process is tedious and error prone.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can pass the CommondOption value in the function params.
Through CommondOption we can get any option values.
@tengqm @justinmclean What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest we don't overly generalize the options.
For example, you may want to model ListOptions, DeleteOptions, UpdateOptions.
The outputFormat, or sortBy option can then be encapsulated into ListOptions,
while an option like force can be encapsulated into DeleteOptions.

Copy link
Member

@justinmclean justinmclean Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I would not pass the CommondOption in the function parameters, as then the command needs to know about the CLI library we use, and it makes it hard to know what options a command uses. Currently, we only have one list option and one delete option. I think we could change to a ListOptions or similar when adding extra options.

@waukin waukin force-pushed the feature/list_catalogs_table_format_ouput branch from f96173d to e3d36c1 Compare December 9, 2024 12:15
justinmclean
justinmclean reviewed Dec 10, 2024
View reviewed changes
clients/cli/src/main/java/org/apache/gravitino/cli/commands/CatalogDetails.java Outdated
@@ -35,14 +36,17 @@ public class CatalogDetails extends Command {
*
* @param url The URL of the Gravitino server.
* @param ignoreVersions If true don't check the client/server versions match.
* @param outputFormat The output format.
* @param listOptions The list options.
Copy link
Member

@justinmclean justinmclean Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The name of this variable is confusing. You have also removed useful information i.e. that it controls the output format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted to using outputFormat for @param.

justinmclean
justinmclean reviewed Dec 10, 2024
View reviewed changes
clients/cli/src/main/java/org/apache/gravitino/cli/commands/Command.java Outdated
@@ -168,14 +169,14 @@ protected <T extends GravitinoClientBase> Builder<T> constructClient(Builder<T>
* @param <T> The type of entity.
*/
protected <T> void output(T entity) {
if (outputFormat == null) {
if (listOptions.getOutputFormat() == null) {
Copy link
Member

Choose a reason for hiding this comment

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

Can listOptions be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The listOptions feature is not implemented at this stage.

justinmclean
justinmclean reviewed Dec 11, 2024
View reviewed changes
clients/cli/src/test/java/org/apache/gravitino/cli/integration/test/TableFormatOutputIT.java
@@ -145,21 +146,19 @@ public void testMetalakeDetailsCommand() {
}

@Test
public void testCatalogDetailsCommand() {
public void testCatalogListCommand() {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why this has changed from a Details command to a List command don't we want to test both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I lost the testCatalogDetailsCommand() during the rebase, but I have already reverted it.

@waukin waukin force-pushed the feature/list_catalogs_table_format_ouput branch from 2235193 to 93a464c Compare December 12, 2024 13:59
xunliu
xunliu approved these changes Dec 16, 2024
View reviewed changes
Copy link
Member

@xunliu xunliu left a comment

Choose a reason for hiding this comment

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

@waukin Thank you for your contributions.
LGTM

@xunliu xunliu merged commit 8021812 into apache:main Dec 16, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tengqm tengqm tengqm left review comments

@justinmclean justinmclean justinmclean left review comments

@xunliu xunliu xunliu approved these changes

Assignees

@waukin waukin

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@waukin @justinmclean @xunliu @tengqm