You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by GitBox <gi...@apache.org> on 2020/06/27 14:24:27 UTC
[GitHub] [hudi] hddong opened a new pull request #1770: [HUDI-708]Add temps show and unit test for TempViewCommand
hddong opened a new pull request #1770:
URL: https://github.com/apache/hudi/pull/1770
## *Tips*
- *Thank you very much for contributing to Apache Hudi.*
- *Please review https://hudi.apache.org/contributing.html before opening a pull request.*
## What is the purpose of the pull request
*1. Add command `temps show`*
*2. Add unit test for TempViewCommand*
## Brief change log
*(for example:)*
- *Modify AnnotationLocation checkstyle rule in checkstyle.xml*
## Verify this pull request
This pull request is a trivial rework / code cleanup without any test coverage.
## Committer checklist
- [x] Has a corresponding JIRA in PR title & commit
- [ ] Commit message is descriptive of the change
- [ ] CI is green
- [ ] Necessary doc changes done or have another open PR
- [ ] For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.
----------------------------------------------------------------
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
[GitHub] [hudi] yanghua commented on a change in pull request #1770: [HUDI-708]Add temps show and unit test for TempViewCommand
Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1770:
URL: https://github.com/apache/hudi/pull/1770#discussion_r459159775
##########
File path: hudi-cli/src/main/java/org/apache/hudi/cli/commands/TempViewCommand.java
##########
@@ -20,36 +20,55 @@
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 = "";
+ public static final String QUERY_SUCCESS = "Query ran successfully!";
+ public static final String QUERY_FAIL = "Query ran failed!";
+ public static final String SHOW_SUCCESS = "Show all views name successfully!";
- @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 QUERY_SUCCESS;
+ } catch (HoodieException ex) {
+ return QUERY_FAIL;
Review comment:
OK~
##########
File path: hudi-cli/src/main/java/org/apache/hudi/cli/commands/TempViewCommand.java
##########
@@ -20,36 +20,55 @@
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 = "";
+ public static final String QUERY_SUCCESS = "Query ran successfully!";
+ public static final String QUERY_FAIL = "Query ran failed!";
+ public static final String SHOW_SUCCESS = "Show all views name successfully!";
- @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 QUERY_SUCCESS;
+ } catch (HoodieException ex) {
+ return QUERY_FAIL;
+ }
+
+ }
+
+ @CliCommand(value = {"temps_show", "temps show"}, help = "Show all views name")
+ public String showAll() {
- HoodieCLI.getTempViewProvider().runQuery(sql);
- return EMPTY_STRING;
+ try {
+ HoodieCLI.getTempViewProvider().showAllViews();
+ return SHOW_SUCCESS;
+ } catch (HoodieException ex) {
+ return "Show all views name failed!";
Review comment:
ditto
##########
File path: hudi-cli/src/main/java/org/apache/hudi/cli/commands/TempViewCommand.java
##########
@@ -20,36 +20,55 @@
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 = "";
+ public static final String QUERY_SUCCESS = "Query ran successfully!";
+ public static final String QUERY_FAIL = "Query ran failed!";
+ public static final String SHOW_SUCCESS = "Show all views name successfully!";
- @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 QUERY_SUCCESS;
+ } catch (HoodieException ex) {
+ return QUERY_FAIL;
+ }
+
+ }
+
+ @CliCommand(value = {"temps_show", "temps show"}, help = "Show all views name")
+ public String showAll() {
- HoodieCLI.getTempViewProvider().runQuery(sql);
- return EMPTY_STRING;
+ try {
+ HoodieCLI.getTempViewProvider().showAllViews();
+ return SHOW_SUCCESS;
+ } catch (HoodieException ex) {
+ return "Show all views name failed!";
+ }
}
- @CliCommand(value = "temp_delete", help = "Delete view name")
+ @CliCommand(value = {"temp_delete", "temp delete"}, help = "Delete view name")
public String delete(
- @CliOption(key = {"view"}, mandatory = true, help = "view name") final String tableName)
- throws IOException {
+ @CliOption(key = {"view"}, mandatory = true, help = "view name") final String tableName) {
- HoodieCLI.getTempViewProvider().deleteTable(tableName);
- return EMPTY_STRING;
+ try {
+ HoodieCLI.getTempViewProvider().deleteTable(tableName);
+ return String.format("Delete view %s successfully!", tableName);
+ } catch (HoodieException ex) {
+ return String.format("Delete view %s failed!", tableName);
Review comment:
ditto
----------------------------------------------------------------
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
[GitHub] [hudi] yanghua commented on a change in pull request #1770: [HUDI-708]Add temps show and unit test for TempViewCommand
Posted by 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
[GitHub] [hudi] yanghua commented on a change in pull request #1770: [HUDI-708]Add temps show and unit test for TempViewCommand
Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1770:
URL: https://github.com/apache/hudi/pull/1770#discussion_r458466053
##########
File path: hudi-cli/src/main/java/org/apache/hudi/cli/commands/TempViewCommand.java
##########
@@ -20,36 +20,55 @@
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 = "";
+ public static final String QUERY_SUCCESS = "Query ran successfully!";
+ public static final String QUERY_FAIL = "Query ran failed!";
+ public static final String SHOW_SUCCESS = "Show all views name successfully!";
- @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 QUERY_SUCCESS;
+ } catch (HoodieException ex) {
+ return QUERY_FAIL;
+ }
+
+ }
+
+ @CliCommand(value = {"temps_show", "temps show"}, help = "Show all views name")
+ public String showAll() {
- HoodieCLI.getTempViewProvider().runQuery(sql);
- return EMPTY_STRING;
+ try {
+ HoodieCLI.getTempViewProvider().showAllViews();
+ return SHOW_SUCCESS;
+ } catch (HoodieException ex) {
+ return "Show all views name failed!";
+ }
}
- @CliCommand(value = "temp_delete", help = "Delete view name")
+ @CliCommand(value = {"temp_delete", "temp delete"}, help = "Delete view name")
public String delete(
- @CliOption(key = {"view"}, mandatory = true, help = "view name") final String tableName)
- throws IOException {
+ @CliOption(key = {"view"}, mandatory = true, help = "view name") final String tableName) {
- HoodieCLI.getTempViewProvider().deleteTable(tableName);
- return EMPTY_STRING;
+ try {
+ HoodieCLI.getTempViewProvider().deleteTable(tableName);
+ return String.format("Delete view %s successfully!", tableName);
+ } catch (HoodieException ex) {
+ return String.format("Delete view %s failed!", tableName);
Review comment:
ditto
##########
File path: hudi-cli/src/main/java/org/apache/hudi/cli/commands/TempViewCommand.java
##########
@@ -20,36 +20,55 @@
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 = "";
+ public static final String QUERY_SUCCESS = "Query ran successfully!";
+ public static final String QUERY_FAIL = "Query ran failed!";
+ public static final String SHOW_SUCCESS = "Show all views name successfully!";
- @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 QUERY_SUCCESS;
+ } catch (HoodieException ex) {
+ return QUERY_FAIL;
+ }
+
+ }
+
+ @CliCommand(value = {"temps_show", "temps show"}, help = "Show all views name")
+ public String showAll() {
- HoodieCLI.getTempViewProvider().runQuery(sql);
- return EMPTY_STRING;
+ try {
+ HoodieCLI.getTempViewProvider().showAllViews();
+ return SHOW_SUCCESS;
+ } catch (HoodieException ex) {
+ return "Show all views name failed!";
Review comment:
ditto
##########
File path: hudi-cli/src/main/java/org/apache/hudi/cli/commands/TempViewCommand.java
##########
@@ -20,36 +20,55 @@
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 = "";
+ public static final String QUERY_SUCCESS = "Query ran successfully!";
+ public static final String QUERY_FAIL = "Query ran failed!";
+ public static final String SHOW_SUCCESS = "Show all views name successfully!";
- @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 QUERY_SUCCESS;
+ } catch (HoodieException ex) {
+ return QUERY_FAIL;
Review comment:
Can we log the detailed exception information before returning the result?
##########
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 showAllViews() {
+ try {
+ sqlContext.sql("SHOW TABLES").show(Integer.MAX_VALUE, false);
+ } catch (Throwable ex) {
+ // log full stack trace and rethrow. Without this its difficult to debug failures, if any
+ LOG.error("unable to initialize spark context ", ex);
Review comment:
IMO, this error description is not correct, right? The throwable not only covers the initialization of the spark context
----------------------------------------------------------------
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
[GitHub] [hudi] hddong commented on a change in pull request #1770: [HUDI-708]Add temps show and unit test for TempViewCommand
Posted by GitBox <gi...@apache.org>.
hddong commented on a change in pull request #1770:
URL: https://github.com/apache/hudi/pull/1770#discussion_r458817753
##########
File path: hudi-cli/src/main/java/org/apache/hudi/cli/commands/TempViewCommand.java
##########
@@ -20,36 +20,55 @@
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 = "";
+ public static final String QUERY_SUCCESS = "Query ran successfully!";
+ public static final String QUERY_FAIL = "Query ran failed!";
+ public static final String SHOW_SUCCESS = "Show all views name successfully!";
- @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 QUERY_SUCCESS;
+ } catch (HoodieException ex) {
+ return QUERY_FAIL;
Review comment:
`runQuery` had log the detailed exception.
----------------------------------------------------------------
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
[GitHub] [hudi] hddong commented on a change in pull request #1770: [HUDI-708]Add temps show and unit test for TempViewCommand
Posted by GitBox <gi...@apache.org>.
hddong commented on a change in pull request #1770:
URL: https://github.com/apache/hudi/pull/1770#discussion_r458817753
##########
File path: hudi-cli/src/main/java/org/apache/hudi/cli/commands/TempViewCommand.java
##########
@@ -20,36 +20,55 @@
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 = "";
+ public static final String QUERY_SUCCESS = "Query ran successfully!";
+ public static final String QUERY_FAIL = "Query ran failed!";
+ public static final String SHOW_SUCCESS = "Show all views name successfully!";
- @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 QUERY_SUCCESS;
+ } catch (HoodieException ex) {
+ return QUERY_FAIL;
Review comment:
`runQuery` `showAllViews ` and `deleteTable` had log the detailed exception.
----------------------------------------------------------------
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
[GitHub] [hudi] codecov-commenter commented on pull request #1770: [HUDI-708]Add temps show and unit test for TempViewCommand
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1770:
URL: https://github.com/apache/hudi/pull/1770#issuecomment-660950311
# [Codecov](https://codecov.io/gh/apache/hudi/pull/1770?src=pr&el=h1) Report
> Merging [#1770](https://codecov.io/gh/apache/hudi/pull/1770?src=pr&el=desc) into [master](https://codecov.io/gh/apache/hudi/commit/c7f1a781ab4ff3784d53a102364fd85e379811d1&el=desc) will **decrease** coverage by `5.84%`.
> The diff coverage is `64.00%`.
[](https://codecov.io/gh/apache/hudi/pull/1770?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #1770 +/- ##
============================================
- Coverage 60.48% 54.63% -5.85%
+ Complexity 3627 3022 -605
============================================
Files 439 404 -35
Lines 19007 16867 -2140
Branches 1916 1664 -252
============================================
- Hits 11496 9215 -2281
- Misses 6725 7028 +303
+ Partials 786 624 -162
```
| Flag | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| #hudicli | `68.96% <64.00%> (+0.58%)` | `1447.00 <6.00> (+18.00)` | |
| #hudiclient | `79.25% <ø> (+0.05%)` | `1257.00 <ø> (ø)` | |
| #hudicommon | `54.74% <ø> (+0.45%)` | `1508.00 <ø> (+22.00)` | |
| #hudihadoopmr | `?` | `?` | |
| #hudihivesync | `?` | `?` | |
| #hudispark | `19.83% <ø> (-27.50%)` | `19.00 <ø> (-83.00)` | |
| #huditimelineservice | `?` | `?` | |
| #hudiutilities | `12.07% <ø> (-62.50%)` | `48.00 <ø> (-231.00)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/hudi/pull/1770?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...g/apache/hudi/cli/utils/SparkTempViewProvider.java](https://codecov.io/gh/apache/hudi/pull/1770/diff?src=pr&el=tree#diff-aHVkaS1jbGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY2xpL3V0aWxzL1NwYXJrVGVtcFZpZXdQcm92aWRlci5qYXZh) | `59.67% <55.55%> (+59.67%)` | `12.00 <2.00> (+12.00)` | |
| [.../org/apache/hudi/cli/commands/TempViewCommand.java](https://codecov.io/gh/apache/hudi/pull/1770/diff?src=pr&el=tree#diff-aHVkaS1jbGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY2xpL2NvbW1hbmRzL1RlbXBWaWV3Q29tbWFuZC5qYXZh) | `69.23% <66.66%> (+49.23%)` | `4.00 <3.00> (+3.00)` | |
| [...i/src/main/java/org/apache/hudi/cli/HoodieCLI.java](https://codecov.io/gh/apache/hudi/pull/1770/diff?src=pr&el=tree#diff-aHVkaS1jbGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY2xpL0hvb2RpZUNMSS5qYXZh) | `89.18% <75.00%> (+7.37%)` | `18.00 <1.00> (+3.00)` | |
| [...g/apache/hudi/keygen/GlobalDeleteKeyGenerator.java](https://codecov.io/gh/apache/hudi/pull/1770/diff?src=pr&el=tree#diff-aHVkaS1zcGFyay9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaHVkaS9rZXlnZW4vR2xvYmFsRGVsZXRlS2V5R2VuZXJhdG9yLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-7.00%)` | |
| [...a/org/apache/hudi/utilities/sources/RowSource.java](https://codecov.io/gh/apache/hudi/pull/1770/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL3NvdXJjZXMvUm93U291cmNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-4.00%)` | |
| [.../org/apache/hudi/utilities/sources/AvroSource.java](https://codecov.io/gh/apache/hudi/pull/1770/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL3NvdXJjZXMvQXZyb1NvdXJjZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [.../org/apache/hudi/utilities/sources/JsonSource.java](https://codecov.io/gh/apache/hudi/pull/1770/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL3NvdXJjZXMvSnNvblNvdXJjZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [...rg/apache/hudi/utilities/sources/CsvDFSSource.java](https://codecov.io/gh/apache/hudi/pull/1770/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL3NvdXJjZXMvQ3N2REZTU291cmNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-10.00%)` | |
| [...g/apache/hudi/utilities/sources/JsonDFSSource.java](https://codecov.io/gh/apache/hudi/pull/1770/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL3NvdXJjZXMvSnNvbkRGU1NvdXJjZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-4.00%)` | |
| [...apache/hudi/utilities/sources/JsonKafkaSource.java](https://codecov.io/gh/apache/hudi/pull/1770/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL3NvdXJjZXMvSnNvbkthZmthU291cmNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-6.00%)` | |
| ... and [103 more](https://codecov.io/gh/apache/hudi/pull/1770/diff?src=pr&el=tree-more) | |
----------------------------------------------------------------
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
[GitHub] [hudi] yanghua merged pull request #1770: [HUDI-708]Add temps show and unit test for TempViewCommand
Posted by GitBox <gi...@apache.org>.
yanghua merged pull request #1770:
URL: https://github.com/apache/hudi/pull/1770
----------------------------------------------------------------
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
[GitHub] [hudi] yanghua commented on pull request #1770: [HUDI-708]Add temps show and unit test for TempViewCommand
Posted by GitBox <gi...@apache.org>.
yanghua commented on pull request #1770:
URL: https://github.com/apache/hudi/pull/1770#issuecomment-657907638
> @yanghua : It's ready, please have a review when free.
Will review soon.
----------------------------------------------------------------
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
[GitHub] [hudi] hddong commented on a change in pull request #1770: [HUDI-708]Add temps show and unit test for TempViewCommand
Posted by GitBox <gi...@apache.org>.
hddong commented on a change in pull request #1770:
URL: https://github.com/apache/hudi/pull/1770#discussion_r458821046
##########
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 showAllViews() {
+ try {
+ sqlContext.sql("SHOW TABLES").show(Integer.MAX_VALUE, false);
+ } catch (Throwable ex) {
+ // log full stack trace and rethrow. Without this its difficult to debug failures, if any
+ LOG.error("unable to initialize spark context ", ex);
Review comment:
> IMO, this error description is not correct, right? The throwable not only covers the initialization of the spark context
Yes, change it to `unable to get all views`.
----------------------------------------------------------------
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
[GitHub] [hudi] hddong commented on pull request #1770: [HUDI-708]Add temps show and unit test for TempViewCommand
Posted by GitBox <gi...@apache.org>.
hddong commented on pull request #1770:
URL: https://github.com/apache/hudi/pull/1770#issuecomment-657904289
@yanghua : It's ready, please have a review when free.
----------------------------------------------------------------
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
[GitHub] [hudi] hddong commented on a change in pull request #1770: [HUDI-708]Add temps show and unit test for TempViewCommand
Posted by GitBox <gi...@apache.org>.
hddong commented on a change in pull request #1770:
URL: https://github.com/apache/hudi/pull/1770#discussion_r458817019
##########
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:
+1, @VisibleForTesting can be useful.
----------------------------------------------------------------
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