Skip to content

Orc 256 unmask range option #184

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
moresandeep wants to merge 3 commits into from
Closed

Orc 256 unmask range option #184

moresandeep wants to merge 3 commits into from

Conversation

@moresandeep
Copy link
Contributor

@moresandeep moresandeep commented Oct 28, 2017

This PR contains changes that enables unmasking range option for redact mask (ORC-256).

  1. The react mask would accept an additional option (option Added a stream block size parameter to change the size of InputStreams' buffers #3 in this case) that has the configuration for leaving certain ranges of strings unmasked
  2. The options will look like "0:4,-4:-1"
  3. Range unmasking is available for 1. Long 2. Double 3. String 4. Decimal

@moresandeep moresandeep force-pushed the ORC-256-Unmask_Range_Option branch from 7f8443c to 7ab1f6f Compare November 9, 2017 17:18
@moresandeep
Copy link
Contributor Author

moresandeep commented Nov 9, 2017

Updated the PR he changes are as follows:

  1. Fixed the find bugs issue.
  2. Merged the feature into a single commit.

@omalley
Copy link
Contributor

omalley commented Dec 6, 2017

I think we should change the processing for numerics (when there is unmasked ranges) to be:

unmasked number -> string -> mask as string -> masked number

@moresandeep moresandeep force-pushed the ORC-256-Unmask_Range_Option branch from 7ab1f6f to 4d3ed79 Compare December 7, 2017 16:55
@moresandeep
Copy link
Contributor Author

moresandeep commented Dec 7, 2017 •
edited
Loading

@omalley I updated the PR with your suggestions, sadly, there are two commits I could not squash the "merge from master" commit, let me know if this is an issue.

thanks for the review !

xndai
xndai reviewed Dec 7, 2017
View reviewed changes
java/core/src/java/org/apache/orc/impl/mask/RedactMaskFactory.java Outdated
private final boolean maskTimestamp;

// index tuples that are not to be masked
private final SortedMap<Integer,Integer> unmaskIndexRanges = Collections.synchronizedSortedMap(new TreeMap());
Copy link
Contributor

@xndai xndai Dec 4, 2017

Choose a reason for hiding this comment

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

Any particular reason that you need a sychronized map here?

Copy link
Contributor Author

@moresandeep moresandeep Dec 7, 2017

Choose a reason for hiding this comment

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

Hello @xndai,
Thanks for the review, I was trying to be cautious, but I can get rid of it.

java/core/src/java/org/apache/orc/impl/mask/RedactMaskFactory.java Outdated
for(int r = start; r < start + length; ++r) {
target.vector[r] = maskLong(source.vector[r]) & mask;
target.isNull[r] = source.isNull[r];
target.vector[r] = maskLong(source.vector[r]) & mask;
Copy link
Contributor

@xndai xndai Dec 4, 2017

Choose a reason for hiding this comment

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

Remove leading space. Same as below.

Copy link
Contributor Author

@moresandeep moresandeep Dec 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is fixed, it was an issue with the previous patch.

java/core/src/java/org/apache/orc/impl/mask/RedactMaskFactory.java Outdated
posn = -posn -2;
}
return DIGIT_REPLACEMENT * base * DOUBLE_POWER_10[posn];
return unmaskRangeDoubleValue(value,DIGIT_REPLACEMENT * base * DOUBLE_POWER_10[posn]);
Copy link
Contributor

@xndai xndai Dec 4, 2017

Choose a reason for hiding this comment

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

Add space after comma.

Copy link
Contributor Author

@moresandeep moresandeep Dec 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is fixed, the method signature is different. I think you are seeing an older commit which is weird, let me know if you still see this.

java/core/src/test/org/apache/orc/impl/mask/TestUnmaskRange.java Outdated
assertEquals(new HiveDecimalWritable("777777777777777777.7777"),
mask.maskDecimal(new HiveDecimalWritable("0123456789123456789.01230")));
*/

Copy link
Contributor

@xndai xndai Dec 7, 2017

Choose a reason for hiding this comment

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

remove empty lines. Same for other places.

Copy link
Contributor Author

@moresandeep moresandeep Dec 7, 2017

Choose a reason for hiding this comment

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

ok, will do.

@moresandeep
Copy link
Contributor Author

moresandeep commented Dec 7, 2017

@xndai @omalley I updated the PR with the suggested changes, let me know if you have any questions.

@asfgit asfgit closed this in 6aa5224 Dec 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xndai xndai xndai left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@moresandeep @omalley @xndai