You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by yjhyjhyjh0 <gi...@git.apache.org> on 2018/11/07 16:31:27 UTC
[GitHub] nifi pull request #3156: NIFI-5780 Add pre and post statements to ExecuteSQL...
GitHub user yjhyjhyjh0 opened a pull request:
https://github.com/apache/nifi/pull/3156
NIFI-5780 Add pre and post statements to ExecuteSQL and ExecuteSQLRecord
Add pre, post query property to AbstractExecuteSQL.
Most of implementation comes from SelectHiveQL.
Add unit test to pre, post query.
Finish local nifi integration test.
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [x] Has your PR been rebased against the latest commit within the target branch (typically master)?
- [x] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
- [x] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
- [ ] If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
- [x] If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [x] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/yjhyjhyjh0/nifi NIFI-5780
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/3156.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #3156
----
commit e5ec707036b04607217c276c616c145aad54df4c
Author: yjhyjhyjh0 <yj...@...>
Date: 2018-11-07T16:25:50Z
NIFI-5780 Add pre and post statements to ExecuteSQL and ExecuteSQLRecord
----
---
[GitHub] nifi pull request #3156: NIFI-5780 Add pre and post statements to ExecuteSQL...
Posted by patricker <gi...@git.apache.org>.
Github user patricker commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3156#discussion_r232148571
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/AbstractExecuteSQL.java ---
@@ -82,6 +84,16 @@
.identifiesControllerService(DBCPService.class)
.build();
+ public static final PropertyDescriptor SQL_PRE_QUERY = new PropertyDescriptor.Builder()
+ .name("sql-pre-query")
+ .displayName("SQL Pre-Query")
+ .description("SQL pre-query to execute. Semicolon-delimited list of queries. "
--- End diff --
Sounds good. Can you squash your next commit?
---
[GitHub] nifi issue #3156: NIFI-5780 Add pre and post statements to ExecuteSQL and Ex...
Posted by patricker <gi...@git.apache.org>.
Github user patricker commented on the issue:
https://github.com/apache/nifi/pull/3156
+1 Looks good. Built and tested. I ran into one issue that I might submit a seperate enhancement for (allowing `;` in the SQL string if they are surrounded by `'` or `"`).
---
[GitHub] nifi pull request #3156: NIFI-5780 Add pre and post statements to ExecuteSQL...
Posted by patricker <gi...@git.apache.org>.
Github user patricker commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3156#discussion_r231759360
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/AbstractExecuteSQL.java ---
@@ -82,6 +84,16 @@
.identifiesControllerService(DBCPService.class)
.build();
+ public static final PropertyDescriptor SQL_PRE_QUERY = new PropertyDescriptor.Builder()
+ .name("sql-pre-query")
+ .displayName("SQL pre-query")
--- End diff --
They are capitalized in the HIVE processors, so this would match them.
---
[GitHub] nifi pull request #3156: NIFI-5780 Add pre and post statements to ExecuteSQL...
Posted by yjhyjhyjh0 <gi...@git.apache.org>.
Github user yjhyjhyjh0 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3156#discussion_r231754034
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestExecuteSQLRecord.java ---
@@ -350,6 +352,141 @@ public void invokeOnTriggerRecords(final Integer queryTimeout, final String quer
assertEquals(durationTime, fetchTime + executionTime);
}
+ @Test
+ public void testPreQuery() throws Exception {
+ // remove previous test database, if any
+ final File dbLocation = new File(DB_LOCATION);
+ dbLocation.delete();
+
+ // load test data to database
+ final Connection con = ((DBCPService) runner.getControllerService("dbcp")).getConnection();
+ Statement stmt = con.createStatement();
+
+ try {
+ stmt.execute("drop table TEST_NULL_INT");
+ } catch (final SQLException sqle) {
+ }
+
+ stmt.execute("create table TEST_NULL_INT (id integer not null, val1 integer, val2 integer, constraint my_pk primary key (id))");
+
+ runner.setIncomingConnection(true);
+ runner.setProperty(ExecuteSQL.SQL_PRE_QUERY, "insert into TEST_NULL_INT values(1,2,3);insert into TEST_NULL_INT values(4,5,6)");
--- End diff --
That's indeed great use case, I'll update test cases.
Thanks for the sharing the information.
---
[GitHub] nifi pull request #3156: NIFI-5780 Add pre and post statements to ExecuteSQL...
Posted by patricker <gi...@git.apache.org>.
Github user patricker commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3156#discussion_r231603266
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestExecuteSQLRecord.java ---
@@ -350,6 +352,141 @@ public void invokeOnTriggerRecords(final Integer queryTimeout, final String quer
assertEquals(durationTime, fetchTime + executionTime);
}
+ @Test
+ public void testPreQuery() throws Exception {
+ // remove previous test database, if any
+ final File dbLocation = new File(DB_LOCATION);
+ dbLocation.delete();
+
+ // load test data to database
+ final Connection con = ((DBCPService) runner.getControllerService("dbcp")).getConnection();
+ Statement stmt = con.createStatement();
+
+ try {
+ stmt.execute("drop table TEST_NULL_INT");
+ } catch (final SQLException sqle) {
+ }
+
+ stmt.execute("create table TEST_NULL_INT (id integer not null, val1 integer, val2 integer, constraint my_pk primary key (id))");
+
+ runner.setIncomingConnection(true);
+ runner.setProperty(ExecuteSQL.SQL_PRE_QUERY, "insert into TEST_NULL_INT values(1,2,3);insert into TEST_NULL_INT values(4,5,6)");
--- End diff --
I know these tests were easy to write with insert/delete, but they don't really show why this feature is needed.
The idea is that we need to configure the DBCP connection in some way, but that putting two ExecuteSQL processors together might cause us to use different connections from the connection pool.
Could you try changing them to set Derby session properties?Maybe something from here? https://db.apache.org/derby/docs/10.1/ref/rrefsetdbpropproc.html
This one looked good, it tells Derby to capture runtime statistics for the current connection, and then turns them back off after, so a legitimate use case.
Pre: `CALL SYSCS_UTIL.SYSCS_SET_RUNTIMESTATISTICS(1)`
Post: `CALL SYSCS_UTIL.SYSCS_SET_RUNTIMESTATISTICS(0)`
---
[GitHub] nifi pull request #3156: NIFI-5780 Add pre and post statements to ExecuteSQL...
Posted by yjhyjhyjh0 <gi...@git.apache.org>.
Github user yjhyjhyjh0 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3156#discussion_r232292846
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/AbstractExecuteSQL.java ---
@@ -82,6 +84,16 @@
.identifiesControllerService(DBCPService.class)
.build();
+ public static final PropertyDescriptor SQL_PRE_QUERY = new PropertyDescriptor.Builder()
+ .name("sql-pre-query")
+ .displayName("SQL Pre-Query")
+ .description("SQL pre-query to execute. Semicolon-delimited list of queries. "
--- End diff --
Sounds great to me.
I'll also modify Hive description and squash the commit.
---
[GitHub] nifi issue #3156: NIFI-5780 Add pre and post statements to ExecuteSQL and Ex...
Posted by patricker <gi...@git.apache.org>.
Github user patricker commented on the issue:
https://github.com/apache/nifi/pull/3156
Sorry for the delay, been having issues with a bug that just got fixed in NIFI-5822. Will wrap up testing.
---
[GitHub] nifi pull request #3156: NIFI-5780 Add pre and post statements to ExecuteSQL...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/3156
---
[GitHub] nifi pull request #3156: NIFI-5780 Add pre and post statements to ExecuteSQL...
Posted by patricker <gi...@git.apache.org>.
Github user patricker commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3156#discussion_r232072895
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/AbstractExecuteSQL.java ---
@@ -94,6 +106,16 @@
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
.build();
+ public static final PropertyDescriptor SQL_POST_QUERY = new PropertyDescriptor.Builder()
+ .name("sql-post-query")
+ .displayName("SQL Post-Query")
+ .description("SQL post-query to execute. Semicolon-delimited list of queries. "
--- End diff --
"A semicolon-delimited list of queries executed after the main SQL query is executed. Results/outputs from these queries will be suppressed if there are no errors."
---
[GitHub] nifi issue #3156: NIFI-5780 Add pre and post statements to ExecuteSQL and Ex...
Posted by yjhyjhyjh0 <gi...@git.apache.org>.
Github user yjhyjhyjh0 commented on the issue:
https://github.com/apache/nifi/pull/3156
Thanks for the suggestion.
Update description to be more precise in ExecuteSQL, ExecuteSQLRecord, SelectHiveQL.
Squash the this commit.
---
[GitHub] nifi pull request #3156: NIFI-5780 Add pre and post statements to ExecuteSQL...
Posted by yjhyjhyjh0 <gi...@git.apache.org>.
Github user yjhyjhyjh0 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3156#discussion_r231753838
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/AbstractExecuteSQL.java ---
@@ -82,6 +84,16 @@
.identifiesControllerService(DBCPService.class)
.build();
+ public static final PropertyDescriptor SQL_PRE_QUERY = new PropertyDescriptor.Builder()
+ .name("sql-pre-query")
+ .displayName("SQL pre-query")
--- End diff --
Hi Peter, thanks for the feedback.
I intentionally type it in 'SQL pre-query' to align with SQL_SELECT_QUERY property displayName 'SQL select query'.
For example currently,
SQL_PRE_QUERY, display '**SQL pre-query**'
SQL_SELECT_QUERY, display '**SQL select query**'
SQL_POST_QUERY, display '**SQL post-query**'
If modified as you mention (I actually try this version in the first try)
For example,
SQL_PRE_QUERY, display '**SQL Pre-Query**'
SQL_SELECT_QUERY, display '**SQL select query**'
SQL_POST_QUERY, display '**SQL Post-Query**'
Please let me know your though on this alignment, thanks
---
[GitHub] nifi pull request #3156: NIFI-5780 Add pre and post statements to ExecuteSQL...
Posted by patricker <gi...@git.apache.org>.
Github user patricker commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3156#discussion_r232072719
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/AbstractExecuteSQL.java ---
@@ -82,6 +84,16 @@
.identifiesControllerService(DBCPService.class)
.build();
+ public static final PropertyDescriptor SQL_PRE_QUERY = new PropertyDescriptor.Builder()
+ .name("sql-pre-query")
+ .displayName("SQL Pre-Query")
+ .description("SQL pre-query to execute. Semicolon-delimited list of queries. "
--- End diff --
@colindean I understand why it was done this way. This functionality is being ported from the HIVE processor where the description begins: `HiveQL pre-query to execute...`. The same for the `Note` comment below.
That doesn't mean it couldn't be clarified, but it is currently internally consistent.
@yjhyjhyjh0 How about, "A semicolon-delimited list of queries executed executed before the main SQL query is executed. Results/outputs from these queries will be suppressed if there are no errors."
---
[GitHub] nifi pull request #3156: NIFI-5780 Add pre and post statements to ExecuteSQL...
Posted by colindean <gi...@git.apache.org>.
Github user colindean commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3156#discussion_r231993492
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/AbstractExecuteSQL.java ---
@@ -82,6 +84,16 @@
.identifiesControllerService(DBCPService.class)
.build();
+ public static final PropertyDescriptor SQL_PRE_QUERY = new PropertyDescriptor.Builder()
+ .name("sql-pre-query")
+ .displayName("SQL Pre-Query")
+ .description("SQL pre-query to execute. Semicolon-delimited list of queries. "
+ + "Note, the results/outputs of these queries will be suppressed if successfully executed.")
--- End diff --
You can probably omit "Note," here.
---
[GitHub] nifi pull request #3156: NIFI-5780 Add pre and post statements to ExecuteSQL...
Posted by colindean <gi...@git.apache.org>.
Github user colindean commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3156#discussion_r231993705
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/AbstractExecuteSQL.java ---
@@ -94,6 +106,16 @@
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
.build();
+ public static final PropertyDescriptor SQL_POST_QUERY = new PropertyDescriptor.Builder()
+ .name("sql-post-query")
+ .displayName("SQL Post-Query")
+ .description("SQL post-query to execute. Semicolon-delimited list of queries. "
--- End diff --
Likewise, this description is duplicative with the name. "Queries that will be executed after the intended query."
---
[GitHub] nifi pull request #3156: NIFI-5780 Add pre and post statements to ExecuteSQL...
Posted by patricker <gi...@git.apache.org>.
Github user patricker commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3156#discussion_r231596320
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/AbstractExecuteSQL.java ---
@@ -82,6 +84,16 @@
.identifiesControllerService(DBCPService.class)
.build();
+ public static final PropertyDescriptor SQL_PRE_QUERY = new PropertyDescriptor.Builder()
+ .name("sql-pre-query")
+ .displayName("SQL pre-query")
--- End diff --
Can you make this `SQL Pre-Query`? Same for `SQL Post-Query`.
---
[GitHub] nifi pull request #3156: NIFI-5780 Add pre and post statements to ExecuteSQL...
Posted by yjhyjhyjh0 <gi...@git.apache.org>.
Github user yjhyjhyjh0 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3156#discussion_r232293044
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/AbstractExecuteSQL.java ---
@@ -82,6 +84,16 @@
.identifiesControllerService(DBCPService.class)
.build();
+ public static final PropertyDescriptor SQL_PRE_QUERY = new PropertyDescriptor.Builder()
+ .name("sql-pre-query")
+ .displayName("SQL Pre-Query")
+ .description("SQL pre-query to execute. Semicolon-delimited list of queries. "
+ + "Note, the results/outputs of these queries will be suppressed if successfully executed.")
--- End diff --
Thanks for the suggestion, will omit it at next commit.
---
[GitHub] nifi pull request #3156: NIFI-5780 Add pre and post statements to ExecuteSQL...
Posted by colindean <gi...@git.apache.org>.
Github user colindean commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3156#discussion_r231993409
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/AbstractExecuteSQL.java ---
@@ -82,6 +84,16 @@
.identifiesControllerService(DBCPService.class)
.build();
+ public static final PropertyDescriptor SQL_PRE_QUERY = new PropertyDescriptor.Builder()
+ .name("sql-pre-query")
+ .displayName("SQL Pre-Query")
+ .description("SQL pre-query to execute. Semicolon-delimited list of queries. "
--- End diff --
"SQL pre-query to execute" is duplicative with the name. Can you explain in another way its intent? Perhaps something like, "Queries that will be executed before each intended query is executed."
---