[GitHub] nifi pull request #2625: NIFI-5049 Fix handling of Phonenix datetime columns

[gardellajuanpablo]

Github user gardellajuanpablo commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2625#discussion_r189665750
  
--- Diff: 
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/AbstractDatabaseFetchProcessor.java
 ---
@@ -30,6 +30,7 @@
 import org.apache.nifi.processor.exception.ProcessException;
 import org.apache.nifi.processor.util.StandardValidators;
 import org.apache.nifi.processors.standard.db.DatabaseAdapter;
+import org.apache.nifi.processors.standard.db.impl.PhoenixDatabaseAdapter;
--- End diff --

Sounds good!


---

[GitHub] nifi pull request #2625: NIFI-5049 Fix handling of Phonenix datetime columns

[mattyb149]

Github user mattyb149 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2625#discussion_r189662885
  
--- Diff: 
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/AbstractDatabaseFetchProcessor.java
 ---
@@ -30,6 +30,7 @@
 import org.apache.nifi.processor.exception.ProcessException;
 import org.apache.nifi.processor.util.StandardValidators;
 import org.apache.nifi.processors.standard.db.DatabaseAdapter;
+import org.apache.nifi.processors.standard.db.impl.PhoenixDatabaseAdapter;
--- End diff --

We may end up moving the abstract class and interface(s) out to its own NAR 
so other folks can implement DB adapters outside the standard NAR. In that case 
we'd want to replace this with something like the contains() we did for Oracle. 
Also if there were a need for 2 separate Phoenix adapters, we'd have to do that 
anyway... 


---

[GitHub] nifi pull request #2625: NIFI-5049 Fix handling of Phonenix datetime columns

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/nifi/pull/2625


---

[GitHub] nifi pull request #2625: NIFI-5049 Fix handling of Phonenix datetime columns

[gardellajuanpablo]

Github user gardellajuanpablo commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2625#discussion_r180787552
  
--- Diff: 
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/db/impl/PhoenixDatabaseAdapter.java
 ---
@@ -0,0 +1,72 @@
+/*
+ * 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.nifi.processors.standard.db.impl;
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.nifi.processors.standard.db.DatabaseAdapter;
+
+/**
+ * A Apache Phoenix database adapter that generates ANSI SQL.
+ */
+public final class PhoenixDatabaseAdapter implements DatabaseAdapter {
+public static final String NAME = "Phoenix";
+
+@Override
+public String getName() {
+return NAME;
+}
+
+@Override
+public String getDescription() {
+return "Generates Phoenix compliant SQL";
+}
+
+@Override
+public String getSelectStatement(String tableName, String columnNames, 
String whereClause, String orderByClause,
--- End diff --

Yes, tt will prevent duplicate code, but any change introduced in 
GenericDatabaseAdapter may break Phoenix adapter. Also PhoenixDatabaseAdapter 
it is not (my personal opinion) a Generic Adapter. Having in this way, it's 
less risk introducing changes in Generic/Phoenix Adapter. 


---

[GitHub] nifi pull request #2625: NIFI-5049 Fix handling of Phonenix datetime columns

[gardellajuanpablo]

Github user gardellajuanpablo commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2625#discussion_r180786540
  
--- Diff: 
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/AbstractDatabaseFetchProcessor.java
 ---
@@ -436,13 +437,26 @@ protected static String getLiteralByType(int type, 
String value, String database
 case NVARCHAR:
 case VARCHAR:
 case ROWID:
-case DATE:
-case TIME:
 return "'" + value + "'";
+case TIME:
+if (PhoenixDatabaseAdapter.NAME.equals(databaseType)) {
+return "time '" + value + "'";
+}
+case DATE:
 case TIMESTAMP:
-if (!StringUtils.isEmpty(databaseType) && 
databaseType.contains("Oracle")) {
-// For backwards compatibility, the type might be 
TIMESTAMP but the state value is in DATE format. This should be a one-time 
occurrence as the next maximum value
-// should be stored as a full timestamp. Even so, 
check to see if the value is missing time-of-day information, and use the 
"date" coercion rather than the
+// TODO delegate to database adapter the conversion 
instead of using if in this
--- End diff --

Yes, but it will break the method signature (it is not private). I prefer 
to keep it compatible in this bug fixing. Maybe it's better to file a 
IMPROVEMENT to improve how the DB specific behavior is handling. 


---

[GitHub] nifi pull request #2625: NIFI-5049 Fix handling of Phonenix datetime columns

[mattyb149]

Github user mattyb149 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2625#discussion_r180766269
  
--- Diff: 
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/db/impl/PhoenixDatabaseAdapter.java
 ---
@@ -0,0 +1,72 @@
+/*
+ * 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.nifi.processors.standard.db.impl;
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.nifi.processors.standard.db.DatabaseAdapter;
+
+/**
+ * A Apache Phoenix database adapter that generates ANSI SQL.
+ */
+public final class PhoenixDatabaseAdapter implements DatabaseAdapter {
+public static final String NAME = "Phoenix";
+
+@Override
+public String getName() {
+return NAME;
+}
+
+@Override
+public String getDescription() {
+return "Generates Phoenix compliant SQL";
+}
+
+@Override
+public String getSelectStatement(String tableName, String columnNames, 
String whereClause, String orderByClause,
--- End diff --

Is this different from the GenericDatabaseAdapter? If not you can just 
extend GenericDatabaseAdapter and only override getName() and getDescription(), 
so you don't need duplicate code.


---

[GitHub] nifi pull request #2625: NIFI-5049 Fix handling of Phonenix datetime columns

[mattyb149]

Github user mattyb149 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2625#discussion_r180767606
  
--- Diff: 
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/AbstractDatabaseFetchProcessor.java
 ---
@@ -436,13 +437,26 @@ protected static String getLiteralByType(int type, 
String value, String database
 case NVARCHAR:
 case VARCHAR:
 case ROWID:
-case DATE:
-case TIME:
 return "'" + value + "'";
+case TIME:
+if (PhoenixDatabaseAdapter.NAME.equals(databaseType)) {
+return "time '" + value + "'";
+}
+case DATE:
 case TIMESTAMP:
-if (!StringUtils.isEmpty(databaseType) && 
databaseType.contains("Oracle")) {
-// For backwards compatibility, the type might be 
TIMESTAMP but the state value is in DATE format. This should be a one-time 
occurrence as the next maximum value
-// should be stored as a full timestamp. Even so, 
check to see if the value is missing time-of-day information, and use the 
"date" coercion rather than the
+// TODO delegate to database adapter the conversion 
instead of using if in this
--- End diff --

This might be a good time to do the TODO :) We can add a default method on 
the interface, getTimestampLiteral() or something, that returns the quoted 
value, and put the if/else clause here in the Oracle and Phoenix adapters.
Also, I assume this same code works for Phoenix, even though the Jira 
mentions TO_TIMESTAMP rather than just timestamp?


---

[GitHub] nifi pull request #2625: NIFI-5049 Fix handling of Phonenix datetime columns

[mattyb149]

Github user mattyb149 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2625#discussion_r180767978
  
--- Diff: 
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/AbstractDatabaseFetchProcessor.java
 ---
@@ -436,13 +437,26 @@ protected static String getLiteralByType(int type, 
String value, String database
 case NVARCHAR:
 case VARCHAR:
 case ROWID:
-case DATE:
-case TIME:
 return "'" + value + "'";
+case TIME:
+if (PhoenixDatabaseAdapter.NAME.equals(databaseType)) {
+return "time '" + value + "'";
+}
+case DATE:
 case TIMESTAMP:
-if (!StringUtils.isEmpty(databaseType) && 
databaseType.contains("Oracle")) {
-// For backwards compatibility, the type might be 
TIMESTAMP but the state value is in DATE format. This should be a one-time 
occurrence as the next maximum value
-// should be stored as a full timestamp. Even so, 
check to see if the value is missing time-of-day information, and use the 
"date" coercion rather than the
+// TODO delegate to database adapter the conversion 
instead of using if in this
--- End diff --

This might be a good time to do the TODO :) We can add a default method on 
the interface, getTimestampLiteral() or something, that just returns the quoted 
value, and we can override it in the Oracle and Phoenix adapters.


---

[GitHub] nifi pull request #2625: NIFI-5049 Fix handling of Phonenix datetime columns

[mattyb149]

Github user mattyb149 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2625#discussion_r180768758
  
--- Diff: 
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/AbstractDatabaseFetchProcessor.java
 ---
@@ -436,13 +437,26 @@ protected static String getLiteralByType(int type, 
String value, String database
 case NVARCHAR:
 case VARCHAR:
 case ROWID:
-case DATE:
-case TIME:
 return "'" + value + "'";
+case TIME:
--- End diff --

See my comment below, perhaps we'd have three methods then, 
getDateLiteral(), getTimeLiteral(), and getTimestampLiteral().


---

[GitHub] nifi pull request #2625: NIFI-5049 Fix handling of Phonenix datetime columns

[gardellajuanpablo]

Github user gardellajuanpablo commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2625#discussion_r180747757
  
--- Diff: 
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/AbstractDatabaseFetchProcessor.java
 ---
@@ -428,33 +429,44 @@ protected static String getMaxValueFromRow(ResultSet 
resultSet,
 protected static String getLiteralByType(int type, String value, 
String databaseType) {
 // Format value based on column type. For example, strings and 
timestamps need to be quoted
 switch (type) {
-// For string-represented values, put in single quotes
-case CHAR:
-case LONGNVARCHAR:
-case LONGVARCHAR:
-case NCHAR:
-case NVARCHAR:
-case VARCHAR:
-case ROWID:
-case DATE:
-case TIME:
-return "'" + value + "'";
-case TIMESTAMP:
-if (!StringUtils.isEmpty(databaseType) && 
databaseType.contains("Oracle")) {
-// For backwards compatibility, the type might be 
TIMESTAMP but the state value is in DATE format. This should be a one-time 
occurrence as the next maximum value
-// should be stored as a full timestamp. Even so, 
check to see if the value is missing time-of-day information, and use the 
"date" coercion rather than the
-// "timestamp" coercion in that case
-if (value.matches("\\d{4}-\\d{2}-\\d{2}")) {
-return "date '" + value + "'";
-} else {
-return "timestamp '" + value + "'";
-}
+// For string-represented values, put in single quotes
--- End diff --

done


---

[GitHub] nifi pull request #2625: NIFI-5049 Fix handling of Phonenix datetime columns

[gardellajuanpablo]

Github user gardellajuanpablo commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2625#discussion_r180747704
  
--- Diff: 
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/QueryDatabaseTable.java
 ---
@@ -303,9 +303,8 @@ public void onTrigger(final ProcessContext context, 
final ProcessSessionFactory
 
 final Integer queryTimeout = 
context.getProperty(QUERY_TIMEOUT).evaluateAttributeExpressions().asTimePeriod(TimeUnit.SECONDS).intValue();
 st.setQueryTimeout(queryTimeout); // timeout in seconds
-try {
-logger.debug("Executing query {}", new 
Object[]{selectQuery});
-final ResultSet resultSet = st.executeQuery(selectQuery);
+logger.debug("Executing query {}", new Object[]{selectQuery});
--- End diff --

done


---

[GitHub] nifi pull request #2625: NIFI-5049 Fix handling of Phonenix datetime columns

[gardellajuanpablo]

Github user gardellajuanpablo commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2625#discussion_r180747659
  
--- Diff: 
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/QueryDatabaseTableTest.java
 ---
@@ -159,6 +160,35 @@ public void testGetQuery() throws Exception {
 dbAdapter = new OracleDatabaseAdapter();
 query = processor.getQuery(dbAdapter, "myTable", null, 
Arrays.asList("id", "DATE_CREATED"), "type = \"CUSTOMER\"", 
stateManager.getState(Scope.CLUSTER).toMap());
 assertEquals("SELECT * FROM myTable WHERE id > 509 AND 
DATE_CREATED >= timestamp '2016-03-07 12:34:56' AND (type = \"CUSTOMER\")", 
query);
+
--- End diff --

done


---

[GitHub] nifi pull request #2625: NIFI-5049 Fix handling of Phonenix datetime columns

[MikeThomsen]

Github user MikeThomsen commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2625#discussion_r180727687
  
--- Diff: 
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/AbstractDatabaseFetchProcessor.java
 ---
@@ -428,33 +429,44 @@ protected static String getMaxValueFromRow(ResultSet 
resultSet,
 protected static String getLiteralByType(int type, String value, 
String databaseType) {
 // Format value based on column type. For example, strings and 
timestamps need to be quoted
 switch (type) {
-// For string-represented values, put in single quotes
-case CHAR:
-case LONGNVARCHAR:
-case LONGVARCHAR:
-case NCHAR:
-case NVARCHAR:
-case VARCHAR:
-case ROWID:
-case DATE:
-case TIME:
-return "'" + value + "'";
-case TIMESTAMP:
-if (!StringUtils.isEmpty(databaseType) && 
databaseType.contains("Oracle")) {
-// For backwards compatibility, the type might be 
TIMESTAMP but the state value is in DATE format. This should be a one-time 
occurrence as the next maximum value
-// should be stored as a full timestamp. Even so, 
check to see if the value is missing time-of-day information, and use the 
"date" coercion rather than the
-// "timestamp" coercion in that case
-if (value.matches("\\d{4}-\\d{2}-\\d{2}")) {
-return "date '" + value + "'";
-} else {
-return "timestamp '" + value + "'";
-}
+// For string-represented values, put in single quotes
--- End diff --

Please restore the original indentation level.


---

[GitHub] nifi pull request #2625: NIFI-5049 Fix handling of Phonenix datetime columns

[MikeThomsen]

Github user MikeThomsen commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2625#discussion_r180728070
  
--- Diff: 
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/QueryDatabaseTable.java
 ---
@@ -303,9 +303,8 @@ public void onTrigger(final ProcessContext context, 
final ProcessSessionFactory
 
 final Integer queryTimeout = 
context.getProperty(QUERY_TIMEOUT).evaluateAttributeExpressions().asTimePeriod(TimeUnit.SECONDS).intValue();
 st.setQueryTimeout(queryTimeout); // timeout in seconds
-try {
-logger.debug("Executing query {}", new 
Object[]{selectQuery});
-final ResultSet resultSet = st.executeQuery(selectQuery);
+logger.debug("Executing query {}", new Object[]{selectQuery});
--- End diff --

Should be wrapped with `if (logger.isDebugEnabled()) {}`


---

[GitHub] nifi pull request #2625: NIFI-5049 Fix handling of Phonenix datetime columns

[MikeThomsen]

Github user MikeThomsen commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2625#discussion_r180732541
  
--- Diff: 
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/QueryDatabaseTableTest.java
 ---
@@ -159,6 +160,35 @@ public void testGetQuery() throws Exception {
 dbAdapter = new OracleDatabaseAdapter();
 query = processor.getQuery(dbAdapter, "myTable", null, 
Arrays.asList("id", "DATE_CREATED"), "type = \"CUSTOMER\"", 
stateManager.getState(Scope.CLUSTER).toMap());
 assertEquals("SELECT * FROM myTable WHERE id > 509 AND 
DATE_CREATED >= timestamp '2016-03-07 12:34:56' AND (type = \"CUSTOMER\")", 
query);
+
--- End diff --

Can you break this out into a separate unit test? We don't want a failure 
in the Oracle unit test to block the Phoenix test from running.


---

[GitHub] nifi pull request #2625: NIFI-5049 Fix handling of Phonenix datetime columns

[gardellajuanpablo]

GitHub user gardellajuanpablo opened a pull request:

https://github.com/apache/nifi/pull/2625

NIFI-5049 Fix handling of Phonenix datetime columns

Fixed Error: ERROR 203 (22005): Type mismatch. TIMESTAMP
and VARCHAR for  = '2015-09-12 18:19:43.293'
(sample date).

The fix consists on applying the same fix added to Oracle DB,
based on NIFI-2323 approach. The changes introduced were:

1) A Phoenix database adapter.
2) Handling timestamp, date and time in Phoenix.
3) Added a close to result set in QueryDatabaseTable processor
   (not related to this bug, but I think it will be useful to
fix).

The changes don't break existing code.

Thank you for submitting a contribution to Apache NiFi.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced 
 in the commit message?

- [ ] Does your PR title start with NIFI- where  is the JIRA number 
you are trying to resolve? Pay particular attention to the hyphen "-" character.

- [ ] Has your PR been rebased against the latest commit within the target 
branch (typically master)?

- [ ] 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?
- [ ] 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?
- [ ] If adding new Properties, have you added .displayName in addition to 
.name (programmatic access) for each of the new properties?

### For documentation related changes:
- [ ] 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/gardellajuanpablo/nifi NIFI-5049

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/nifi/pull/2625.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 #2625


commit 20f2638e7f792ec84f61ea42f135587e2f30ea1f
Author: Gardella Juan Pablo 
Date:   2018-04-11T04:23:00Z

NIFI-5049 Fix handling of Phonenix datetime columns

Fixed Error: ERROR 203 (22005): Type mismatch. TIMESTAMP
and VARCHAR for  = '2015-09-12 18:19:43.293'
(sample date).

The fix consists on applying the same fix added to Oracle DB,
based on NIFI-2323 approach. The changes introduced were:

1) A Phoenix database adapter.
2) Handling timestamp, date and time in Phoenix.
3) Added a close to result set in QueryDatabaseTable processor
   (not related to this bug, but I think it will be useful to
fix).

The changes don't break existing code.




---