ACCUMULO-4730 Created EntryLengthSummarizer by jkrdev · Pull Request #320 · apache/accumulo

jkrdev · 2017-11-08T16:23:35Z

Built in Summarizer that computes summary information about field lengths. Specifically key length, row length, family length, qualifier length, visibility length, and value length. Incrementally computes min, max, count, sum, and log2 histogram.

keith-turner

@jkrdev thanks for the contribution and welcome to Accumuo! I am still looking at this, I have not looked at the test yet.

keith-turner · 2017-11-08T21:11:23Z

...src/main/java/org/apache/accumulo/core/client/summary/summarizers/EntryLengthSummarizer.java

+  public static final String SUM_VALUES_STAT = "sumValues";
+
+  public static final String TOTAL_STAT = "total";// Total number of Keys
+


It would be nice to reduce the redundant code. One possible way to do this is to create an internal class like the following.

private static class LengthStats { private long min = Long.MAX_VALUE; private long max = Long.MIN_VALUE; private long sum = 0; private long[] counts = new long[32]; private void accept(int lenght) { int idx; if (length < minKey) { min = length; } if (length > max) { maxKey = length; } sum += length; if (length == 0) { idx = 0; } else { idx = IntMath.log2(length, RoundingMode.HALF_UP); } counts[idx]++; } void summarize(String prefix, SummaryConsumer sc) { sc.accept(prefix+"min", (min != Long.MAX_VALUE ? min:0)); sc.accept(prexif+"max", (max != Long.MIN_VALUE ? max:0)); sc.accept(prefix+"sum", sum); for (int i = 0;i < counts.length;i++) { if(counts[i] > 0) { sc.accept(prefix+".logHist."+i, counts[i]); } } } }

Then create an instance of this class for each field and use it in the Collectors accept and summarize methods.

Thanks I will implement this, definitely will make the code cleaner.

keith-turner · 2017-11-08T21:26:48Z

...src/main/java/org/apache/accumulo/core/client/summary/summarizers/EntryLengthSummarizer.java

+
+    };
+  }
+


Could possibly a have static method like the following to cut down on redundant code.

private static merge(String prefix, Map<string, Long> stats1, Map<String, Long> stats2) { // merge min, max, sum, and hist for all props with prefix. }

This method could be called for each prefix.

keith-turner · 2017-11-09T17:09:16Z

...src/main/java/org/apache/accumulo/core/client/summary/summarizers/EntryLengthSummarizer.java

+    stats1.merge(prefix+".min", stats2.get(prefix+".min"), Long::max);
+    stats1.merge(prefix+".max", stats2.get(prefix+".max"), Long::max);
+    stats1.merge(prefix+".sum", stats2.get(prefix+".sum"), Long::sum);
+    stats2.forEach((k,v) -> stats1.merge(k, v, Long::sum));


It seems like this will sum everything, even the min and max keys. I suspect this is only intended to sum log histogram keys? If so it needs a check for the key before doing merge. Also, it seems like the test should catch this?

I'm not sure I can articulate this correctly but I will try. I believe that since there is only one value passed into the merge functions that the remapping function recomputes the same value passed in. If that makes sense. Either way though, I played around with that during my initial coding, and it doesn't in this particular case seem to make any difference. I could make that parameter "null" if that would be better, the output will be the same according to the test.

Now looking at it again it does seem it shouldn't be working but why it is I don't exactly know, but again whether max, min, sum or null it returns sams values.

I wasn't sure why the test were not catching this so I cloned the branch to experiment locally. Realized the test are not calling this code, I completely missed that. Below are the beginnings of a test I wrote while looking into this.

@Test public void testFoo() { SummarizerConfiguration sc = SummarizerConfiguration.builder(EntryLengthSummarizer.class).build(); EntryLengthSummarizer entrySum = new EntryLengthSummarizer(); Collector collector1 = entrySum.collector(sc); collector1.accept(new Key("1","f1","q1"), new Value("v1")); collector1.accept(new Key("1234","f1","q1"), new Value("v111")); collector1.accept(new Key("12345678","f1","q1"), new Value("v111111")); Map<String, Long> stats1 = new TreeMap<>(); collector1.summarize(stats1::put); Collector collector2 = entrySum.collector(sc); collector2.accept(new Key("5432","f11","q12"), new Value("2")); collector2.accept(new Key("12","f11","q1234"), new Value("12")); collector2.accept(new Key("12","f11","q11234567"), new Value("4444")); Map<String, Long> stats2 = new TreeMap<>(); collector2.summarize(stats2::put); System.out.println("========= Stats 1"); stats1.entrySet().forEach(System.out::println); System.out.println("========= Stats 2"); stats2.entrySet().forEach(System.out::println); Combiner combiner = entrySum.combiner(sc); combiner.merge(stats1, stats2); System.out.println("========= Merged"); stats1.entrySet().forEach(System.out::println); }

Oh wow that is complete oversight on my part, I was so focused on the Collector, I didn't test the Combiner. I will write up some test for that also and fix according. Thanks for pointing this out.

Hmmm so for the Combiner I think I understand what should happen for the values that are singular; min,max,sum, and total, but for the log2Histo should it just add the value of the 2nd map to the 1st and combine the total for keys that match?

…ombiner

…unt of keys

jkrdev · 2017-11-13T16:57:39Z

...src/main/java/org/apache/accumulo/core/client/summary/summarizers/EntryLengthSummarizer.java

+    }
+
+    /* i must be less than 32 since counts[] size max is 32. */
+    for (int i = 0; i < 32; i++) {


Not quite sure if 32 is correct here, if someone can confirm would be appreciated.

keith-turner · 2017-11-13T19:54:58Z

...src/main/java/org/apache/accumulo/core/client/summary/summarizers/EntryLengthSummarizer.java

+
+  /* Helper function for merging that is used by the Combiner. */
+  private static void merge(String prefix, Map<String, Long> stats1, Map<String,Long> stats2 ) {
+    if (stats2.containsKey(prefix+".min") && (stats1.containsKey(prefix+".min") == false)) {


I would put this code into a private function where you pass in a key and function. Something like the following

private void merge(String key, BiFunction<Long,Long,Long> mergeFunc, Map<String, Long> stats1, Map<String,Long> stats2){...}

Then the code in this function could just call

merge(prefix+".min", Long::min, stats1, stats2);

keith-turner · 2017-11-14T19:27:51Z

...src/main/java/org/apache/accumulo/core/client/summary/summarizers/EntryLengthSummarizer.java

+
+  /* Helper functions for merging that is used by the Combiner. */
+  private static void merge(String key, BiFunction<Long,Long,Long> mergeFunc, Map<String, Long> stats1, Map<String,Long> stats2) {
+    if (stats2.containsKey(key) && (stats1.containsKey(key) == false)) {


I think Java's merge() function on map handles the case where a key is not present. So I think the following code will insert the value from stats2 if stats1 did not have a value (w/o calling mergeFunc).

Long val2 = stats2.get(key); if(val2 != null) { stats1.merge(key, val2, mergeFunc); }

This also avoid calling contains() and then later get() on the map which is two lookups.

keith-turner · 2017-11-14T19:48:28Z

...est/java/org/apache/accumulo/core/client/summary/summarizers/EntryLengthSummarizersTest.java

+    HashMap<String, Long> stats2 = new HashMap<>();
+    collector2.summarize(stats2::put);
+
+    /*


should remove these comments

keith-turner · 2017-11-14T19:48:47Z

...est/java/org/apache/accumulo/core/client/summary/summarizers/EntryLengthSummarizersTest.java

+    combiner.merge(stats1, stats2);
+
+    /*
+    System.out.println("========= Merged");


keith-turner · 2017-11-14T19:48:59Z

...est/java/org/apache/accumulo/core/client/summary/summarizers/EntryLengthSummarizersTest.java

+    combiner.merge(stats1, stats2);
+
+    /*
+    System.out.println("========= Merged");


keith-turner · 2017-11-14T19:50:20Z

...src/main/java/org/apache/accumulo/core/client/summary/summarizers/EntryLengthSummarizer.java

+ * Summarizer that computes summary information about field lengths.
+ * Specifically key length, row length, family length, qualifier length, visibility length, and value length.
+ * Incrementally computes minimum, maximum, count, sum, and log2 histogram of the lengths.
+ */


should add @since 2.0.0 javadoc tag.

keith-turner

@jkrdev thanks for the contribution! Let me know if you would like to be added to the people page as a contributor. I can add you if you give me the info or you can submit a PR to the accumulo-website repo.

keith-turner · 2017-11-14T22:01:56Z

I squashed these commits into 9cd4be0 and pushed that to master. I ran mvn verify -DskipITs and it did some slight reformatting which is included in 9cd4be0

jkrdev · 2017-11-15T18:42:09Z

I would like to be added to the people page the info I want is just my name Jared R. Thanks.

keith-turner · 2017-11-17T00:35:06Z

I added Jared R. to people page.

jkrdev added 7 commits November 1, 2017 18:31

WIP

a520717

WIP

d3e2b41

WIP - added testing, need to add Log2 Histogram

b5e6da5

Completed with testing.

631fe85

Cleaned up code. Completed with testing.

88e6a87

ACCUMULO-4730 Cleaned up code and Completed w/testing.

dea3472

ACCUMULO-4730 Trailing whitspace removed.

7a0e5ec

ctubbsii changed the title ~~Accumulo-4730 Created EntryLengthSummarizer~~ ACCUMULO-4730 Created EntryLengthSummarizer Nov 8, 2017

ACCUMULO-4730 Cleaned Whitespace again...

74b7235

keith-turner reviewed Nov 8, 2017

View reviewed changes

ACCUMULO-4730 Optimized output and reduced redunacy.

3ddc86b

keith-turner reviewed Nov 9, 2017

View reviewed changes

jkrdev added 2 commits November 13, 2017 15:14

ACCUMULO-4730 Corrected functionality of Merge and added testing of C…

841f83b

…ombiner

ACCUMULO-4730 Added checks in merge for empty stats and different amo…

7b660ab

…unt of keys

jkrdev commented Nov 13, 2017

View reviewed changes

keith-turner reviewed Nov 13, 2017

View reviewed changes

ACCUMULO-4730 Added overloaded merge function to cut redundancy.

b729935

keith-turner reviewed Nov 14, 2017

View reviewed changes

ACCUMULO-4730 updated merge to null check to prevent multiple lookups

4decda3

keith-turner reviewed Nov 14, 2017

View reviewed changes

keith-turner approved these changes Nov 14, 2017

View reviewed changes

ACCUMULO-4730 Cleaned up comments in Test and add javadoc tag

2803096

keith-turner closed this Nov 14, 2017

jkrdev deleted the ACCUMULO-4730 branch December 1, 2017 19:20

		public static final String SUM_VALUES_STAT = "sumValues";

		public static final String TOTAL_STAT = "total";// Total number of Keys

Conversation

jkrdev commented Nov 8, 2017

Uh oh!

keith-turner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkrdev Nov 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keith-turner Nov 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keith-turner left a comment

Choose a reason for hiding this comment

Uh oh!

keith-turner commented Nov 14, 2017

Uh oh!

jkrdev commented Nov 15, 2017

Uh oh!

keith-turner commented Nov 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jkrdev Nov 9, 2017 •

edited

Loading

keith-turner Nov 13, 2017 •

edited

Loading