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

[#6496] feat(CLI): Support plain format output for Schema and Table command #6497

Merged
merged 4 commits into from
Feb 26, 2025

Conversation

Abyss-lord
Copy link
Contributor

@Abyss-lord Abyss-lord commented Feb 22, 2025

What changes were proposed in this pull request?

Support plain format output for Schema and Table command

Why are the changes needed?

Fix: #6496

Does this PR introduce any user-facing change?

No

How was this patch tested?

local test

gcli schema list -m demo_metalake --name Hive_catalog  -i
default,Default Hive database

gcli table list -m demo_metalake --name Hive_catalog.default -i 
employee_partitions
source_table
test_dt_partition
test_key_en
sales
order_tb1
tmp
float_test
test_tbl

gcli table details -m demo_metalake --name Hive_catalog.default.test1  -i 
test1,N/A

gcli schema details -m demo_metalake --name Hive_catalog.default -i 
default,Default Hive database

@Abyss-lord
Copy link
Contributor Author

@justinmclean @tengqm could you please review this PR when you have time? I’d really appreciate your feedback.

@justinmclean
Copy link
Member

It looks good to me, except for one minor thing with the output. There should be no spaces after the comma. The output is basically CSV, and we should not add extra spaces.

@tengqm
Copy link
Contributor

tengqm commented Feb 24, 2025

What's the use case for the CSV output?

@justinmclean
Copy link
Member

It's not so much a use case for CSV. This change changes the output and could break user scripts using our CLI.

@justinmclean
Copy link
Member

Similar to the table output PR there's no real need to call loadTable/loadSchema

@tengqm
Copy link
Contributor

tengqm commented Feb 24, 2025

Even if we have a use case for this, we'll need a design for the output.
For example, should we always quote a field value with ""? What if there are , in a field value that breaks the parser? How do we want a boolean value to be presented? true or "true" or TRUE or something else? How will we present null values? so on and so forth.

We NEVER do something simply because it is DOABLE. There are simply a thousand ways to spend your precious time.

@Abyss-lord
Copy link
Contributor Author

Even if we have a use case for this, we'll need a design for the output. For example, should we always quote a field value with ""? What if there are , in a field value that breaks the parser? How do we want a boolean value to be presented? true or "true" or TRUE or something else? How will we present null values? so on and so forth.

We NEVER do something simply because it is DOABLE. There are simply a thousand ways to spend your precious time.

@tengqm The main thing is that we can't use web UI on our side, and users are not professional and technical personnel. There is no need to use Java API or REST API to create and delete tables for now. We need to provide a simple and practical client-side script for non-technical people to query(just list\ details).

@tengqm
Copy link
Contributor

tengqm commented Feb 24, 2025

Even if we have a use case for this, we'll need a design for the output. For example, should we always quote a field value with ""? What if there are , in a field value that breaks the parser? How do we want a boolean value to be presented? true or "true" or TRUE or something else? How will we present null values? so on and so forth.
We NEVER do something simply because it is DOABLE. There are simply a thousand ways to spend your precious time.

@tengqm The main thing is that we can't use web UI on our side, and users are not professional and technical personnel. There is no need to use Java API or REST API to create and delete tables for now. We need to provide a simple and practical client-side script for non-technical people to query(just list\ details).

Okay, we can support tabular output for command line users, where the most important fields are included. For cases where there are too many fields, we can simply dump the JSON response. So ... I don't see a need for CSV output. Am I missing something?

@Abyss-lord
Copy link
Contributor Author

Even if we have a use case for this, we'll need a design for the output. For example, should we always quote a field value with ""? What if there are , in a field value that breaks the parser? How do we want a boolean value to be presented? true or "true" or TRUE or something else? How will we present null values? so on and so forth.
We NEVER do something simply because it is DOABLE. There are simply a thousand ways to spend your precious time.

@tengqm The main thing is that we can't use web UI on our side, and users are not professional and technical personnel. There is no need to use Java API or REST API to create and delete tables for now. We need to provide a simple and practical client-side script for non-technical people to query(just list\ details).

Okay, we can support tabular output for command line users, where the most important fields are included. For cases where there are too many fields, we can simply dump the JSON response. So ... I don't see a need for CSV output. Am I missing something?

@tengqm @justinmclean Yes, just to be consistent with the tableformat output logic, Whether the plainformat and tableformat outputs are inconsistent for the same entity

@justinmclean
Copy link
Member

If people want JSON output they can use the REST interface. JSON output was discarded very early on as an aim for the CLI.

@tengqm
Copy link
Contributor

tengqm commented Feb 25, 2025

If people want JSON output they can use the REST interface. JSON output was discarded very early on as an aim for the CLI.

Good to know. gcli doesn't support -o json, so people have to use curl, right?

@justinmclean
Copy link
Member

Yes the REST interface can be interacted with using curl.

@Abyss-lord
Copy link
Contributor Author

@justinmclean should the plainformat and tableformat outputs are inconsistent for the same entity? or plainformat show different messages.

@justinmclean
Copy link
Member

@Abyss-lord I'm not sure what you mean here, can you point to an example?

…able command

Support plain format output for Schema and Table command
@Abyss-lord
Copy link
Contributor Author

Abyss-lord commented Feb 25, 2025

@Abyss-lord I'm not sure what you mean here, can you point to an example?

@justinmclean in table format table details will return as follows:

+----------+---------+---------------+----------+---------+
|   Name   |  Type   | AutoIncrement | Nullable | Comment |
+----------+---------+---------------+----------+---------+
| id       | integer | false         | true     | N/A     |
| name     | string  | false         | true     | N/A     |
| standard | long    | false         | true     | N/A     |
| dt       | string  | false         | true     | N/A     |
+----------+---------+---------------+----------+---------+

in plain format, it should return as same as table format or just return
table_name, comment?

@justinmclean
Copy link
Member

justinmclean commented Feb 25, 2025

The original object was that a LIST command should give just the basic information, i.e. name and comment, and a DETAILS command would provide more details. They should be consistent across both table and plain formats.

@Abyss-lord Abyss-lord force-pushed the feat-schema-table-plain-format branch from 36822fd to b587c72 Compare February 25, 2025 08:17
@Abyss-lord
Copy link
Contributor Author

Abyss-lord commented Feb 25, 2025

The original object was that a LIST command should give just the basic information, i.e. name and comment, and a DETAILS command would provide more details. I don't know why the table list command has more fields than name and

@justinmclean yes Justin , details should give more information, I’ve finished updating the code. Please take a look at the PR again when you have time.

@justinmclean
Copy link
Member

Have the space after coma's been removed in the plain output?

@Abyss-lord
Copy link
Contributor Author

Abyss-lord commented Feb 25, 2025

Have the space after coma's been removed in the plain output?

@justinmclean The COMMA JOINER has removed the Spaces.

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 the contribution.

@justinmclean justinmclean merged commit af9c2b4 into apache:main Feb 26, 2025
27 checks passed
@Abyss-lord Abyss-lord deleted the feat-schema-table-plain-format branch February 26, 2025 03:15
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.

[FEATURE] Support plain format output for Schema and Table command
3 participants