fsk119 commented on code in PR #23809: URL: https://github.com/apache/flink/pull/23809#discussion_r1406993946
########## flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliTableauResultView.java: ########## @@ -128,8 +128,13 @@ private void printBatchResults(AtomicInteger receivedRowCount) { resultDescriptor.getRowDataStringConverter(), resultDescriptor.maxColumnWidth(), false, - false); - style.print(resultRows.iterator(), terminal.writer()); + false, + resultDescriptor.isPrintQueryTimeCost()); + style.print(resultRows.iterator(), terminal.writer(), getQueryBeginTime()); + } + + long getQueryBeginTime() { + return System.currentTimeMillis(); Review Comment: If we get the time here, I think we don't take the submission time into the consideration. ########## flink-table/flink-table-common/src/main/java/org/apache/flink/table/utils/print/TableauStyle.java: ########## @@ -92,12 +95,14 @@ public final class TableauStyle implements PrintStyle { int[] columnWidths, int maxColumnWidth, boolean printNullAsEmpty, - boolean printRowKind) { + boolean printRowKind, + boolean printQueryTimeCost) { Review Comment: `printQueryTimeCost` relies on the input parameter of the `print` method. So I just think whether it's better don't introduce this parameter here? ########## flink-table/flink-table-common/src/main/java/org/apache/flink/table/utils/print/TableauStyle.java: ########## @@ -117,6 +122,10 @@ public final class TableauStyle implements PrintStyle { @Override public void print(Iterator<RowData> it, PrintWriter printWriter) { + print(it, printWriter, -1); Review Comment: If -1 is a magic number, it's better to introduce a special variable to represent its meaning. ########## flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/config/TableConfigOptions.java: ########## @@ -125,6 +126,14 @@ private TableConfigOptions() {} + "This only applies to columns with variable-length types (e.g. CHAR, VARCHAR, STRING) in the streaming mode. " + "Fixed-length types are printed in the batch mode using a deterministic column width."); + @Documentation.TableOption(execMode = Documentation.ExecMode.BATCH) + public static final ConfigOption<Boolean> DISPLAY_QUERY_TIME_COST = + ConfigOptions.key("table.display.query-time-cost") Review Comment: I think we introduce an option that works for the sql client only? If so, I perfer to move it into the sql client options right now. BTW, I think it also influence other SQL statement behaviour. What about `sql-client.display.print-time-cost`? ########## flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/config/TableConfigOptions.java: ########## @@ -125,6 +126,14 @@ private TableConfigOptions() {} + "This only applies to columns with variable-length types (e.g. CHAR, VARCHAR, STRING) in the streaming mode. " + "Fixed-length types are printed in the batch mode using a deterministic column width."); + @Documentation.TableOption(execMode = Documentation.ExecMode.BATCH) + public static final ConfigOption<Boolean> DISPLAY_QUERY_TIME_COST = + ConfigOptions.key("table.display.query-time-cost") + .booleanType() + .defaultValue(false) Review Comment: I think we mark the default value true here. Becaue Hive or Presto both prints time cost by default. You can refer to [hive](https://github.com/apache/hive/blob/master/beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java#L86) for more details ########## flink-table/flink-table-common/src/main/java/org/apache/flink/table/utils/print/TableauStyle.java: ########## @@ -117,6 +122,10 @@ public final class TableauStyle implements PrintStyle { @Override public void print(Iterator<RowData> it, PrintWriter printWriter) { + print(it, printWriter, -1); + } + + public void print(Iterator<RowData> it, PrintWriter printWriter, long queryBeginTime) { Review Comment: Do we need to modify this here. Please take a look at `CliTableauResultView`. SQL Client controls the print style by itself in streaming mode. ![image](https://github.com/apache/flink/assets/33114724/3908fe91-675e-42ee-8bbc-d7a0b5e1936d) -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org