tengqm commented on code in PR #6461: URL: https://github.com/apache/gravitino/pull/6461#discussion_r1957512935
########## clients/cli/src/main/java/org/apache/gravitino/cli/outputs/BaseOutputFormat.java: ########## @@ -0,0 +1,99 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.gravitino.cli.outputs; + +import com.google.common.base.Preconditions; +import java.io.BufferedOutputStream; +import java.io.IOException; +import java.io.OutputStream; +import java.io.PrintStream; +import java.io.UncheckedIOException; +import java.nio.charset.StandardCharsets; +import org.apache.gravitino.cli.CommandContext; + +/** + * Abstract base implementation of {@link OutputFormat} interface providing common functionality for + * various output format implementations. This class handles basic output operations and provides + * configurable behavior for quiet mode, output limiting. + */ +public abstract class BaseOutputFormat<T> implements OutputFormat<T> { + protected int limit; + protected CommandContext context; + + /** + * Creates a new {@link BaseOutputFormat} with specified configuration. + * + * @param context the command context, must not be null; + */ + public BaseOutputFormat(CommandContext context) { + Preconditions.checkNotNull(context, "CommandContext cannot be null"); Review Comment: ```suggestion Preconditions.checkNotNull(context, "context cannot be null"); ``` ########## clients/cli/src/main/java/org/apache/gravitino/cli/outputs/BaseOutputFormat.java: ########## @@ -0,0 +1,99 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.gravitino.cli.outputs; + +import com.google.common.base.Preconditions; +import java.io.BufferedOutputStream; +import java.io.IOException; +import java.io.OutputStream; +import java.io.PrintStream; +import java.io.UncheckedIOException; +import java.nio.charset.StandardCharsets; +import org.apache.gravitino.cli.CommandContext; + +/** + * Abstract base implementation of {@link OutputFormat} interface providing common functionality for + * various output format implementations. This class handles basic output operations and provides + * configurable behavior for quiet mode, output limiting. + */ +public abstract class BaseOutputFormat<T> implements OutputFormat<T> { + protected int limit; + protected CommandContext context; + + /** + * Creates a new {@link BaseOutputFormat} with specified configuration. + * + * @param context the command context, must not be null; + */ + public BaseOutputFormat(CommandContext context) { + Preconditions.checkNotNull(context, "CommandContext cannot be null"); + this.context = context; + this.limit = context.outputLimit(); + } + + /** + * Outputs a message to the specified OutputStream. This method handles both system streams + * ({@code System.out}, {@code System.err}) and regular output streams differently: - For system + * streams: Preserves the stream open after writing - For other streams: Automatically closes the + * stream after writing + * + * @param message the message to output, must not be null + * @param os the output stream to write to, must not be null If this is {@code System.out} or + * {@code System.err}, the stream will not be closed + * @throws IllegalArgumentException if either message or os is null + * @throws UncheckedIOException if an I/O error occurs during writing + */ + public static void output(String message, OutputStream os) { + if (message == null || os == null) { + throw new IllegalArgumentException("Message and OutputStream cannot be null"); Review Comment: The error string should mention the parameter name rather than its type, right? ########## clients/cli/src/main/java/org/apache/gravitino/cli/outputs/PlainFormat.java: ########## @@ -21,71 +21,247 @@ import java.util.Arrays; import java.util.List; import java.util.stream.Collectors; +import org.apache.gravitino.Audit; import org.apache.gravitino.Catalog; import org.apache.gravitino.Metalake; +import org.apache.gravitino.Schema; +import org.apache.gravitino.cli.CommandContext; +import org.apache.gravitino.rel.Table; -/** Plain format to print a pretty string to standard out. */ -public class PlainFormat { - public static void output(Object object) { - if (object instanceof Metalake) { - new MetalakePlainFormat().output((Metalake) object); - } else if (object instanceof Metalake[]) { - new MetalakesPlainFormat().output((Metalake[]) object); - } else if (object instanceof Catalog) { - new CatalogPlainFormat().output((Catalog) object); - } else if (object instanceof Catalog[]) { - new CatalogsPlainFormat().output((Catalog[]) object); +/** + * Formats entity into plain text representation for command-line output. Supports formatting of + * single objects and arrays of Metalake, Catalog, Schema, and Table objects. Each supported type + * has its own specialized formatter as an inner class. + */ +public abstract class PlainFormat<T> extends BaseOutputFormat<T> { + + /** + * Routes the object to its appropriate formatter and outputs the formatted result. Creates a new + * formatter instance for the given object type and delegates the formatting. + * + * @param entity The object to format + * @param context The command context + * @throws IllegalArgumentException if the object type is not supported + */ + public static void output(Object entity, CommandContext context) { + if (entity instanceof Metalake) { Review Comment: Please change this long chain of `if...else...` into a `switch` ? ########## clients/cli/src/main/java/org/apache/gravitino/cli/commands/ListTables.java: ########## @@ -47,24 +48,26 @@ public ListTables(CommandContext context, String metalake, String catalog, Strin /** List the names of all tables in a schema. */ @Override public void handle() { - NameIdentifier[] tables = null; + NameIdentifier[] tables; Namespace name = Namespace.of(schema); + TableCatalog tableCatalog; + List<Table> tablesList = new ArrayList<>(); + try { - tables = tableCatalog().listTables(name); + // There may be a performance overhead + tableCatalog = tableCatalog(); + tables = tableCatalog.listTables(name); + for (NameIdentifier table : tables) { + tablesList.add(tableCatalog.loadTable(table)); + } } catch (Exception exp) { exitWithError(exp.getMessage()); } - List<String> tableNames = new ArrayList<>(); - for (int i = 0; i < tables.length; i++) { - tableNames.add(tables[i].name()); + if (tablesList.isEmpty()) { + printResults("No tables exist."); Review Comment: Did you mean `printInfo` here? ########## clients/cli/src/main/java/org/apache/gravitino/cli/outputs/PlainFormat.java: ########## @@ -21,71 +21,247 @@ import java.util.Arrays; import java.util.List; import java.util.stream.Collectors; +import org.apache.gravitino.Audit; import org.apache.gravitino.Catalog; import org.apache.gravitino.Metalake; +import org.apache.gravitino.Schema; +import org.apache.gravitino.cli.CommandContext; +import org.apache.gravitino.rel.Table; -/** Plain format to print a pretty string to standard out. */ -public class PlainFormat { - public static void output(Object object) { - if (object instanceof Metalake) { - new MetalakePlainFormat().output((Metalake) object); - } else if (object instanceof Metalake[]) { - new MetalakesPlainFormat().output((Metalake[]) object); - } else if (object instanceof Catalog) { - new CatalogPlainFormat().output((Catalog) object); - } else if (object instanceof Catalog[]) { - new CatalogsPlainFormat().output((Catalog[]) object); +/** + * Formats entity into plain text representation for command-line output. Supports formatting of + * single objects and arrays of Metalake, Catalog, Schema, and Table objects. Each supported type + * has its own specialized formatter as an inner class. + */ +public abstract class PlainFormat<T> extends BaseOutputFormat<T> { + + /** + * Routes the object to its appropriate formatter and outputs the formatted result. Creates a new + * formatter instance for the given object type and delegates the formatting. + * + * @param entity The object to format + * @param context The command context + * @throws IllegalArgumentException if the object type is not supported + */ + public static void output(Object entity, CommandContext context) { + if (entity instanceof Metalake) { + new MetalakePlainFormat(context).output((Metalake) entity); + } else if (entity instanceof Metalake[]) { + new MetalakesPlainFormat(context).output((Metalake[]) entity); + } else if (entity instanceof Catalog) { + new CatalogPlainFormat(context).output((Catalog) entity); + } else if (entity instanceof Catalog[]) { + new CatalogsPlainFormat(context).output((Catalog[]) entity); + } else if (entity instanceof Schema) { + new SchemaPlainFormat(context).output((Schema) entity); + } else if (entity instanceof Schema[]) { + new SchemasPlainFormat(context).output((Schema[]) entity); + } else if (entity instanceof Table) { + new TablePlainFormat(context).output((Table) entity); + } else if (entity instanceof Table[]) { + new TablesPlainFormat(context).output((Table[]) entity); + } else if (entity instanceof Audit) { + new AuditPlainFormat(context).output((Audit) entity); } else { throw new IllegalArgumentException("Unsupported object type"); } } - static final class MetalakePlainFormat implements OutputFormat<Metalake> { + /** + * Creates a new {@link PlainFormat} with the specified output properties. + * + * @param context The command context. + */ + public PlainFormat(CommandContext context) { + super(context); + } + + /** + * Formats a single {@link Metalake} instance as a comma-separated string. Output format: name, + * comment + */ + static final class MetalakePlainFormat extends PlainFormat<Metalake> { + + public MetalakePlainFormat(CommandContext context) { + super(context); + } + + /** {@inheritDoc} */ @Override - public void output(Metalake metalake) { - System.out.println(metalake.name() + "," + metalake.comment()); + public String getOutput(Metalake metalake) { + return COMMA_JOINER.join(metalake.name(), metalake.comment()); } } - static final class MetalakesPlainFormat implements OutputFormat<Metalake[]> { + /** + * Formats an array of Metalakes, outputting one name per line. Returns null if the array is empty + * or null. + */ + static final class MetalakesPlainFormat extends PlainFormat<Metalake[]> { + + public MetalakesPlainFormat(CommandContext context) { + super(context); + } + + /** {@inheritDoc} */ @Override - public void output(Metalake[] metalakes) { - if (metalakes.length == 0) { - System.out.println("No metalakes exist."); + public String getOutput(Metalake[] metalakes) { + if (metalakes == null || metalakes.length == 0) { + return null; } else { List<String> metalakeNames = Arrays.stream(metalakes).map(Metalake::name).collect(Collectors.toList()); - String all = String.join(System.lineSeparator(), metalakeNames); - System.out.println(all); + return NEWLINE_JOINER.join(metalakeNames); } } } - static final class CatalogPlainFormat implements OutputFormat<Catalog> { + /** + * Formats a single {@link Catalog} instance as a comma-separated string. Output format: name, + * type, provider, comment + */ + static final class CatalogPlainFormat extends PlainFormat<Catalog> { + public CatalogPlainFormat(CommandContext context) { + super(context); + } + + /** {@inheritDoc} */ @Override - public void output(Catalog catalog) { - System.out.println( - catalog.name() - + "," - + catalog.type() - + "," - + catalog.provider() - + "," - + catalog.comment()); + public String getOutput(Catalog catalog) { + return COMMA_JOINER.join( + catalog.name(), catalog.type(), catalog.provider(), catalog.comment()); } } - static final class CatalogsPlainFormat implements OutputFormat<Catalog[]> { + /** + * Formats an array of Catalogs, outputting one name per line. Returns null if the array is empty + * or null. + */ + static final class CatalogsPlainFormat extends PlainFormat<Catalog[]> { + public CatalogsPlainFormat(CommandContext context) { + super(context); + } + + /** {@inheritDoc} */ @Override - public void output(Catalog[] catalogs) { - if (catalogs.length == 0) { - System.out.println("No catalogs exist."); + public String getOutput(Catalog[] catalogs) { + if (catalogs == null || catalogs.length == 0) { + output("No catalogs exists.", System.err); + return null; } else { List<String> catalogNames = Arrays.stream(catalogs).map(Catalog::name).collect(Collectors.toList()); - String all = String.join(System.lineSeparator(), catalogNames); - System.out.println(all); + return NEWLINE_JOINER.join(catalogNames); } } } + + /** + * Formats a single {@link Schema} instance as a comma-separated string. Output format: name, + * comment + */ + static final class SchemaPlainFormat extends PlainFormat<Schema> { + public SchemaPlainFormat(CommandContext context) { + super(context); + } + + /** {@inheritDoc} */ + @Override + public String getOutput(Schema schema) { + return COMMA_JOINER.join(schema.name(), schema.comment()); + } + } + + /** + * Formats an array of Schemas, outputting one name per line. Returns null if the array is empty + * or null. + */ + static final class SchemasPlainFormat extends PlainFormat<Schema[]> { Review Comment: The difference between `SchemaPlainFormat` and `SchemasPlainFormat` can be made more noticeable. e.g. `SchemaListPlainFormat`. ########## clients/cli/src/main/java/org/apache/gravitino/cli/outputs/Column.java: ########## @@ -0,0 +1,245 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.gravitino.cli.outputs; + +import com.google.common.collect.Lists; +import java.util.List; +import org.apache.gravitino.cli.CommandContext; + +/** + * Represents a column in a formatted table output. Manages column properties including header, + * alignment, and content cells. Handles width calculations. + */ +public class Column { Review Comment: This class can be spit into a separate PR. ########## clients/cli/src/main/java/org/apache/gravitino/cli/outputs/BaseOutputFormat.java: ########## @@ -0,0 +1,99 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.gravitino.cli.outputs; + +import com.google.common.base.Preconditions; +import java.io.BufferedOutputStream; +import java.io.IOException; +import java.io.OutputStream; +import java.io.PrintStream; +import java.io.UncheckedIOException; +import java.nio.charset.StandardCharsets; +import org.apache.gravitino.cli.CommandContext; + +/** + * Abstract base implementation of {@link OutputFormat} interface providing common functionality for + * various output format implementations. This class handles basic output operations and provides + * configurable behavior for quiet mode, output limiting. + */ +public abstract class BaseOutputFormat<T> implements OutputFormat<T> { + protected int limit; + protected CommandContext context; + + /** + * Creates a new {@link BaseOutputFormat} with specified configuration. + * + * @param context the command context, must not be null; + */ + public BaseOutputFormat(CommandContext context) { + Preconditions.checkNotNull(context, "CommandContext cannot be null"); + this.context = context; + this.limit = context.outputLimit(); Review Comment: I'm not sure this `limit` is gonna be a generic parameter for most commands. ########## clients/cli/src/main/java/org/apache/gravitino/cli/outputs/Constant.java: ########## @@ -0,0 +1,62 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.gravitino.cli.outputs; + +import com.google.common.collect.ImmutableList; + +public class Constant { Review Comment: I'm not sure we want to beautify the table output to this extent. It is meaningless, at least to me. -- 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
