pgyori commented on pull request #5388: URL: https://github.com/apache/nifi/pull/5388#issuecomment-920928975
Thank you for reviewing, @exceptionfactory ! You are correct, the intention is to use `STRING` type when `useLogicalTypes` is `false`. This idea comes from the implementation of JdbcCommon class: https://github.com/apache/nifi/blob/main/nifi-nar-bundles/nifi-extension-utils/nifi-database-utils/src/main/java/org/apache/nifi/util/db/JdbcCommon.java In JdbcCommon, in each case when `options.useLogicalTypes` is false and otherwise the type in question would require avro logical type, `STRING` type is returned. In RecordSqlWriter, this behavior is correct until (including) this line: https://github.com/apache/nifi/blob/4af3fac07a521b8334b36e55ad9b4929e182c3d8/nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/sql/RecordSqlWriter.java#L72 However, the next line overwrites the settings in the original implementation and avro logical types do appear in the result. So the goal of this fix is to unify the behavior when having avro logical types in the result is to be avoided. I believe it does make sense to use `STRING` as the default type for Date, Time, and Decimal columns, but @mattyb149 may be able to provide more background information on this decision. Currently I'm not planning to add further unit tests with actual data content (merely because I'm trying to save a little time), I'm concentrating on checking the proper schema creation. I found that some branches of the logic are not yet covered by the tests that exist so far, so I'm working on adding more cases (with concentrating on schema only). -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
