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<RowData> resultRows = waitBatchResults();
receivedRowCount.addAndGet(resultRows.size());
+
+ if (printBatchEmptySet(resultRows)) return;
+ printBatchTable(resultRows);
+ printBatchFooter(resultRows.size());
+ }
+
+ /**
+ * Print "Empty set" if the given <code>resultRows</code> is empty,
otherwise will do nothing.
+ *
+ * @return false if <code>resultRows</code> is not empty
+ */
+ public boolean printBatchEmptySet(List<RowData> 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<RowData> 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<RowData> resultRows = waitBatchResults();
receivedRowCount.addAndGet(resultRows.size());
+
+ if (printBatchEmptySet(resultRows)) return;
+ printBatchTable(resultRows);
+ printBatchFooter(resultRows.size());
+ }
+
+ /**
+ * Print "Empty set" if the given <code>resultRows</code> is empty,
otherwise will do nothing.
+ *
+ * @return false if <code>resultRows</code> is not empty
+ */
+ public boolean printBatchEmptySet(List<RowData> 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<RowData> 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<RowData> 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<RowData> resultRows = waitBatchResults();
receivedRowCount.addAndGet(resultRows.size());
+
+ if (printBatchEmptySet(resultRows)) return;
+ printBatchTable(resultRows);
+ printBatchFooter(resultRows.size());
+ }
+
+ /**
+ * Print "Empty set" if the given <code>resultRows</code> is empty,
otherwise will do nothing.
+ *
+ * @return false if <code>resultRows</code> is not empty
+ */
+ public boolean printBatchEmptySet(List<RowData> 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 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:
Do we need to introduce a static member fields here if it is just used by
tests? I think we can use the System.currentMills() if no one specifies the
begin time.
--
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
