Re: [PR] FLINK-33600][table] print the query time cost for batch query in cli [flink]

[via GitHub]

JingGe merged PR #23809:
URL: https://github.com/apache/flink/pull/23809


-- 
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

Re: [PR] FLINK-33600][table] print the query time cost for batch query in cli [flink]

[via GitHub]

JingGe commented on PR #23809:
URL: https://github.com/apache/flink/pull/23809#issuecomment-1835434229

   Thanks @fsk119 for your reviews! Appreciate it!


-- 
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

Re: [PR] FLINK-33600][table] print the query time cost for batch query in cli [flink]

[via GitHub]

JingGe commented on PR #23809:
URL: https://github.com/apache/flink/pull/23809#issuecomment-1833917568

   @flinkbot run azure


-- 
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

Re: [PR] FLINK-33600][table] print the query time cost for batch query in cli [flink]

[via GitHub]

fsk119 commented on code in PR #23809:
URL: https://github.com/apache/flink/pull/23809#discussion_r1410478769


##
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliTableauResultView.java:
##
@@ -122,14 +137,83 @@ private void checkAndCleanUpQuery(boolean cleanUpQuery) {
 private void printBatchResults(AtomicInteger receivedRowCount) {
 final List resultRows = waitBatchResults();
 receivedRowCount.addAndGet(resultRows.size());
+
+if (printBatchEmptySet(resultRows)) return;
+printBatchTable(resultRows);
+printBatchFooter(resultRows.size());
+}
+
+/**
+ * Print "Empty set" if the given resultRows is empty, 
otherwise will do nothing.
+ *
+ * @return false if resultRows is not empty
+ */
+public boolean printBatchEmptySet(List resultRows) {
+
+if (!resultRows.isEmpty()) {
+return false;
+}
+
+if (!resultDescriptor.isPrintQueryTimeCost()
+|| DEFAULT_QUERY_BEGIN_TIME == queryBeginTime) {
+terminal.writer().println("Empty set");
+} else {
+String timeCost =
+calculateTimeCostInPrintFormat(queryBeginTime, 
System.currentTimeMillis());
+terminal.writer().println("Empty set" + timeCost);
+}
+terminal.writer().flush();
+return true;
+}
+
+/**
+ * Print table with column names and borders
+ *
+ * @return the row number printed in the table
+ */
+public void printBatchTable(List resultRows) {

Review Comment:
   ok.



-- 
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

Re: [PR] FLINK-33600][table] print the query time cost for batch query in cli [flink]

[via GitHub]

JingGe commented on code in PR #23809:
URL: https://github.com/apache/flink/pull/23809#discussion_r1410463737


##
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliTableauResultView.java:
##
@@ -122,14 +137,83 @@ private void checkAndCleanUpQuery(boolean cleanUpQuery) {
 private void printBatchResults(AtomicInteger receivedRowCount) {
 final List resultRows = waitBatchResults();
 receivedRowCount.addAndGet(resultRows.size());
+
+if (printBatchEmptySet(resultRows)) return;
+printBatchTable(resultRows);
+printBatchFooter(resultRows.size());
+}
+
+/**
+ * Print "Empty set" if the given resultRows is empty, 
otherwise will do nothing.
+ *
+ * @return false if resultRows is not empty
+ */
+public boolean printBatchEmptySet(List resultRows) {
+
+if (!resultRows.isEmpty()) {
+return false;
+}
+
+if (!resultDescriptor.isPrintQueryTimeCost()
+|| DEFAULT_QUERY_BEGIN_TIME == queryBeginTime) {
+terminal.writer().println("Empty set");
+} else {
+String timeCost =
+calculateTimeCostInPrintFormat(queryBeginTime, 
System.currentTimeMillis());
+terminal.writer().println("Empty set" + timeCost);
+}
+terminal.writer().flush();
+return true;
+}
+
+/**
+ * Print table with column names and borders
+ *
+ * @return the row number printed in the table
+ */
+public void printBatchTable(List resultRows) {

Review Comment:
   using two methods is more about the code readability. Table and footer are 
two different concepts in this case. I would suggest keeping them isolated and 
also for better maintainability, e.g. footer might be changed without need to 
know anything about the table like this PR is doing. WDYT? 



-- 
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

Re: [PR] FLINK-33600][table] print the query time cost for batch query in cli [flink]

[via GitHub]

JingGe commented on code in PR #23809:
URL: https://github.com/apache/flink/pull/23809#discussion_r1410463737


##
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliTableauResultView.java:
##
@@ -122,14 +137,83 @@ private void checkAndCleanUpQuery(boolean cleanUpQuery) {
 private void printBatchResults(AtomicInteger receivedRowCount) {
 final List resultRows = waitBatchResults();
 receivedRowCount.addAndGet(resultRows.size());
+
+if (printBatchEmptySet(resultRows)) return;
+printBatchTable(resultRows);
+printBatchFooter(resultRows.size());
+}
+
+/**
+ * Print "Empty set" if the given resultRows is empty, 
otherwise will do nothing.
+ *
+ * @return false if resultRows is not empty
+ */
+public boolean printBatchEmptySet(List resultRows) {
+
+if (!resultRows.isEmpty()) {
+return false;
+}
+
+if (!resultDescriptor.isPrintQueryTimeCost()
+|| DEFAULT_QUERY_BEGIN_TIME == queryBeginTime) {
+terminal.writer().println("Empty set");
+} else {
+String timeCost =
+calculateTimeCostInPrintFormat(queryBeginTime, 
System.currentTimeMillis());
+terminal.writer().println("Empty set" + timeCost);
+}
+terminal.writer().flush();
+return true;
+}
+
+/**
+ * Print table with column names and borders
+ *
+ * @return the row number printed in the table
+ */
+public void printBatchTable(List resultRows) {

Review Comment:
   using two methods is more about the code readability. Table and footer are 
two different concepts in this case. I would suggest keeping them isolated and 
also for better maintainability, e.g. footer might be changed without need to 
know anything about the table. WDYT? 



-- 
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

Re: [PR] FLINK-33600][table] print the query time cost for batch query in cli [flink]

[via GitHub]

fsk119 commented on code in PR #23809:
URL: https://github.com/apache/flink/pull/23809#discussion_r1410477547


##
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliTableauResultView.java:
##
@@ -38,31 +38,46 @@
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Collectors;
 
 /** Print result in tableau mode. */
 public class CliTableauResultView implements AutoCloseable {
 
+public static final long DEFAULT_QUERY_BEGIN_TIME = -1L;
+
 private final Terminal terminal;
 private final ResultDescriptor resultDescriptor;
 
 private final ChangelogResult collectResult;
 private final ExecutorService displayResultExecutorService;
 
-public CliTableauResultView(final Terminal terminal, final 
ResultDescriptor resultDescriptor) {
-this(terminal, resultDescriptor, resultDescriptor.createResult());
+private final long queryBeginTime;
+
+public CliTableauResultView(
+final Terminal terminal, final ResultDescriptor resultDescriptor, 
long queryBeginTime) {
+this(terminal, resultDescriptor, resultDescriptor.createResult(), 
queryBeginTime);
 }
 
 @VisibleForTesting
 public CliTableauResultView(
 final Terminal terminal,
 final ResultDescriptor resultDescriptor,
 final ChangelogResult collectResult) {
+this(terminal, resultDescriptor, collectResult, 
DEFAULT_QUERY_BEGIN_TIME);

Review Comment:
   I think it's better to simplify the logic. I think we only need to control 
with the option only



-- 
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

Re: [PR] FLINK-33600][table] print the query time cost for batch query in cli [flink]

[via GitHub]

JingGe commented on code in PR #23809:
URL: https://github.com/apache/flink/pull/23809#discussion_r1410468089


##
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliTableauResultView.java:
##
@@ -38,31 +38,46 @@
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Collectors;
 
 /** Print result in tableau mode. */
 public class CliTableauResultView implements AutoCloseable {
 
+public static final long DEFAULT_QUERY_BEGIN_TIME = -1L;
+
 private final Terminal terminal;
 private final ResultDescriptor resultDescriptor;
 
 private final ChangelogResult collectResult;
 private final ExecutorService displayResultExecutorService;
 
-public CliTableauResultView(final Terminal terminal, final 
ResultDescriptor resultDescriptor) {
-this(terminal, resultDescriptor, resultDescriptor.createResult());
+private final long queryBeginTime;
+
+public CliTableauResultView(
+final Terminal terminal, final ResultDescriptor resultDescriptor, 
long queryBeginTime) {
+this(terminal, resultDescriptor, resultDescriptor.createResult(), 
queryBeginTime);
 }
 
 @VisibleForTesting
 public CliTableauResultView(
 final Terminal terminal,
 final ResultDescriptor resultDescriptor,
 final ChangelogResult collectResult) {
+this(terminal, resultDescriptor, collectResult, 
DEFAULT_QUERY_BEGIN_TIME);

Review Comment:
   the static member field will be used to control the time cost display logic



-- 
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

Re: [PR] FLINK-33600][table] print the query time cost for batch query in cli [flink]

[via GitHub]

JingGe commented on code in PR #23809:
URL: https://github.com/apache/flink/pull/23809#discussion_r1410463737


##
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliTableauResultView.java:
##
@@ -122,14 +137,83 @@ private void checkAndCleanUpQuery(boolean cleanUpQuery) {
 private void printBatchResults(AtomicInteger receivedRowCount) {
 final List resultRows = waitBatchResults();
 receivedRowCount.addAndGet(resultRows.size());
+
+if (printBatchEmptySet(resultRows)) return;
+printBatchTable(resultRows);
+printBatchFooter(resultRows.size());
+}
+
+/**
+ * Print "Empty set" if the given resultRows is empty, 
otherwise will do nothing.
+ *
+ * @return false if resultRows is not empty
+ */
+public boolean printBatchEmptySet(List resultRows) {
+
+if (!resultRows.isEmpty()) {
+return false;
+}
+
+if (!resultDescriptor.isPrintQueryTimeCost()
+|| DEFAULT_QUERY_BEGIN_TIME == queryBeginTime) {
+terminal.writer().println("Empty set");
+} else {
+String timeCost =
+calculateTimeCostInPrintFormat(queryBeginTime, 
System.currentTimeMillis());
+terminal.writer().println("Empty set" + timeCost);
+}
+terminal.writer().flush();
+return true;
+}
+
+/**
+ * Print table with column names and borders
+ *
+ * @return the row number printed in the table
+ */
+public void printBatchTable(List resultRows) {

Review Comment:
   using two methods is more about code readability. Table and footer are two 
different concepts in this case. I would suggest keep them isolated and also 
for better maintainability, e.g. footer might be changed without need to know 
anything about the table. WDYT? 



-- 
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

Re: [PR] FLINK-33600][table] print the query time cost for batch query in cli [flink]

[via GitHub]

fsk119 commented on code in PR #23809:
URL: https://github.com/apache/flink/pull/23809#discussion_r1410406456


##
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliTableauResultView.java:
##
@@ -122,14 +137,83 @@ private void checkAndCleanUpQuery(boolean cleanUpQuery) {
 private void printBatchResults(AtomicInteger receivedRowCount) {
 final List resultRows = waitBatchResults();
 receivedRowCount.addAndGet(resultRows.size());
+
+if (printBatchEmptySet(resultRows)) return;
+printBatchTable(resultRows);
+printBatchFooter(resultRows.size());
+}
+
+/**
+ * Print "Empty set" if the given resultRows is empty, 
otherwise will do nothing.
+ *
+ * @return false if resultRows is not empty
+ */
+public boolean printBatchEmptySet(List resultRows) {
+
+if (!resultRows.isEmpty()) {
+return false;
+}
+
+if (!resultDescriptor.isPrintQueryTimeCost()
+|| DEFAULT_QUERY_BEGIN_TIME == queryBeginTime) {
+terminal.writer().println("Empty set");
+} else {
+String timeCost =
+calculateTimeCostInPrintFormat(queryBeginTime, 
System.currentTimeMillis());
+terminal.writer().println("Empty set" + timeCost);
+}
+terminal.writer().flush();
+return true;
+}
+
+/**
+ * Print table with column names and borders
+ *
+ * @return the row number printed in the table
+ */
+public void printBatchTable(List resultRows) {

Review Comment:
   public -> private
   
   printBatchTable -> printBatchResultsInternal
   
   BTW, I think we can merge printBatchTable and printBatchFooter into one. 
WDYT?



##
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliTableauResultView.java:
##
@@ -122,14 +137,83 @@ private void checkAndCleanUpQuery(boolean cleanUpQuery) {
 private void printBatchResults(AtomicInteger receivedRowCount) {
 final List resultRows = waitBatchResults();
 receivedRowCount.addAndGet(resultRows.size());
+
+if (printBatchEmptySet(resultRows)) return;
+printBatchTable(resultRows);
+printBatchFooter(resultRows.size());
+}
+
+/**
+ * Print "Empty set" if the given resultRows is empty, 
otherwise will do nothing.
+ *
+ * @return false if resultRows is not empty
+ */
+public boolean printBatchEmptySet(List resultRows) {

Review Comment:
   nit: pubilc -> private.
   
   The method name describes it is an action to do something. At the first 
glance, I think it should retun void. How about
   
   ```
if (resultRows.isEmpty()) {
   printBatchEmptySet();
   } else {
   printBatchTable(resultRows);
   printBatchFooter(resultRows.size());
   }
   ```



##
flink-table/flink-table-common/src/main/java/org/apache/flink/table/utils/print/TableauStyle.java:
##
@@ -154,9 +164,26 @@ public void print(Iterator it, PrintWriter 
printWriter) {
 
 // print border line
 printBorderLine(printWriter);
-final String rowTerm = numRows > 1 ? "rows" : "row";
-printWriter.println(numRows + " " + rowTerm + " in set");
+return numRows;
+}
+
+public boolean printEmptySet(Iterator it, PrintWriter 
printWriter) {

Review Comment:
   Do we need to keep this? I think we can revert the refactor. It's better to 
open a new PR to do this if you need



##
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliTableauResultView.java:
##
@@ -122,14 +137,83 @@ private void checkAndCleanUpQuery(boolean cleanUpQuery) {
 private void printBatchResults(AtomicInteger receivedRowCount) {
 final List resultRows = waitBatchResults();
 receivedRowCount.addAndGet(resultRows.size());
+
+if (printBatchEmptySet(resultRows)) return;
+printBatchTable(resultRows);
+printBatchFooter(resultRows.size());
+}
+
+/**
+ * Print "Empty set" if the given resultRows is empty, 
otherwise will do nothing.
+ *
+ * @return false if resultRows is not empty
+ */
+public boolean printBatchEmptySet(List resultRows) {
+

Review Comment:
   nit: remove the empty blank line here?



##
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliTableauResultView.java:
##
@@ -38,31 +38,46 @@
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Collectors;
 
 /** Print result in tableau mode. */
 public class CliTableauResultView implements AutoCloseable {
 
+public static final long DEFAULT_QUERY_BEGIN_TIME = -1L;
+
 private final Terminal terminal;
 private final ResultDescriptor resultDescriptor;
 
 private final ChangelogResult collectResult;
 private final 

Re: [PR] FLINK-33600][table] print the query time cost for batch query in cli [flink]

[via GitHub]

JingGe commented on code in PR #23809:
URL: https://github.com/apache/flink/pull/23809#discussion_r1410249663


##
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/config/SqlClientOptions.java:
##
@@ -70,6 +70,14 @@ private SqlClientOptions() {}
 + "This only applies to columns with 
variable-length types (e.g. CHAR, VARCHAR, STRING) in streaming mode. "
 + "Fixed-length types and all types in 
batch mode are printed using a deterministic column width.");
 
+@Documentation.TableOption(execMode = Documentation.ExecMode.BATCH)
+public static final ConfigOption DISPLAY_QUERY_TIME_COST =

Review Comment:
   After talking with @fsk119 offline, this PR will focus on the batch and will 
create a follow-up PR for the stream mode.



-- 
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

Re: [PR] FLINK-33600][table] print the query time cost for batch query in cli [flink]

[via GitHub]

JingGe commented on code in PR #23809:
URL: https://github.com/apache/flink/pull/23809#discussion_r1410249663


##
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/config/SqlClientOptions.java:
##
@@ -70,6 +70,14 @@ private SqlClientOptions() {}
 + "This only applies to columns with 
variable-length types (e.g. CHAR, VARCHAR, STRING) in streaming mode. "
 + "Fixed-length types and all types in 
batch mode are printed using a deterministic column width.");
 
+@Documentation.TableOption(execMode = Documentation.ExecMode.BATCH)
+public static final ConfigOption DISPLAY_QUERY_TIME_COST =

Review Comment:
   I'd like to focus on batch in the PR and will create a follow-up PR for the 
stream mode.



-- 
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

Re: [PR] FLINK-33600][table] print the query time cost for batch query in cli [flink]

[via GitHub]

JingGe commented on code in PR #23809:
URL: https://github.com/apache/flink/pull/23809#discussion_r1410249002


##
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliTableauResultView.java:
##
@@ -128,8 +146,9 @@ private void printBatchResults(AtomicInteger 
receivedRowCount) {
 resultDescriptor.getRowDataStringConverter(),
 resultDescriptor.maxColumnWidth(),
 false,
-false);
-style.print(resultRows.iterator(), terminal.writer());
+false,

Review Comment:
   updated accordingly. Please check again. Thanks!



-- 
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

Re: [PR] FLINK-33600][table] print the query time cost for batch query in cli [flink]

[via GitHub]

JingGe commented on code in PR #23809:
URL: https://github.com/apache/flink/pull/23809#discussion_r1410248169


##
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 it, PrintWriter printWriter) {
+print(it, printWriter, -1);
+}
+
+public void print(Iterator it, PrintWriter printWriter, long 
queryBeginTime) {

Review Comment:
   update to control the print style for batch result in `CliTableauResultView` 
too



##
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 it, PrintWriter printWriter) {
+print(it, printWriter, -1);
+}
+
+public void print(Iterator it, PrintWriter printWriter, long 
queryBeginTime) {

Review Comment:
   updated to control the print style for batch result in 
`CliTableauResultView` too



-- 
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

Re: [PR] FLINK-33600][table] print the query time cost for batch query in cli [flink]

[via GitHub]

JingGe commented on code in PR #23809:
URL: https://github.com/apache/flink/pull/23809#discussion_r1410247314


##
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliTableauResultView.java:
##
@@ -42,27 +42,45 @@
 /** Print result in tableau mode. */
 public class CliTableauResultView implements AutoCloseable {
 
+public static final long DEFAULT_QUERY_BEGIN_TIME = -1;
+
 private final Terminal terminal;
 private final ResultDescriptor resultDescriptor;
 
 private final ChangelogResult collectResult;
 private final ExecutorService displayResultExecutorService;
 
+private final long queryBeginTime;
+
 public CliTableauResultView(final Terminal terminal, final 
ResultDescriptor resultDescriptor) {

Review Comment:
   removed



-- 
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

Re: [PR] FLINK-33600][table] print the query time cost for batch query in cli [flink]

[via GitHub]

JingGe commented on code in PR #23809:
URL: https://github.com/apache/flink/pull/23809#discussion_r1409698883


##
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliTableauResultView.java:
##
@@ -42,27 +42,45 @@
 /** Print result in tableau mode. */
 public class CliTableauResultView implements AutoCloseable {
 
+public static final long DEFAULT_QUERY_BEGIN_TIME = -1;
+
 private final Terminal terminal;
 private final ResultDescriptor resultDescriptor;
 
 private final ChangelogResult collectResult;
 private final ExecutorService displayResultExecutorService;
 
+private final long queryBeginTime;
+
 public CliTableauResultView(final Terminal terminal, final 
ResultDescriptor resultDescriptor) {

Review Comment:
   removed



-- 
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

Re: [PR] FLINK-33600][table] print the query time cost for batch query in cli [flink]

[via GitHub]

fsk119 commented on code in PR #23809:
URL: https://github.com/apache/flink/pull/23809#discussion_r1408955517


##
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliTableauResultView.java:
##
@@ -42,27 +42,45 @@
 /** Print result in tableau mode. */
 public class CliTableauResultView implements AutoCloseable {
 
+public static final long DEFAULT_QUERY_BEGIN_TIME = -1;
+
 private final Terminal terminal;
 private final ResultDescriptor resultDescriptor;
 
 private final ChangelogResult collectResult;
 private final ExecutorService displayResultExecutorService;
 
+private final long queryBeginTime;
+
 public CliTableauResultView(final Terminal terminal, final 
ResultDescriptor resultDescriptor) {

Review Comment:
   I mean the constructor at the line 55



-- 
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

Re: [PR] FLINK-33600][table] print the query time cost for batch query in cli [flink]

[via GitHub]

fsk119 commented on code in PR #23809:
URL: https://github.com/apache/flink/pull/23809#discussion_r1408879750


##
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliTableauResultView.java:
##
@@ -128,8 +146,9 @@ private void printBatchResults(AtomicInteger 
receivedRowCount) {
 resultDescriptor.getRowDataStringConverter(),
 resultDescriptor.maxColumnWidth(),
 false,
-false);
-style.print(resultRows.iterator(), terminal.writer());
+false,

Review Comment:
   Can we do like this? The following modification doesn't influence the table 
api behaviour and we can control the timeout by ourself. `TableauStyle` convert 
the RowData to String[] twice in the `print` method.
   
   ```
   private void printBatchResults(AtomicInteger receivedRowCount) {
   final List resultRows = waitBatchResults();
   receivedRowCount.addAndGet(resultRows.size());
   if (resultRows.isEmpty()) {
   terminal.writer()
   .println(
   "Empty set."
   + calculateTimeCostInPrintFormat(
   queryBeginTime, 
System.currentTimeMillis()));
   terminal.flush();
   return;
   }
   
   TableauStyle style =
   PrintStyle.tableauWithDataInferredColumnWidths(
   resultDescriptor.getResultSchema(),
   resultDescriptor.getRowDataStringConverter(),
   resultDescriptor.maxColumnWidth(),
   false,
   false,
   resultDescriptor.isPrintQueryTimeCost());
   List content =
   
resultRows.stream().map(style::rowFieldsToString).collect(Collectors.toList());
   
   // infer column width from the actual content
   style.inferColumnWidth(content);
   // print filed names
   style.printBorderLine(terminal.writer());
   style.printColumnNamesTableauRow(terminal.writer());
   style.printBorderLine(terminal.writer());
   terminal.flush();
   
   for (String[] resultRow : content) {
   if (Thread.currentThread().isInterrupted()) {
   return;
   }
   style.printTableauRow(resultRow, terminal.writer());
   }
   String rowTerm = getRowTerm(receivedRowCount);
   terminal.writer()
   .println(
   "Received a total of "
   + receivedRowCount
   + " "
   + rowTerm
   + calculateTimeCostInPrintFormat(
   queryBeginTime, 
System.currentTimeMillis()));
   }
   ```



##
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliTableauResultView.java:
##
@@ -42,27 +42,45 @@
 /** Print result in tableau mode. */
 public class CliTableauResultView implements AutoCloseable {
 
+public static final long DEFAULT_QUERY_BEGIN_TIME = -1;
+
 private final Terminal terminal;
 private final ResultDescriptor resultDescriptor;
 
 private final ChangelogResult collectResult;
 private final ExecutorService displayResultExecutorService;
 
+private final long queryBeginTime;
+
 public CliTableauResultView(final Terminal terminal, final 
ResultDescriptor resultDescriptor) {

Review Comment:
   Do we need this? Actually, no one uses it and it is a internal api.



##
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/config/SqlClientOptions.java:
##
@@ -70,6 +70,14 @@ private SqlClientOptions() {}
 + "This only applies to columns with 
variable-length types (e.g. CHAR, VARCHAR, STRING) in streaming mode. "
 + "Fixed-length types and all types in 
batch mode are printed using a deterministic column width.");
 
+@Documentation.TableOption(execMode = Documentation.ExecMode.BATCH)
+public static final ConfigOption DISPLAY_QUERY_TIME_COST =

Review Comment:
   It seems the option only works for the batch mode. I just wonder whether we 
should print the time cost for streaming mode. 



##
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliTableauResultView.java:
##
@@ -42,27 +42,45 @@
 /** Print result in tableau mode. */
 public class CliTableauResultView implements AutoCloseable {
 
+public static final long DEFAULT_QUERY_BEGIN_TIME = -1;

Review Comment:
   nit: -1L
   
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please 

Re: [PR] FLINK-33600][table] print the query time cost for batch query in cli [flink]

[via GitHub]

JingGe commented on PR #23809:
URL: https://github.com/apache/flink/pull/23809#issuecomment-1831318436

   @flinkbot run azure


-- 
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

Re: [PR] FLINK-33600][table] print the query time cost for batch query in cli [flink]

[via GitHub]

JingGe commented on code in PR #23809:
URL: https://github.com/apache/flink/pull/23809#discussion_r1408745083


##
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 it, PrintWriter printWriter) {
+print(it, printWriter, -1);
+}
+
+public void print(Iterator it, PrintWriter printWriter, long 
queryBeginTime) {

Review Comment:
   Not sure I understand you correctly. For batch result the footer line is 
printed/controlled by `TableauStyle`.



-- 
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

Re: [PR] FLINK-33600][table] print the query time cost for batch query in cli [flink]

[via GitHub]

JingGe commented on code in PR #23809:
URL: https://github.com/apache/flink/pull/23809#discussion_r1408742394


##
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:
   both `printQueryTimeCost` and the input parameter `queryBeginTime` will be 
used to make decision whether the time cost should be displayed.



-- 
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

Re: [PR] FLINK-33600][table] print the query time cost for batch query in cli [flink]

[via GitHub]

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 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 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 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 it, PrintWriter printWriter) {
+print(it, printWriter, -1);
+}
+
+public void print(Iterator 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

Re: [PR] FLINK-33600][table] print the query time cost for batch query in cli [flink]

[via GitHub]

JingGe commented on PR #23809:
URL: https://github.com/apache/flink/pull/23809#issuecomment-1827956246

   @flinkbot run azure


-- 
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

Re: [PR] FLINK-33600][table] print the query time cost for batch query in cli [flink]

[via GitHub]

JingGe commented on PR #23809:
URL: https://github.com/apache/flink/pull/23809#issuecomment-1827902974

   @flinkbot run azure


-- 
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

Re: [PR] FLINK-33600][table] print the query time cost for batch query in cli [flink]

[via GitHub]

flinkbot commented on PR #23809:
URL: https://github.com/apache/flink/pull/23809#issuecomment-1827310156

   
   ## CI report:
   
   * 20ebdff8ebda287f1640c770a3bbe164a29f2be8 UNKNOWN
   
   
   Bot commands
 The @flinkbot bot supports the following commands:
   
- `@flinkbot run azure` re-run the last Azure build
   


-- 
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

[PR] FLINK-33600][table] print the query time cost for batch query in cli [flink]

[via GitHub]

JingGe opened a new pull request, #23809:
URL: https://github.com/apache/flink/pull/23809

   ## What is the purpose of the change
   
   print the query time cost for batch query in cli
   
   
   ## Brief change log
   
 - define new table config to swtich the display the query time cost on/off
 - extend `PrintStyle` and `TableauStyle`
 - update `CliTableauResultView` to show the result for batch
 - extend tests to cover the new feature
   
   
   ## Verifying this change
   
   After update existing tests, this change is already covered by those tests, 
such as `TableauStyleTest` and `CliTableauResultViewTest`.
   
   ## Does this pull request potentially affect one of the following parts:
   
 - Dependencies (does it add or upgrade a dependency): (yes / **no**)
 - The public API, i.e., is any changed class annotated with 
`@Public(Evolving)`: (yes / **no**)
 - The serializers: (yes / **no** / don't know)
 - The runtime per-record code paths (performance sensitive): (yes / **no** 
/ don't know)
 - Anything that affects deployment or recovery: JobManager (and its 
components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (yes / **no** / don't 
know)
 - The S3 file system connector: (yes / **no** / don't know)
   
   ## Documentation
   
 - Does this pull request introduce a new feature? (**yes** / no)
 - Document will be updated in another PR once the community reach the 
consensus of the solution.
   


-- 
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