Skip to content

NIFI-3873: Improve Avro logical types readability#1788

Closed
ijokarumawak wants to merge 1 commit intoapache:masterfrom
ijokarumawak:nifi-3873
Closed

NIFI-3873: Improve Avro logical types readability#1788
ijokarumawak wants to merge 1 commit intoapache:masterfrom
ijokarumawak:nifi-3873

Conversation

@ijokarumawak
Copy link
Member

Thank you for submitting a contribution to Apache NiFi.

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 master)?

  • Is your initial contribution a single, squashed commit?

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?
  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • 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 travis-ci for build issues and submit an update to your PR as soon as possible.

@ijokarumawak
Copy link
Member Author

This PR changes rendering for logical types.

Before: underlying value is displayed as it is
image

After: display logically converted value
image

while (dataFileReader.hasNext()) {
final GenericData.Record record = dataFileReader.next();
final String formattedRecord = record.toString();
final String formattedRecord = genericData.toString(record);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So as not to forget/ignore the conversation, do you mind taking a look at the comments in #1179 ? As your anonymous GenericData class demonstrates, the toString() method is less than ideal for rendering Avro as JSON. I have a link in the other PR for an implementation of StandardJsonEncoder that mimics the GenericData.toString()
output (mattyb149@879efb7), feel free to use that and add your date/time rendering as needed. If not, that's fine too, just let me know and I'll continue the review, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your suggestion. I took a look at #1179 and customized StandardJsonEncoder. However, it's not clear to me what benefit it provides. GenericData.toString doesn't add extra layer for union objects as described in NIFI-969. Even with a union value, GenericData.toString seems to be working as expected (at least for me).

Since I don't know what should be fixed by applying the StandardJsonEncoder, I prefer keep using .toString unless there is a clear reason.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment in the other PR was that toString() is not guaranteed to be stable or output what you'd expect. The two approaches output different JSON for example (although my StandardJsonEncoder subclass emulates the current GenericData.toString()). However toString() has been fine in the past and for that other PR (which was closed without modification), so I'm ok with keeping toString() for now as well. If it ever "breaks", we'd have to change it in a few places anyway.

@mattyb149
Copy link
Contributor

+1 LGTM, thanks much! Merging to master

@asfgit asfgit closed this in 3fd7d3b May 17, 2017
mattyb149 pushed a commit to mattyb149/nifi that referenced this pull request Nov 30, 2017
Signed-off-by: Matt Burgess <mattyb149@apache.org>

This closes apache#1788
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants