nifi-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [nifi] mattyb149 commented on a change in pull request #4528: NIFI-7810:Improvement for 'Translate Field Names' for PutDatabaseRecord and ConverJsonToSQL
Date Fri, 02 Oct 2020 16:46:09 GMT

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



Mime
View raw message