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 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 {
+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 th
