Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NIFI-12530: Support CREATE TABLE in Oracle database adapters #8175

Closed
wants to merge 2 commits into from

Conversation

mattyb149
Copy link
Contributor

Summary

NIFI-12530 This PR adds support for CREATE TABLE (via UpdateDatabaseRecord) for the two existing Oracle database adapters.

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Pull Request Tracking

  • Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
  • Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

  • Pull Request based on current revision of the main branch
  • 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

  • Build completed using mvn clean install -P contrib-check
    • JDK 21

Licensing

  • New dependencies are compatible with the Apache License 2.0 according to the License Policy
  • New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

  • Documentation formatting appears as expected in rendered files

@EndzeitBegins
Copy link
Contributor

Thank you for working on this @mattyb149. 👍🏻

I just glanced over the code and have two remarks.

Is there a reason, there is no test added for the OracleDatabaseAdapter implementation?

Does it make sense to populate the columns into the StringBuilder directly, instead of putting them into a List and only then joining them into the StringBuilder. I was under the impression that the StringBuilder is especially designed for these kinds of iterative String creations. However, Java is not my primary language, so I might be in the wrong here. 🤷🏻‍♂️

@mattyb149
Copy link
Contributor Author

I didn't add the unit test to the older OracleDatabaseAdapter because it was basically copy/paste, but since the new code is too, I will add that test to it. The only reason for using the List instead of the StringBuilder was for the join(), I felt it awkward to have to use a for loop and add a comma if i != 0. If there are performance-related concerns I can change that as well.

Copy link
Contributor

@EndzeitBegins EndzeitBegins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your work on this @mattyb149. The code changes look go to me. 👍

Copy link
Contributor

@emiliosetiadarma emiliosetiadarma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to build and test with local NiFi against an RDS instance using an Oracle JDBC driver. The UpdateDatabaseTable I have was successfully able to create tables that didn't exist yet in the database.

Code changes also make sense to me.

@gresockj
Copy link
Contributor

Thanks for the reviews -- I will merge this!

@asfgit asfgit closed this in 31d04c8 Jan 12, 2024
asfgit pushed a commit that referenced this pull request Jan 12, 2024
Signed-off-by: Joe Gresock <jgresock@gmail.com>
This closes #8175.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants