Re: [PR] NIFI-13126: Add Impala DB Adapter [nifi]
exceptionfactory closed pull request #8728: NIFI-13126: Add Impala DB Adapter URL: https://github.com/apache/nifi/pull/8728 -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] NIFI-13126: Add Impala DB Adapter [nifi]
exceptionfactory commented on PR #8728: URL: https://github.com/apache/nifi/pull/8728#issuecomment-2174020221 Thanks for putting forward the new Controller Service approach in #8892. I'm closing this pull request in favor of those changes, and will look to follow up there. If there turns out to be some blocker in the course of that review, this PR could be revisited. -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] NIFI-13126: Add Impala DB Adapter [nifi]
takraj commented on PR #8728: URL: https://github.com/apache/nifi/pull/8728#issuecomment-2137622470 @mattyb149 @joewitt I have created a PoC / PR implementing the concept of having controller services instead of fixed lists of supported SQL dialects. If we accept this solution, then Impala dialect is not necessarily needed to be included & maintained in the NiFi repository. Please have a look: https://github.com/apache/nifi/pull/8892 -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] NIFI-13126: Add Impala DB Adapter [nifi]
mattyb149 commented on PR #8728: URL: https://github.com/apache/nifi/pull/8728#issuecomment-2123057860 I didn't make them controller services at the time because they just didn't feel like controller services to me, they shouldn't need to be configured separately and just represent specific dialects of SQL. Having said that I'm not opposed to the idea if it makes sense going forward. -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] NIFI-13126: Add Impala DB Adapter [nifi]
takraj commented on PR #8728: URL: https://github.com/apache/nifi/pull/8728#issuecomment-2114122812 @mattyb149 , @joewitt How about doing some refactor *for 2.x*, and extracting these DB adapters as controller services? For 1.x line we could keep the current behavior for compatibility, and add an option to use controller service instead. Eg. the dropdown could have an option like 'Use Controller Service', and when that's selected the user can pick one. Thoughts? -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] NIFI-13126: Add Impala DB Adapter [nifi]
mattyb149 commented on PR #8728: URL: https://github.com/apache/nifi/pull/8728#issuecomment-2113419404 Without Impala dependencies I don't see why it can't go into the standard NAR, especially since `PhoenixDatabaseAdapter` is a precedent. I think there are a number of people in the community (including myself) that can maintain this. Thoughts? -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] NIFI-13126: Add Impala DB Adapter [nifi]
joewitt commented on PR #8728: URL: https://github.com/apache/nifi/pull/8728#issuecomment-2104694695 I am thinking we want to be careful about including this in the standard bundle. Though I notice this doesn't have any apparent relationship to a particular binary/jdbc driver. It does offer a SPI mechanism to find providers. Does that mean we can have this in its own nar which could be optionally included? My concern is how many people in the community will be able to participate/actively engage in maintaining this component. -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] NIFI-13126: Add Impala DB Adapter [nifi]
dan-s1 commented on code in PR #8728:
URL: https://github.com/apache/nifi/pull/8728#discussion_r1595765725
##
nifi-extension-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/db/impl/ImpalaDatabaseAdapter.java:
##
@@ -0,0 +1,116 @@
+/*
+ * 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 java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.stream.Collectors;
+import org.apache.nifi.processors.standard.db.ColumnDescription;
+import org.apache.nifi.util.StringUtils;
+
+/**
+ * A generic database adapter that generates Impala compatible SQL.
+ */
+public class ImpalaDatabaseAdapter extends GenericDatabaseAdapter {
+
+private static final String QUOTE_MARK = "`";
+
+@Override
+public String getName() {
+return "Impala";
+}
+
+@Override
+public String getDescription() {
+return "Generates Impala compatible SQL";
+}
+
+@Override
+public String unwrapIdentifier(String identifier) {
+// Removes double quotes and back-ticks.
+return identifier == null ? null : identifier.replaceAll("[\"`]", "");
+}
+
+@Override
+public boolean supportsUpsert() {
+return true;
+}
+
+@Override
+public String getUpsertStatement(String table, List columnNames,
Collection uniqueKeyColumnNames) {
+if (StringUtils.isEmpty(table)) {
+throw new IllegalArgumentException("Table name cannot be null or
blank");
+}
+if (columnNames == null || columnNames.isEmpty()) {
+throw new IllegalArgumentException("Column names cannot be null or
empty");
+}
+if (uniqueKeyColumnNames == null || uniqueKeyColumnNames.isEmpty()) {
+throw new IllegalArgumentException("Key column names cannot be
null or empty");
+}
+
+final String columns = String.join(", ", columnNames);
+
+final String parameterizedInsertValues = columnNames.stream()
+.map(__ -> "?")
+.collect(Collectors.joining(", "));
+
+return "UPSERT INTO " + table + "(" + columns + ")" + " VALUES " + "("
+ parameterizedInsertValues + ")";
+}
+
+@Override
+public String getTableQuoteString() {
+return QUOTE_MARK;
+}
+
+@Override
+public String getColumnQuoteString() {
+return QUOTE_MARK;
+}
+
+@Override
+public boolean supportsCreateTableIfNotExists() {
+return true;
+}
+
+@Override
+public List getAlterTableStatements(final String tableName, final
List columnsToAdd, final boolean quoteTableName, final
boolean quoteColumnNames) {
+List columnsAndDatatypes = new
ArrayList<>(columnsToAdd.size());
+for (ColumnDescription column : columnsToAdd) {
+String dataType = getSQLForDataType(column.getDataType());
+columnsAndDatatypes.add(
+(quoteColumnNames ? getColumnQuoteString() :
StringUtils.EMPTY)
++ column.getColumnName()
++ (quoteColumnNames ? getColumnQuoteString() :
StringUtils.EMPTY)
++ " "
++ dataType
+);
+}
+
+StringBuilder alterTableStatement = new StringBuilder();
+return Collections.singletonList(
+alterTableStatement.append("ALTER TABLE ")
+.append(quoteTableName ? getTableQuoteString() :
StringUtils.EMPTY)
+.append(tableName)
+.append(quoteTableName ? getTableQuoteString() :
StringUtils.EMPTY)
+.append(" ADD COLUMNS ")
+.append("(").append(String.join(", ",
columnsAndDatatypes)).append(")")
+.toString()
+);
Review Comment:
I believe this can be simplified by using streams to convert from
`ColumnDescription `to `String` and without having to call the tertiary
statements twice to determine whether to quote. (I am sorry for the formatting
of the suggestion as it looks different than what I had typed)
```suggestion
final List
Re: [PR] NIFI-13126: Add Impala DB Adapter [nifi]
dan-s1 commented on code in PR #8728:
URL: https://github.com/apache/nifi/pull/8728#discussion_r1589300753
##
nifi-extension-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/db/impl/ImpalaDatabaseAdapter.java:
##
@@ -0,0 +1,127 @@
+/*
+ * 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 java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.stream.Collectors;
+import org.apache.nifi.processors.standard.db.ColumnDescription;
+import org.apache.nifi.util.StringUtils;
+
+/**
+ * A generic database adapter that generates Impala compatible SQL.
+ */
+public class ImpalaDatabaseAdapter extends GenericDatabaseAdapter {
+@Override
+public String getName() {
+return "Impala";
+}
+
+@Override
+public String getDescription() {
+return "Generates Impala compatible SQL";
+}
+
+@Override
+public String unwrapIdentifier(String identifier) {
+// Removes double quotes and back-ticks.
+return identifier == null ? null : identifier.replaceAll("[\"`]", "");
+}
+
+@Override
+public boolean supportsUpsert() {
+return true;
+}
+
+@Override
+public boolean supportsInsertIgnore() {
+return false;
+}
+
+@Override
+public String getUpsertStatement(String table, List columnNames,
Collection uniqueKeyColumnNames) {
+if (StringUtils.isEmpty(table)) {
+throw new IllegalArgumentException("Table name cannot be null or
blank");
+}
+if (columnNames == null || columnNames.isEmpty()) {
+throw new IllegalArgumentException("Column names cannot be null or
empty");
+}
+if (uniqueKeyColumnNames == null || uniqueKeyColumnNames.isEmpty()) {
+throw new IllegalArgumentException("Key column names cannot be
null or empty");
+}
Review Comment:
```suggestion
if (StringUtils.isEmpty(columnNames) {
throw new IllegalArgumentException("Column names cannot be null
or empty");
}
if (StringUtils.isEmpty(uniqueKeyColumnNames) {
throw new IllegalArgumentException("Key column names cannot be
null or empty");
}
```
##
nifi-extension-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/db/impl/TestImpalaDatabaseAdapter.java:
##
@@ -0,0 +1,104 @@
+/*
+ * 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 static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+public class TestImpalaDatabaseAdapter {
+
+private ImpalaDatabaseAdapter testSubject;
+
+@BeforeEach
+public void setUp() throws Exception {
+testSubject = new ImpalaDatabaseAdapter();
+}
+
+@Test
+public void testSupportsUpsert() {
+assertTrue(testSubject.supportsUpsert(),
testSubject.getClass().getSimpleName() + " should support upsert");
[PR] NIFI-13126: Add Impala DB Adapter [nifi]
takraj opened a new pull request, #8728: URL: https://github.com/apache/nifi/pull/8728 # Summary [NIFI-13126](https://issues.apache.org/jira/browse/NIFI-13126) # Tracking Please complete the following tracking steps prior to pull request creation. ### Issue Tracking - [x] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created ### Pull Request Tracking - [x] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-0` - [x] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-0` ### Pull Request Formatting - [x] Pull Request based on current revision of the `main` branch - [x] Pull Request refers to a feature branch with one commit containing changes # Verification Please indicate the verification steps performed prior to pull request creation. ### Build - [x] Build completed using `mvn clean install -P contrib-check` - [x] JDK 21 ### UI Contributions - [x] NiFi is modernizing its UI. Any contributions that update the [current UI](https://github.com/apache/nifi/tree/main/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui) also need to be implemented in the [new UI](https://github.com/apache/nifi/tree/main/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi). ### Licensing - [x] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html) - [x] New dependencies are documented in applicable `LICENSE` and `NOTICE` files ### Documentation - [x] Documentation formatting appears as expected in rendered files -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
