NIFI-13531: Support dynamic delimiter in CSVRecordSetWriter via FlowFile attribute#10061
NIFI-13531: Support dynamic delimiter in CSVRecordSetWriter via FlowFile attribute#10061samkitdev17 wants to merge 2 commits intoapache:mainfrom
Conversation
exceptionfactory
left a comment
exceptionfactory
left a comment
There was a problem hiding this comment.
Thanks for the contribution @samkitdev17.
Please review the unit test output as 18 tests are currently failing as a result of these changes.
There are some other code issues with the changes, but the test failures should be corrected first.
Running a local build with -P contrib-check will also surface stylistic issues.
exceptionfactory
left a comment
exceptionfactory
left a comment
There was a problem hiding this comment.
@samkitdev17 Thanks for updating the pull request, but the proposed change and subsequent disabling of Expression Language validation are not heading in the right direction. Most of the code changes are related to formatting and should be reverted.
The core change in ExecuteSQLRecord is also incorrect because it is evaluating a standard property for the Record Writer.
Given the number of issues, I am closing this pull request for now. Please revisit the approach and consider a new pull request after appropriately scoping the changes desired.
| + "will be the column names (unless the 'Include Header Line' property is false). All subsequent lines will be the values " | ||
| + "corresponding to the record fields.") | ||
| public class CSVRecordSetWriter extends DateTimeTextRecordSetWriter implements RecordSetWriterFactory { | ||
| private static final org.slf4j.Logger log = org.slf4j.LoggerFactory.getLogger(CSVRecordSetWriter.class); |
There was a problem hiding this comment.
The logging additions are not necessary and should be reverted.
| final Map<String, String> dbcpProperties = new HashMap<>(); | ||
|
|
||
| runner = TestRunners.newTestRunner(ExecuteSQLRecord.class); | ||
| runner.setValidateExpressionUsage(false); |
There was a problem hiding this comment.
Disabling expression language validation does not look like a suitable approach, the actual reasons behind the failure should be revisited.
| this.recordSetWriterFactory = recordSetWriterFactory; | ||
| this.writeResultRef = new AtomicReference<>(); | ||
| this.maxRowsPerFlowFile = maxRowsPerFlowFile; | ||
| this.options = options; | ||
| this.originalAttributes = originalAttributes; | ||
| } | ||
| this.writeResultRef = new AtomicReference<>(); | ||
| this.maxRowsPerFlowFile = maxRowsPerFlowFile; | ||
| this.recordSetWriterFactory = recordSetWriterFactory; | ||
| this.originalAttributes = originalAttributes; | ||
|
|
||
|
|
||
| // Create a new AvroConversionOptions with the evaluated options | ||
| this.options = options; |
There was a problem hiding this comment.
This changes should be reverted.
|
|
||
| import java.io.IOException; | ||
| import java.io.OutputStream; | ||
| import java.io.OutputStream; |
There was a problem hiding this comment.
This change should be reverted.
| import java.util.Collections; | ||
| import java.util.List; | ||
| import java.util.Set; | ||
| import java.util.*; |
There was a problem hiding this comment.
Wildcard imports are disallowed based on the project Checkstyle, so this should be reverted.
2 participants
Description
Added support in
CSVRecordSetWriterto dynamically resolve the delimiter from the FlowFile attribute (csv.delimiter), allowing per-FlowFile customization.Highlights:
Value Separator resolved to: '{}'Testing:
JIRA Ticket: https://issues.apache.org/jira/browse/NIFI-13531
✔️ I have signed the Apache Contributor License Agreement.