NIFI-6934 In PutDatabaseRecord added Postgres UPSERT support #4350

tpalfy · 2020-06-18T18:02:14Z

https://issues.apache.org/jira/browse/NIFI-6934

Implemented using DatabaseAdapter

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-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 master)?
Is your initial contribution a single, squashed commit? Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not squash or use --force when pushing to allow for clean monitoring of changes.

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?
Have you verified that the full build is successful on JDK 8?
Have you verified that the full build is successful on JDK 11?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
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 GitHub Actions CI for build issues and submit an update to your PR as soon as possible.

...standard-processors/src/main/java/org/apache/nifi/processors/standard/PutDatabaseRecord.java

mattyb149 · 2020-06-22T17:18:05Z

Looking and working good, just some comments on extending the documentation to make things as clear as possible, should be good to go after that

simonbence · 2020-06-23T07:12:42Z

...standard-processors/src/main/java/org/apache/nifi/processors/standard/PutDatabaseRecord.java

@@ -101,6 +106,7 @@
    static final String UPDATE_TYPE = "UPDATE";
    static final String INSERT_TYPE = "INSERT";
    static final String DELETE_TYPE = "DELETE";
+    static final String UPSERT_TYPE = "UPSERT";


Minor: I think this is not a really widely known terminology. It might worth to think about UPDATE_OR_INSERT for better readability. (at least in variable name)

UPSERT is a fairly widespread expression, although not a standard.

The standard would be MERGE but many RDBMS - Postgres for one - doesn't support it. (And it's a more than 10 year old standard so I guess we'll just have to live without it.)

Okay, let's got with "UPDATE_OR_INSERT" to describe the concept.

I would vote for the original UPSERT but don't want to veto the change.

I'll keep it as "UPSERT" after all. It's sufficiently understandable (the expression is widely used), simpler and don't want to rename supportsUpsert to something like supportsInsertOrUpdate.

simonbence · 2020-06-23T08:22:12Z

...ors/src/main/java/org/apache/nifi/processors/standard/db/impl/PostgreSQLDatabaseAdapter.java

+            .map(__ -> "?")
+            .collect(Collectors.joining(", "));
+
+        String updateValues = columnNames.stream()


Minor: updateColumns?

But these are values for the update.

simonbence · 2020-06-23T08:23:30Z

...ors/src/main/java/org/apache/nifi/processors/standard/db/impl/PostgreSQLDatabaseAdapter.java

@@ -0,0 +1,68 @@
+/*


Could you please add a test like TestMSSQLDatabaseAdapter?

simonbence · 2020-06-23T08:25:28Z

...standard-processors/src/main/java/org/apache/nifi/processors/standard/PutDatabaseRecord.java

@@ -29,6 +29,8 @@
 import org.apache.nifi.annotation.lifecycle.OnScheduled;


Could you please extend TestPutDatabaseRecord with relevant case?

Unfortunately we will have to settle for manual testing.
TestPutDatabaseRecord is not suitable for testing non-standard functionality (it uses derby).

mattyb149 · 2020-07-07T14:40:22Z

Can you please change the base branch of this PR to main and rebase against the latest main branch? That's the one we'll be using from now on. Thanks in advance!

…port for Postgres (9.5+)

mattyb149 · 2020-07-08T18:20:38Z

+1 LGTM, ran contrib-check and tested a few scenarios. @simonbence @turcsanyip Are your reviews complete? If so I can merge to main. Thanks for this improvement! It will also make it easier to do DB-specific things now that we have the Database Type property in there.

simonbence · 2020-07-09T12:05:35Z

I got answer for my questions, thanks! LGTM

turcsanyip · 2020-07-09T12:49:27Z

LGTM too.
@mattyb149 It can be merged from my side.

…port for Postgres (9.5+) NIFI-6934 Added more documentation and unit tests. NIFI-6934 Added missing license for new test class. Signed-off-by: Matthew Burgess <mattyb149@apache.org> This closes apache#4350

mattyb149 reviewed Jun 22, 2020

View reviewed changes

...standard-processors/src/main/java/org/apache/nifi/processors/standard/PutDatabaseRecord.java Show resolved Hide resolved

simonbence reviewed Jun 23, 2020

View reviewed changes

tpalfy requested review from mattyb149 and turcsanyip June 29, 2020 15:36

tpalfy added 3 commits July 7, 2020 19:56

NIFI-6934 In PutDatabaseRecord added DatabaseAdapter-based UPSERT sup…

fbaf3e8

…port for Postgres (9.5+)

NIFI-6934 Added more documentation and unit tests.

482a912

NIFI-6934 Added missing license for new test class.

85cede9

tpalfy force-pushed the NIFI-6934 branch from 70f19be to 85cede9 Compare July 7, 2020 17:58

tpalfy mentioned this pull request Jul 7, 2020

NIFI-6934 In PutDatabaseRecord added Postgres UPSERT support #4390

Closed

13 tasks

tpalfy changed the base branch from master to main July 7, 2020 18:13

mattyb149 closed this in ad95287 Jul 9, 2020

pvillard31 mentioned this pull request Feb 25, 2021

NIFI-6934: Add support for Postgres flavour of UPSERT action #3951

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NIFI-6934 In PutDatabaseRecord added Postgres UPSERT support #4350

NIFI-6934 In PutDatabaseRecord added Postgres UPSERT support #4350

tpalfy commented Jun 18, 2020 •

edited

mattyb149 commented Jun 22, 2020

simonbence Jun 23, 2020

tpalfy Jun 23, 2020

turcsanyip Jun 23, 2020

tpalfy Jun 23, 2020

simonbence Jun 23, 2020

tpalfy Jun 23, 2020

simonbence Jun 23, 2020

simonbence Jun 23, 2020

tpalfy Jun 23, 2020 •

edited

mattyb149 commented Jul 7, 2020

mattyb149 commented Jul 8, 2020

simonbence commented Jul 9, 2020

turcsanyip commented Jul 9, 2020

		@@ -29,6 +29,8 @@
		import org.apache.nifi.annotation.lifecycle.OnScheduled;

NIFI-6934 In PutDatabaseRecord added Postgres UPSERT support #4350

NIFI-6934 In PutDatabaseRecord added Postgres UPSERT support #4350

Conversation

tpalfy commented Jun 18, 2020 • edited

For all changes:

For code changes:

For documentation related changes:

Note:

mattyb149 commented Jun 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpalfy Jun 23, 2020 • edited

Choose a reason for hiding this comment

mattyb149 commented Jul 7, 2020

mattyb149 commented Jul 8, 2020

simonbence commented Jul 9, 2020

turcsanyip commented Jul 9, 2020

tpalfy commented Jun 18, 2020 •

edited

tpalfy Jun 23, 2020 •

edited