-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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-11858 Configurable Column Name Normalization in PutDatabaseRecord and UpdateDatabaseTable #7544
Conversation
…abaseTable processor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ravinarayansingh Thanks for the contribution. There are still some static code analysis issues. You can test locally by running the following command:
./mvnw clean install -Pcontrib-check -am -pl :nifi-standard-processors
Hi @exceptionfactory , following is the rat report: SummaryGenerated at: 2023-08-01T08:57:42-07:00 Notes: 24 Apache Licensed: 13 JavaDocs are generated, thus a license header is optional. 14 Unknown Licenses Files with unapproved licenses: nifi-h2/nifi-h2-database/nifi-h2-database.iml Archives:
Files with Apache License headers will be marked AL let me know if i have to change anything |
@ravinarayansingh The Static Analysis build lists the particular problem: https://github.com/apache/nifi/actions/runs/5719909353/job/15509139654?pr=7544#step:5:5763 It looks like those |
Hi @exceptionfactory |
@ravinarayansingh you have a merge conflict. |
…NIFI-11858 � Conflicts: � nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/UpdateDatabaseTable.java � nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/db/TableSchema.java
resolved |
Hi @exceptionfactory , |
@ravinarayansingh These changes still need substantive review, along with a number of other pull requests, but thanks for addressing the initial feedback so far. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your patience on this pull request review @ravinarayansingh. Reviewing the implementation details, the general concept of a configurable column normalization strategy is helpful.
As a general recommendation, it looks like it would be helpful to make ColumnNameNormalizer
an interface, with getNormalizedName(String columnName)
as the interface method. Although the current implementation is simple enough, providing separate implementations would remove the need for the conditional logic. Given that this method will be called for every column, there seems to be value in having narrowly defined implementation classes.
Hi @exceptionfactory |
…NIFI-11858 � Conflicts: � nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutDatabaseRecord.java
Hi @exceptionfactory , I have made the required changes, please have a look |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates @ravinarayansingh. The general approach looks good, but there are a few more naming recommendations. In particular, any regular expression pattern should be parsed to a Pattern
object for more efficient processing in the normalizer implementation.
...standard-processors/src/main/java/org/apache/nifi/processors/standard/PutDatabaseRecord.java
Outdated
Show resolved
Hide resolved
...standard-processors/src/main/java/org/apache/nifi/processors/standard/PutDatabaseRecord.java
Outdated
Show resolved
Hide resolved
...standard-processors/src/main/java/org/apache/nifi/processors/standard/PutDatabaseRecord.java
Outdated
Show resolved
Hide resolved
...standard-processors/src/main/java/org/apache/nifi/processors/standard/PutDatabaseRecord.java
Outdated
Show resolved
Hide resolved
...standard-processors/src/main/java/org/apache/nifi/processors/standard/PutDatabaseRecord.java
Outdated
Show resolved
Hide resolved
...rd-processors/src/main/java/org/apache/nifi/processors/standard/db/ColumnNameNormalizer.java
Outdated
Show resolved
Hide resolved
...essors/src/main/java/org/apache/nifi/processors/standard/db/ColumnNameNormalizerUtility.java
Outdated
Show resolved
Hide resolved
...ard-processors/src/main/java/org/apache/nifi/processors/standard/db/TranslationStrategy.java
Outdated
Show resolved
Hide resolved
...ard-processors/src/main/java/org/apache/nifi/processors/standard/db/TranslationStrategy.java
Outdated
Show resolved
Hide resolved
...ard-processors/src/main/java/org/apache/nifi/processors/standard/db/TranslationStrategy.java
Outdated
Show resolved
Hide resolved
…/src/main/java/org/apache/nifi/processors/standard/PutDatabaseRecord.java Co-authored-by: David Handermann <[email protected]>
…/src/main/java/org/apache/nifi/processors/standard/PutDatabaseRecord.java Co-authored-by: David Handermann <[email protected]>
Hi @exceptionfactory |
Thanks for the updates @ravinarayansingh, the |
This needs a rebase against the latest main, then I will take a look also, thanks! |
Hi @mattyb149 |
…NIFI-11858 # Conflicts: # nifi-extension-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/db/ColumnNameNormalizer.java # nifi-extension-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/db/ColumnNameNormalizerFactory.java # nifi-extension-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/db/TranslationStrategy.java # nifi-extension-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/db/impl/PatternNormalizer.java # nifi-extension-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/db/impl/RemoveAllSpecialCharNormalizer.java # nifi-extension-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/db/impl/RemoveSpaceNormalizer.java # nifi-extension-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/db/impl/RemoveUnderscoreNormalizer.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed this PR today. I would like at least 24 hours more to review this and see if I have feedback on the code. Right now I am giving some wording feedback.
...ard-processors/src/main/java/org/apache/nifi/processors/standard/db/TranslationStrategy.java
Outdated
Show resolved
Hide resolved
...ard-processors/src/main/java/org/apache/nifi/processors/standard/db/TranslationStrategy.java
Outdated
Show resolved
Hide resolved
...ard-processors/src/main/java/org/apache/nifi/processors/standard/db/TranslationStrategy.java
Outdated
Show resolved
Hide resolved
...ard-processors/src/main/java/org/apache/nifi/processors/standard/db/TranslationStrategy.java
Outdated
Show resolved
Hide resolved
…NIFI-11858 # Conflicts: # nifi-extension-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/PutDatabaseRecordTest.java
Hi @jrsteinebrey |
Thanks for the changes you made. Some other files in this PR now have checkstyle violations: Warning: src/main/java/org/apache/nifi/processors/standard/db/TableSchema.java:[155,47] (whitespace) WhitespaceAround: '!=' is not followed by whitespace. |
The checkstyle rules were recently made more stringent, looks like this needs another rebase and please run your Maven build with the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like @mattyb149, @exceptionfactory and anyone else interested to respond to my questions or comment if they disagree with any of my suggestions.
@@ -177,7 +181,25 @@ public class UpdateDatabaseTable extends AbstractProcessor { | |||
.allowableValues("true", "false") | |||
.defaultValue("true") | |||
.build(); | |||
public static final PropertyDescriptor TRANSLATION_STRATEGY = new PropertyDescriptor.Builder() | |||
.required(true) | |||
.name("Column Name Translation Strategy") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ravinarayansingh Same comment as above that these apply to both field and column names.
public static final PropertyDescriptor TRANSLATION_STRATEGY = new PropertyDescriptor.Builder() | ||
.required(true) | ||
.name("Column Name Translation Strategy") | ||
.description("The strategy used to normalize table column name") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ravinarayansingh The code uses all three of the these Translate properties (Translate Field Names, Column Name Translation Strategy, and Column Name Translation Pattern) and uses them to translate BOTH field names AND column names. I recommend that the displayable names and descriptions be changed to reflect the fact that they apply to both field and column names.
This comment also equally applies to UpdateDatabaseTable class.
@@ -270,6 +274,7 @@ public class PutDatabaseRecord extends AbstractProcessor { | |||
.build(); | |||
|
|||
static final PropertyDescriptor TRANSLATE_FIELD_NAMES = new Builder() | |||
.required(true) | |||
.name("put-db-record-translate-field-names") | |||
.displayName("Translate Field Names") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel it is valuable to make these property names as clear as possible because this is a sophisticated feature.
I think another word besides "Translate" could be found to communicate what this feature does.
Here are some property display names I have thought of. I think Normalize is clearer. What to people think?
"Translate Field and Column Names for Comparison"
"Normalize Field and Column Names for Comparison"
"Adjust Field and Column Names for Comparison"
"Filter Field and Column Names for Comparison"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @exceptionfactory and @exceptionfactory
if you have any thought or suggestion regarding @jrsteinebrey above comment please let me know
...fi-standard-processors/src/main/java/org/apache/nifi/processors/standard/db/TableSchema.java
Outdated
Show resolved
Hide resolved
...fi-standard-processors/src/main/java/org/apache/nifi/processors/standard/db/TableSchema.java
Outdated
Show resolved
Hide resolved
...rd-processors/src/main/java/org/apache/nifi/processors/standard/db/ColumnNameNormalizer.java
Outdated
Show resolved
Hide resolved
...fi-standard-processors/src/main/java/org/apache/nifi/processors/standard/db/TableSchema.java
Outdated
Show resolved
Hide resolved
...ndard-processors/src/main/java/org/apache/nifi/processors/standard/db/ColumnDescription.java
Show resolved
Hide resolved
Thanks, @ravinarayansingh. My comments that are automatically marked as Outdated are all resolved. |
Looks like there are some merge and/or rebase issues here, might be worth a new PR applying the desired commits to a feature branch based off the latest main branch. |
Hi @mattyb149 |
Closing this in favor of the new PR, thanks! |
Summary
NiFi 11858
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000
NIFI-00000
Pull Request Formatting
main
branchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
mvn clean install -P contrib-check
Licensing
LICENSE
andNOTICE
filesDocumentation