[GitHub] [hudi] yanghua commented on a change in pull request #1770: [HUDI-708]Add temps show and unit test for TempViewCommand

[GitBox <gi...@apache.org>]

yanghua commented on a change in pull request #1770:
URL: https://github.com/apache/hudi/pull/1770#discussion_r454722777



##########
File path: hudi-cli/src/main/java/org/apache/hudi/cli/HoodieCLI.java
##########
@@ -115,4 +115,16 @@ public static synchronized TempViewProvider getTempViewProvider() {
     return tempViewProvider;
   }
 
+  /**
+   * Close tempViewProvider.
+   * <p/>
+   * For test, avoid multiple SparkContexts.
+   */
+  public static synchronized void closeTempViewProvider() {

Review comment:
       We could introduce a new annotation e.g. `@VisibleForTesting` in the future just like Flink has done to only open the public method for testing purposes. WDYT? @vinothchandar 

##########
File path: hudi-cli/src/main/java/org/apache/hudi/cli/commands/TempViewCommand.java
##########
@@ -20,36 +20,51 @@
 
 import org.apache.hudi.cli.HoodieCLI;
 
+import org.apache.hudi.exception.HoodieException;
 import org.springframework.shell.core.CommandMarker;
 import org.springframework.shell.core.annotation.CliCommand;
 import org.springframework.shell.core.annotation.CliOption;
 import org.springframework.stereotype.Component;
 
-import java.io.IOException;
-
 /**
  * CLI command to query/delete temp views.
  */
 @Component
 public class TempViewCommand implements CommandMarker {
 
-  private static final String EMPTY_STRING = "";
-
-  @CliCommand(value = "temp_query", help = "query against created temp view")
+  @CliCommand(value = {"temp_query", "temp query"}, help = "query against created temp view")
   public String query(
-          @CliOption(key = {"sql"}, mandatory = true, help = "select query to run against view") final String sql)
-          throws IOException {
+          @CliOption(key = {"sql"}, mandatory = true, help = "select query to run against view") final String sql) {
+
+    try {
+      HoodieCLI.getTempViewProvider().runQuery(sql);
+      return "Success queried!";

Review comment:
       It would be better to return the similar messages for `try` and `catch` block, for example, `Query ran successfully`?

##########
File path: hudi-cli/src/main/java/org/apache/hudi/cli/utils/SparkTempViewProvider.java
##########
@@ -101,6 +101,17 @@ public void runQuery(String sqlText) {
     }
   }
 
+  @Override
+  public void showAllTables() {

Review comment:
       IMO, maybe `showAllViews` is a better name (just a thought)? I understand here is a method named `deleteTable`.

##########
File path: hudi-cli/src/main/java/org/apache/hudi/cli/utils/TempViewProvider.java
##########
@@ -18,12 +18,18 @@
 
 package org.apache.hudi.cli.utils;
 
+import java.io.Closeable;
 import java.util.List;
 
-public interface TempViewProvider {
+public interface TempViewProvider extends Closeable {
   void createOrReplace(String tableName, List<String> headers, List<List<Comparable>> rows);
 
   void runQuery(String sqlText);
 
+  void showAllTables();
+
   void deleteTable(String tableName);
+
+  @Override

Review comment:
       remove this annotation here.

##########
File path: hudi-cli/src/main/java/org/apache/hudi/cli/commands/TempViewCommand.java
##########
@@ -20,36 +20,51 @@
 
 import org.apache.hudi.cli.HoodieCLI;
 
+import org.apache.hudi.exception.HoodieException;
 import org.springframework.shell.core.CommandMarker;
 import org.springframework.shell.core.annotation.CliCommand;
 import org.springframework.shell.core.annotation.CliOption;
 import org.springframework.stereotype.Component;
 
-import java.io.IOException;
-
 /**
  * CLI command to query/delete temp views.
  */
 @Component
 public class TempViewCommand implements CommandMarker {
 
-  private static final String EMPTY_STRING = "";
-
-  @CliCommand(value = "temp_query", help = "query against created temp view")
+  @CliCommand(value = {"temp_query", "temp query"}, help = "query against created temp view")
   public String query(
-          @CliOption(key = {"sql"}, mandatory = true, help = "select query to run against view") final String sql)
-          throws IOException {
+          @CliOption(key = {"sql"}, mandatory = true, help = "select query to run against view") final String sql) {
+
+    try {
+      HoodieCLI.getTempViewProvider().runQuery(sql);
+      return "Success queried!";
+    } catch (HoodieException ex) {
+      return "Query ran failed!";
+    }
+
+  }
+
+  @CliCommand(value = "temps show", help = "Show all views name")

Review comment:
       Shall we introduce another alias (e.g. `temps_shoe`) to keep the same style with other commands?

##########
File path: hudi-cli/src/test/java/org/apache/hudi/cli/commands/TestTempViewCommand.java
##########
@@ -0,0 +1,84 @@
+/*
+ * 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.hudi.cli.commands;
+
+import org.apache.hudi.cli.HoodieCLI;
+import org.apache.hudi.cli.testutils.AbstractShellBaseIntegrationTest;
+import org.apache.hudi.exception.HoodieException;
+
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.springframework.shell.core.CommandResult;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+public class TestTempViewCommand extends AbstractShellBaseIntegrationTest {
+
+  String tableName = "test_table";

Review comment:
       It would be better to add the access modifier.

##########
File path: hudi-cli/src/test/java/org/apache/hudi/cli/commands/TestTempViewCommand.java
##########
@@ -0,0 +1,84 @@
+/*
+ * 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.hudi.cli.commands;
+
+import org.apache.hudi.cli.HoodieCLI;
+import org.apache.hudi.cli.testutils.AbstractShellBaseIntegrationTest;
+import org.apache.hudi.exception.HoodieException;
+
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.springframework.shell.core.CommandResult;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+public class TestTempViewCommand extends AbstractShellBaseIntegrationTest {
+
+  String tableName = "test_table";
+
+  @BeforeEach
+  public void init() {
+    List<List<Comparable>> rows = new ArrayList<>();
+    for (int i = 0; i < 3; i++) {
+      rows.add(Arrays.asList(new Comparable[] {"c1", "c2", "c3"}));
+    }
+    HoodieCLI.getTempViewProvider().createOrReplace(tableName, Arrays.asList("t1", "t2", "t3"), rows);
+  }
+
+  @AfterAll
+  public static void shutdown() {
+    if (HoodieCLI.getTempViewProvider() != null) {
+      HoodieCLI.closeTempViewProvider();
+    }
+  }
+
+  @Test
+  public void testQueryWithException() {
+    CommandResult cr = getShell().executeCommand(String.format("temp query --sql 'select * from %s'", "table_1"));
+    assertTrue(cr.getResult().toString().startsWith("Query ran failed!"));
+  }
+
+  @Test
+  public void testQuery() {
+    CommandResult cr = getShell().executeCommand(String.format("temp query --sql 'select * from %s'", tableName));
+    assertEquals("Success queried!", cr.getResult().toString());
+  }
+
+  @Test
+  public void testShowAll() {
+    CommandResult cr = getShell().executeCommand(String.format("temps show"));

Review comment:
       No need to call `String.format`?

##########
File path: hudi-cli/src/main/java/org/apache/hudi/cli/commands/TempViewCommand.java
##########
@@ -20,36 +20,51 @@
 
 import org.apache.hudi.cli.HoodieCLI;
 
+import org.apache.hudi.exception.HoodieException;
 import org.springframework.shell.core.CommandMarker;
 import org.springframework.shell.core.annotation.CliCommand;
 import org.springframework.shell.core.annotation.CliOption;
 import org.springframework.stereotype.Component;
 
-import java.io.IOException;
-
 /**
  * CLI command to query/delete temp views.
  */
 @Component
 public class TempViewCommand implements CommandMarker {
 
-  private static final String EMPTY_STRING = "";
-
-  @CliCommand(value = "temp_query", help = "query against created temp view")
+  @CliCommand(value = {"temp_query", "temp query"}, help = "query against created temp view")
   public String query(
-          @CliOption(key = {"sql"}, mandatory = true, help = "select query to run against view") final String sql)
-          throws IOException {
+          @CliOption(key = {"sql"}, mandatory = true, help = "select query to run against view") final String sql) {
+
+    try {
+      HoodieCLI.getTempViewProvider().runQuery(sql);
+      return "Success queried!";

Review comment:
       Considering it will be used to assert in the test case. Maybe we could define a constant message?

##########
File path: hudi-cli/src/main/java/org/apache/hudi/cli/commands/TempViewCommand.java
##########
@@ -20,36 +20,51 @@
 
 import org.apache.hudi.cli.HoodieCLI;
 
+import org.apache.hudi.exception.HoodieException;
 import org.springframework.shell.core.CommandMarker;
 import org.springframework.shell.core.annotation.CliCommand;
 import org.springframework.shell.core.annotation.CliOption;
 import org.springframework.stereotype.Component;
 
-import java.io.IOException;
-
 /**
  * CLI command to query/delete temp views.
  */
 @Component
 public class TempViewCommand implements CommandMarker {
 
-  private static final String EMPTY_STRING = "";
-
-  @CliCommand(value = "temp_query", help = "query against created temp view")
+  @CliCommand(value = {"temp_query", "temp query"}, help = "query against created temp view")
   public String query(
-          @CliOption(key = {"sql"}, mandatory = true, help = "select query to run against view") final String sql)
-          throws IOException {
+          @CliOption(key = {"sql"}, mandatory = true, help = "select query to run against view") final String sql) {
+
+    try {
+      HoodieCLI.getTempViewProvider().runQuery(sql);
+      return "Success queried!";
+    } catch (HoodieException ex) {
+      return "Query ran failed!";
+    }
+
+  }
+
+  @CliCommand(value = "temps show", help = "Show all views name")
+  public String showAll() {
 
-    HoodieCLI.getTempViewProvider().runQuery(sql);
-    return EMPTY_STRING;
+    try {
+      HoodieCLI.getTempViewProvider().showAllTables();
+      return "Show all views name successfully!";
+    } catch (HoodieException ex) {
+      return "Show all views failed!";

Review comment:
       Do we need to add the keyword `name` here?




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org