Skip to content

NIFI-1663: Add ConvertAvroToORC processor#727

Closed
mattyb149 wants to merge 3 commits intoapache:masterfrom
mattyb149:old_orc
Closed

NIFI-1663: Add ConvertAvroToORC processor#727
mattyb149 wants to merge 3 commits intoapache:masterfrom
mattyb149:old_orc

Conversation

@mattyb149
Copy link
Contributor

@mattyb149 mattyb149 commented Jul 27, 2016

This PR is based on #706 which removed the ConvertAvroToORC processor using Hive 2.x and Apache ORC 1.x. This PR replaces that processor with one that uses Hive 1.2.1 (which includes hive-orc before it was split into its own Apache project).

@olegz
Copy link
Contributor

olegz commented Aug 2, 2016

Reviewing

/**
* Utility methods for ORC support (conversion from Avro, conversion to Hive types, e.g.
*/
public class NiFiOrcUtils {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it have to be public?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I will reduce the visibility. For util classes I usually start them as public, but since it's unlikely this NAR will be the parent/dependency of another, public is not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it does have to be public, it is referenced from multiple packages.

@mattyb149
Copy link
Contributor Author

mattyb149 commented Aug 3, 2016

I see a few comments about PutHiveStreaming, that is part of a previous commit that is being handled under #706, it's just in here in order to inherit the downgrade of Hive.

@mattyb149 mattyb149 force-pushed the old_orc branch 3 times, most recently from 8082e95 to 242e4dc Compare August 5, 2016 21:14
@mattyb149
Copy link
Contributor Author

Closing the PR and cancelling the current patch as there are issues with complex types, will reopen once fixed

@olegz
Copy link
Contributor

olegz commented Aug 10, 2016

Aside from minor stylistic things (didn't comment on them), I am +1. Verified new tests, all pass. Test coverage on the overall module is high.
Merging

@asfgit asfgit closed this in d972023 Aug 10, 2016
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