tengqm commented on code in PR #6495:
URL: https://github.com/apache/gravitino/pull/6495#discussion_r1966491313
##########
clients/cli/src/main/java/org/apache/gravitino/cli/commands/ListTables.java:
##########
@@ -49,22 +50,32 @@ public ListTables(CommandContext context, String metalake,
String catalog, Strin
public void handle() {
NameIdentifier[] tables = null;
Namespace name = Namespace.of(schema);
+ TableCatalog tableCatalog = null;
+
try {
- tables = tableCatalog().listTables(name);
+ tableCatalog = tableCatalog();
+ tables = tableCatalog.listTables(name);
} catch (Exception exp) {
exitWithError(exp.getMessage());
}
List<String> tableNames = new ArrayList<>();
- for (int i = 0; i < tables.length; i++) {
- tableNames.add(tables[i].name());
+ for (NameIdentifier table : tables) {
+ tableNames.add(table.name());
+ }
+ // PERF load table may cause performance issue
+ Table[] tableObjects = new Table[tableNames.size()];
+ int i = 0;
+ for (NameIdentifier tableIdent : tables) {
+ tableObjects[i] = tableCatalog.loadTable(tableIdent);
+ i++;
}
- String all =
- tableNames.isEmpty()
- ? "No tables exist."
- : Joiner.on(System.lineSeparator()).join(tableNames);
+ if (tableNames.isEmpty()) {
+ printInformation("No tables exist.");
Review Comment:
Here we see an inconsistency between the information we print for different
LIST.
Sometimes we print the catalog name, sometimes we don't.
##########
clients/cli/src/main/java/org/apache/gravitino/cli/commands/ListSchema.java:
##########
@@ -60,8 +64,16 @@ public void handle() {
exitWithError(exp.getMessage());
}
- String all = schemas.length == 0 ? "No schemas exist." :
Joiner.on(",").join(schemas);
+ if (schemas.length == 0) {
+ printInformation("No schemas found in catalog " + catalog);
+ return;
+ }
+ // PERF load table may cause performance issue
+ Schema[] schemaObjects = new Schema[schemas.length];
+ for (int i = 0; i < schemas.length; i++) {
+ schemaObjects[i] = tableCatalog.asSchemas().loadSchema(schemas[i]);
Review Comment:
line 58 has a listSchemas() call, I'm not sure how heavy an operation that
is.
line 74 reads each and every schema again.
For a "LIST schemas" operation, do we really want to do these TWO iterations?
##########
clients/cli/src/main/java/org/apache/gravitino/cli/outputs/TableFormat.java:
##########
@@ -75,6 +77,14 @@ public static void output(Object entity, CommandContext
context) {
new CatalogTableFormat(context).output((Catalog) entity);
} else if (entity instanceof Catalog[]) {
new CatalogListTableFormat(context).output((Catalog[]) entity);
+ } else if (entity instanceof Schema) {
+ new SchemaTableFormat(context).output((Schema) entity);
+ } else if (entity instanceof Schema[]) {
+ new SchemaListTableFormat(context).output((Schema[]) entity);
+ } else if (entity instanceof Table) {
+ new TableDetailsTableFormat(context).output((Table) entity);
+ } else if (entity instanceof Table[]) {
+ new TableListTableFormat(context).output((Table[]) entity);
Review Comment:
In Java 12, there is pattern matching for a list of classes:
```java
switch (entity) {
case Metalake metalake -> new
MetalakeTableFormat(context).output(metalake);
case Metalake[] metaLakeList ->
newMetalakeListTableFormat(context).output(metaLakeList);
// ...
default:
//...
}
```
##########
clients/cli/src/main/java/org/apache/gravitino/cli/commands/TableDetails.java:
##########
@@ -57,6 +57,8 @@ public void handle() {
exitWithError(exp.getMessage());
}
- printInformation(gTable.name() + "," + gTable.comment());
+ if (gTable != null) {
+ printResults(gTable);
+ }
Review Comment:
Here is another inconsistency. We don't print "verbose" information if table
is not found?
##########
clients/cli/src/main/java/org/apache/gravitino/cli/outputs/TableFormat.java:
##########
@@ -540,4 +550,97 @@ public String getOutput(Catalog[] catalogs) {
return getTableFormat(columnName);
}
}
+
+ /**
+ * Formats a single {@link Schema} instance into a two-column table display.
Displays catalog
+ * details including name and comment information.
+ */
+ static final class SchemaTableFormat extends TableFormat<Schema> {
+ public SchemaTableFormat(CommandContext context) {
+ super(context);
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public String getOutput(Schema schema) {
Review Comment:
The actual logic that gets overridden is the process of building a list of
Columns.
So is it possible to refactor this function into a protected function
`getColumns`, and then we invoke the public `getOutput` function from the
parent class?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@gravitino.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
