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 2020/09/15 07:04:27 UTC
[GitHub] [nifi] KuKuDeCheng opened a new pull request #4528: NIFI-7801:Improvement for 'Translate Field Names'.
KuKuDeCheng opened a new pull request #4528:
URL: https://github.com/apache/nifi/pull/4528
We can custom define the rule of mapping the field and column by using Expression Language, not fixed code `colName.toUpperCase().replace("_", "")`
Thank you for submitting a contribution to Apache NiFi.
Please provide a short description of the PR here:
#### Description of PR
_Enables X functionality; fixes bug NIFI-YYYY._
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 **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 `main`)?
- [√ ] 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](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] 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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] KuKuDeCheng commented on a change in pull request #4528: NIFI-7810:Improvement for 'Translate Field Names' for PutDatabaseRecord and ConverJsonToSQL
Posted by GitBox <gi...@apache.org>.
KuKuDeCheng commented on a change in pull request #4528:
URL: https://github.com/apache/nifi/pull/4528#discussion_r502158467
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutDatabaseRecord.java
##########
@@ -146,6 +148,18 @@
protected static Set<Relationship> relationships;
+ public static final Validator TRANSLATE_FIELD_EXPRESSION_VALIDATOR = new Validator() {
Review comment:
Copy that.Sorry reply so late because I just ended my long vacation, I would modify the code 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] KuKuDeCheng commented on pull request #4528: NIFI-7810:Improvement for 'Translate Field Names'.
Posted by GitBox <gi...@apache.org>.
KuKuDeCheng commented on pull request #4528:
URL: https://github.com/apache/nifi/pull/4528#issuecomment-692521618
> Hi @KuKuDeCheng !
>
> I think, the Jira issue id must be mistaken as https://issues.apache.org/jira/browse/NIFI-7801 is about extending Splunk support :)
Thanks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] KuKuDeCheng commented on a change in pull request #4528: NIFI-7810:Improvement for 'Translate Field Names' for PutDatabaseRecord and ConverJsonToSQL
Posted by GitBox <gi...@apache.org>.
KuKuDeCheng commented on a change in pull request #4528:
URL: https://github.com/apache/nifi/pull/4528#discussion_r502158467
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutDatabaseRecord.java
##########
@@ -146,6 +148,18 @@
protected static Set<Relationship> relationships;
+ public static final Validator TRANSLATE_FIELD_EXPRESSION_VALIDATOR = new Validator() {
Review comment:
Copy that.Sorry reply so late because I just ended my long vacation, I would modify the code 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] github-actions[bot] commented on pull request #4528: NIFI-7810:Improvement for 'Translate Field Names' for PutDatabaseRecord and ConverJsonToSQL
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #4528:
URL: https://github.com/apache/nifi/pull/4528#issuecomment-821732093
We're marking this PR as stale due to lack of updates in the past few months. If after another couple of weeks the stale label has not been removed this PR will be closed. This stale marker and eventual auto close does not indicate a judgement of the PR just lack of reviewer bandwidth and helps us keep the PR queue more manageable. If you would like this PR re-opened you can do so and a committer can remove the stale tag. Or you can open a new PR. Try to help review other PRs to increase PR review bandwidth which in turn helps yours.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] simonbence commented on pull request #4528: NIFI-7801:Improvement for 'Translate Field Names'.
Posted by GitBox <gi...@apache.org>.
simonbence commented on pull request #4528:
URL: https://github.com/apache/nifi/pull/4528#issuecomment-692513815
Hi @KuKuDeCheng !
I think, the Jira issue id must be mistaken as https://issues.apache.org/jira/browse/NIFI-7801 is about extending Splunk support :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] mattyb149 commented on a change in pull request #4528: NIFI-7810:Improvement for 'Translate Field Names' for PutDatabaseRecord and ConverJsonToSQL
Posted by GitBox <gi...@apache.org>.
mattyb149 commented on a change in pull request #4528:
URL: https://github.com/apache/nifi/pull/4528#discussion_r498888543
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ConvertJSONToSQL.java
##########
@@ -249,7 +271,7 @@
properties.add(TABLE_NAME);
properties.add(CATALOG_NAME);
properties.add(SCHEMA_NAME);
- properties.add(TRANSLATE_FIELD_NAMES);
Review comment:
Removing a field from the list of supported property descriptors will cause all existing flows (after upgrade) to mark these processors as Invalid. I think we should keep this field and mention in the description of Translate Field Expression that it will be ignored if Translate Field Names is false. That means the other logic below would have to change slightly as well, to provide the default expression if Translate Field Names is set to true.
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ConvertJSONToSQL.java
##########
@@ -124,6 +129,18 @@
"Fail on Unmatched Columns",
"A flow will fail if any column in the database that does not have a field in the JSON document. An error will be logged");
+ public static final Validator TRANSLATE_FIELD_EXPRESSION_VALIDATOR = new Validator() {
Review comment:
I'm not sure we need a special field validator for this (see my comments below), I think a NON_EMPTY_EL_VALIDATOR would suffice.
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutDatabaseRecord.java
##########
@@ -146,6 +148,18 @@
protected static Set<Relationship> relationships;
+ public static final Validator TRANSLATE_FIELD_EXPRESSION_VALIDATOR = new Validator() {
Review comment:
All the same comments for ConvertJSONToSQL apply here as well
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ConvertJSONToSQL.java
##########
@@ -158,12 +175,17 @@
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
- static final PropertyDescriptor TRANSLATE_FIELD_NAMES = new PropertyDescriptor.Builder()
- .name("Translate Field Names")
- .description("If true, the Processor will attempt to translate JSON field names into the appropriate column names for the table specified. "
- + "If false, the JSON field names must match the column names exactly, or the column will not be updated")
- .allowableValues("true", "false")
- .defaultValue("true")
+ static final PropertyDescriptor TRANSLATE_FIELD_EXPRESSION = new PropertyDescriptor.Builder()
+ .name("put-db-record-translate-expression")
+ .displayName("Translate Field Expression")
+ .description("If set, the Processor will attempt to translate field names into the appropriate column names for the table specified using "
+ + "the Expression language, and the expression's value should start with '${colName:'. If not set, the field names must match the column names exactly, "
Review comment:
We may want to mention here that we are translating both field and column names in order to match the two, rather than just translating field names and matching against actual column names. That makes `colName` a bit of an awkward choice, but I think users would understand what it means (with the additional doc I just suggested). In fact `columnName` might be even more helpful.
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ConvertJSONToSQL.java
##########
@@ -158,12 +175,17 @@
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
- static final PropertyDescriptor TRANSLATE_FIELD_NAMES = new PropertyDescriptor.Builder()
- .name("Translate Field Names")
- .description("If true, the Processor will attempt to translate JSON field names into the appropriate column names for the table specified. "
- + "If false, the JSON field names must match the column names exactly, or the column will not be updated")
- .allowableValues("true", "false")
- .defaultValue("true")
+ static final PropertyDescriptor TRANSLATE_FIELD_EXPRESSION = new PropertyDescriptor.Builder()
+ .name("put-db-record-translate-expression")
+ .displayName("Translate Field Expression")
+ .description("If set, the Processor will attempt to translate field names into the appropriate column names for the table specified using "
+ + "the Expression language, and the expression's value should start with '${colName:'. If not set, the field names must match the column names exactly, "
+ + "or the column will not be updated. Note that the scope of expression language is 'Variable Registry and FlowFile Attributes', "
+ + "but we would not use them when evaluating.")
+ .expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
+ .required(false)
+ .defaultValue("${colName:toUpper():replace('_','')}")
Review comment:
Since you are adding the column name as a `colName` attribute before evaluating the Expression Language expression, I think requiring the entire expression's value to start with `${colName` is more strict than it has to be. Instead we could just document that the `colName` (or `columnName`, see above) is available for use in any EL expression. I imagine most use cases will see `colName` as the subject (and indeed it has to be part of the expression or else it will not match one of the field or column), and most operations can be done that way (append, prepend, e.g.), but I like to make things as flexible as possible.
The documentation can include good examples of how to use this the intended way, but flexibility may allow for support of more use cases.
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ConvertJSONToSQL.java
##########
@@ -158,12 +175,17 @@
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
- static final PropertyDescriptor TRANSLATE_FIELD_NAMES = new PropertyDescriptor.Builder()
- .name("Translate Field Names")
- .description("If true, the Processor will attempt to translate JSON field names into the appropriate column names for the table specified. "
- + "If false, the JSON field names must match the column names exactly, or the column will not be updated")
- .allowableValues("true", "false")
- .defaultValue("true")
+ static final PropertyDescriptor TRANSLATE_FIELD_EXPRESSION = new PropertyDescriptor.Builder()
+ .name("put-db-record-translate-expression")
+ .displayName("Translate Field Expression")
+ .description("If set, the Processor will attempt to translate field names into the appropriate column names for the table specified using "
+ + "the Expression language, and the expression's value should start with '${colName:'. If not set, the field names must match the column names exactly, "
+ + "or the column will not be updated. Note that the scope of expression language is 'Variable Registry and FlowFile Attributes', "
+ + "but we would not use them when evaluating.")
+ .expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
Review comment:
You shouldn't need to set this to FLOWFILE_ATTRIBUTES in order to call the method that evaluates the expression based on the single `colName` attribute, you can just call the version that takes a Map and no FlowFile reference.
Having said that, if you make the expression more flexible (see below), then you could support Variably Registry and FlowFile attributes in the Translate Field Expression property, you'd just need to document that `colName` will contain the field/column name and will override any value that had been previously set for that attribute.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] github-actions[bot] closed pull request #4528: NIFI-7810:Improvement for 'Translate Field Names' for PutDatabaseRecord and ConverJsonToSQL
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #4528:
URL: https://github.com/apache/nifi/pull/4528
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org