You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/01/12 15:37:40 UTC
[GitHub] [nifi-minifi-cpp] adamdebreceni opened a new pull request #1238: MINIFICPP-1718 - Handle case sensitive column names
adamdebreceni opened a new pull request #1238:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1238
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
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 MINIFICPP-XXXX where XXXX 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 main)?
- [ ] Is your initial contribution a single, squashed commit?
### For code 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?
- [ ] If applicable, have you updated the NOTICE file?
### 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 GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1238: MINIFICPP-1718 - Handle case sensitive column names
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1238:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1238#discussion_r783759484
##########
File path: docker/test/integration/features/sql.feature
##########
@@ -32,6 +32,21 @@ Feature: Executing SQL operations from MiNiFi-C++
When all instances start up
Then at least one flowfile with the content '[{"int_col":2,"text_col":"banana"},{"int_col":1,"text_col":"apple"}]' is placed in the monitored directory in less than 120 seconds
+ Scenario: A MiNiFi instance can query to test table containing mixed case column names with ExecuteSQL processor
+ Given a GenerateFlowFile processor with the "File Size" property set to "0B"
+ And a UpdateAttribute processor with the "sql.args.1.value" property set to "ApPlE"
+ And the "sql.args.2.value" property of the UpdateAttribute processor is set to "BaNaNa"
+ And a ExecuteSQL processor with the "SQL select query" property set to "SELECT * FROM test_table2 WHERE "tExT_Col" = ? OR "tExT_Col" = ? ORDER BY int_col DESC"
+ And the "Output Format" property of the ExecuteSQL processor is set to "JSON"
+ And a PutFile processor with the "Directory" property set to "/tmp/output"
+ And the "success" relationship of the GenerateFlowFile processor is connected to the UpdateAttribute
+ And the "success" relationship of the UpdateAttribute processor is connected to the ExecuteSQL
+ And the "success" relationship of the ExecuteSQL processor is connected to the PutFile
+ And an ODBCService is setup up for ExecuteSQL with the name "ODBCService" and connection string "Driver={PostgreSQL ANSI};Server=postgresql-server;Port=5432;Database=postgres;Uid=postgres;Pwd=password;"
Review comment:
This step could be refactored to have the name and the connection string hardcoded in the step implementation. I don't think it's going to change in any use case and we can remove the duplication this way.
##########
File path: docker/test/integration/features/sql.feature
##########
@@ -32,6 +32,21 @@ Feature: Executing SQL operations from MiNiFi-C++
When all instances start up
Then at least one flowfile with the content '[{"int_col":2,"text_col":"banana"},{"int_col":1,"text_col":"apple"}]' is placed in the monitored directory in less than 120 seconds
+ Scenario: A MiNiFi instance can query to test table containing mixed case column names with ExecuteSQL processor
+ Given a GenerateFlowFile processor with the "File Size" property set to "0B"
+ And a UpdateAttribute processor with the "sql.args.1.value" property set to "ApPlE"
+ And the "sql.args.2.value" property of the UpdateAttribute processor is set to "BaNaNa"
+ And a ExecuteSQL processor with the "SQL select query" property set to "SELECT * FROM test_table2 WHERE "tExT_Col" = ? OR "tExT_Col" = ? ORDER BY int_col DESC"
Review comment:
We could add a comment that PostgreSQL's quoted columns mark case sensitive queries just to be clear.
##########
File path: extensions/sql/data/SQLIdentifier.cpp
##########
@@ -16,30 +16,26 @@
* limitations under the License.
*/
-#include "Utils.h"
+#include "SQLIdentifier.h"
-#include <vector>
-#include <string>
+namespace org::apache::nifi::minifi::sql {
-#include "utils/StringUtils.h"
+SQLIdentifier::SQLIdentifier(std::string str) {
Review comment:
As it is a self-contained entity I would add some simple unit tests for it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1238: MINIFICPP-1718 - Handle case sensitive column names
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1238:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1238#discussion_r783815264
##########
File path: extensions/sql/data/SQLIdentifier.h
##########
@@ -19,18 +19,40 @@
#pragma once
#include <string>
-#include <vector>
+#include <functional>
-namespace org {
-namespace apache {
-namespace nifi {
-namespace minifi {
-namespace utils {
+namespace org::apache::nifi::minifi::sql {
-std::vector<std::string> inputStringToList(const std::string& str);
+class SQLIdentifier {
Review comment:
as for the capitalization, all other SQL entities in this extension have all-caps "SQL" in them, so I would stick to it for consistency
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1238: MINIFICPP-1718 - Handle case sensitive column names
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1238:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1238#discussion_r783248145
##########
File path: extensions/sql/data/SQLIdentifier.h
##########
@@ -19,18 +19,40 @@
#pragma once
#include <string>
-#include <vector>
+#include <functional>
-namespace org {
-namespace apache {
-namespace nifi {
-namespace minifi {
-namespace utils {
+namespace org::apache::nifi::minifi::sql {
-std::vector<std::string> inputStringToList(const std::string& str);
+class SQLIdentifier {
Review comment:
I suggest renaming this to something like `SqlColumnName` to avoid confusion. "Identifier" typically refers to a primary key in the relational database context. "Column" is not the most precise term (that would be "attribute"), but it's the most intuitive.
And I suggest avoiding full-caps acronyms in CamelCase, because it makes it more difficult to determine word boundaries (e.g. `IXMLHTTPRequest`) and to programmatically convert between conventions (e.g. `s_q_l_identifier` instead of `sql_identifier`). It's also recommended in the [google style guide](https://google.github.io/styleguide/cppguide.html#General_Naming_Rules).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1238: MINIFICPP-1718 - Handle case sensitive column names
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1238:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1238#discussion_r783814194
##########
File path: extensions/sql/data/SQLIdentifier.h
##########
@@ -19,18 +19,40 @@
#pragma once
#include <string>
-#include <vector>
+#include <functional>
-namespace org {
-namespace apache {
-namespace nifi {
-namespace minifi {
-namespace utils {
+namespace org::apache::nifi::minifi::sql {
-std::vector<std::string> inputStringToList(const std::string& str);
+class SQLIdentifier {
Review comment:
a quick googling seems to suggest [otherwise](https://www.google.com/search?q=sql+identifier)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1238: MINIFICPP-1718 - Handle case sensitive column names
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1238:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1238#discussion_r783838662
##########
File path: extensions/sql/data/SQLIdentifier.h
##########
@@ -19,18 +19,40 @@
#pragma once
#include <string>
-#include <vector>
+#include <functional>
-namespace org {
-namespace apache {
-namespace nifi {
-namespace minifi {
-namespace utils {
+namespace org::apache::nifi::minifi::sql {
-std::vector<std::string> inputStringToList(const std::string& str);
+class SQLIdentifier {
Review comment:
I think we can constrain it to columns for now, renamed it to `SQLColumnIdentifier` (if we need to use something like this for tables/others we can revisit this)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1238: MINIFICPP-1718 - Handle case sensitive column names
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1238:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1238#discussion_r783767943
##########
File path: docker/test/integration/features/sql.feature
##########
@@ -32,6 +32,21 @@ Feature: Executing SQL operations from MiNiFi-C++
When all instances start up
Then at least one flowfile with the content '[{"int_col":2,"text_col":"banana"},{"int_col":1,"text_col":"apple"}]' is placed in the monitored directory in less than 120 seconds
+ Scenario: A MiNiFi instance can query to test table containing mixed case column names with ExecuteSQL processor
+ Given a GenerateFlowFile processor with the "File Size" property set to "0B"
+ And a UpdateAttribute processor with the "sql.args.1.value" property set to "ApPlE"
+ And the "sql.args.2.value" property of the UpdateAttribute processor is set to "BaNaNa"
+ And a ExecuteSQL processor with the "SQL select query" property set to "SELECT * FROM test_table2 WHERE "tExT_Col" = ? OR "tExT_Col" = ? ORDER BY int_col DESC"
Review comment:
We could add a comment that PostgreSQL's quoted columns marking case sensitive queries just to be clear.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1238: MINIFICPP-1718 - Handle case sensitive column names
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1238:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1238#discussion_r783820974
##########
File path: extensions/sql/data/SQLIdentifier.h
##########
@@ -19,18 +19,40 @@
#pragma once
#include <string>
-#include <vector>
+#include <functional>
-namespace org {
-namespace apache {
-namespace nifi {
-namespace minifi {
-namespace utils {
+namespace org::apache::nifi::minifi::sql {
-std::vector<std::string> inputStringToList(const std::string& str);
+class SQLIdentifier {
Review comment:
fair. What do you think about `SQLColumnIdentifier`? Or do you intend this to be applicable to table and other identifiers as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1238: MINIFICPP-1718 - Handle case sensitive column names
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1238:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1238#discussion_r783834127
##########
File path: extensions/sql/data/SQLIdentifier.cpp
##########
@@ -16,30 +16,26 @@
* limitations under the License.
*/
-#include "Utils.h"
+#include "SQLIdentifier.h"
-#include <vector>
-#include <string>
+namespace org::apache::nifi::minifi::sql {
-#include "utils/StringUtils.h"
+SQLIdentifier::SQLIdentifier(std::string str) {
Review comment:
added unit tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits closed pull request #1238: MINIFICPP-1718 - Handle case sensitive column names
Posted by GitBox <gi...@apache.org>.
fgerlits closed pull request #1238:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1238
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1238: MINIFICPP-1718 - Handle case sensitive column names
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1238:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1238#discussion_r783833399
##########
File path: docker/test/integration/features/sql.feature
##########
@@ -32,6 +32,21 @@ Feature: Executing SQL operations from MiNiFi-C++
When all instances start up
Then at least one flowfile with the content '[{"int_col":2,"text_col":"banana"},{"int_col":1,"text_col":"apple"}]' is placed in the monitored directory in less than 120 seconds
+ Scenario: A MiNiFi instance can query to test table containing mixed case column names with ExecuteSQL processor
+ Given a GenerateFlowFile processor with the "File Size" property set to "0B"
+ And a UpdateAttribute processor with the "sql.args.1.value" property set to "ApPlE"
+ And the "sql.args.2.value" property of the UpdateAttribute processor is set to "BaNaNa"
+ And a ExecuteSQL processor with the "SQL select query" property set to "SELECT * FROM test_table2 WHERE "tExT_Col" = ? OR "tExT_Col" = ? ORDER BY int_col DESC"
+ And the "Output Format" property of the ExecuteSQL processor is set to "JSON"
+ And a PutFile processor with the "Directory" property set to "/tmp/output"
+ And the "success" relationship of the GenerateFlowFile processor is connected to the UpdateAttribute
+ And the "success" relationship of the UpdateAttribute processor is connected to the ExecuteSQL
+ And the "success" relationship of the ExecuteSQL processor is connected to the PutFile
+ And an ODBCService is setup up for ExecuteSQL with the name "ODBCService" and connection string "Driver={PostgreSQL ANSI};Server=postgresql-server;Port=5432;Database=postgres;Uid=postgres;Pwd=password;"
Review comment:
moved the string into the step
##########
File path: docker/test/integration/features/sql.feature
##########
@@ -32,6 +32,21 @@ Feature: Executing SQL operations from MiNiFi-C++
When all instances start up
Then at least one flowfile with the content '[{"int_col":2,"text_col":"banana"},{"int_col":1,"text_col":"apple"}]' is placed in the monitored directory in less than 120 seconds
+ Scenario: A MiNiFi instance can query to test table containing mixed case column names with ExecuteSQL processor
+ Given a GenerateFlowFile processor with the "File Size" property set to "0B"
+ And a UpdateAttribute processor with the "sql.args.1.value" property set to "ApPlE"
+ And the "sql.args.2.value" property of the UpdateAttribute processor is set to "BaNaNa"
+ And a ExecuteSQL processor with the "SQL select query" property set to "SELECT * FROM test_table2 WHERE "tExT_Col" = ? OR "tExT_Col" = ? ORDER BY int_col DESC"
Review comment:
added a comment
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org