From 4ff1620131703b06ca831fb2f5467ae1f80ef32d Mon Sep 17 00:00:00 2001 From: "navis.ryu" Date: Mon, 7 Mar 2016 10:04:55 +0900 Subject: [PATCH 1/2] Relay final value to yielder in CombineSequence (Fix for #2586) --- .../druid/common/guava/CombiningSequence.java | 7 +- .../common/guava/CombiningSequenceTest.java | 82 +++++++++++++++++++ 2 files changed, 85 insertions(+), 4 deletions(-) diff --git a/common/src/main/java/io/druid/common/guava/CombiningSequence.java b/common/src/main/java/io/druid/common/guava/CombiningSequence.java index ceb714e43ae..da021fd936b 100644 --- a/common/src/main/java/io/druid/common/guava/CombiningSequence.java +++ b/common/src/main/java/io/druid/common/guava/CombiningSequence.java @@ -19,7 +19,6 @@ package io.druid.common.guava; -import com.google.common.base.Function; import com.google.common.collect.Ordering; import com.metamx.common.guava.Accumulator; import com.metamx.common.guava.Sequence; @@ -81,7 +80,7 @@ public class CombiningSequence implements Sequence return makeYielder(baseYielder, combiningAccumulator, false); } - public Yielder makeYielder( + public Yielder makeYielder( Yielder yielder, final CombiningYieldingAccumulator combiningAccumulator, boolean finalValue @@ -102,13 +101,13 @@ public class CombiningSequence implements Sequence finalFinalValue = true; if(!combiningAccumulator.yielded()) { - return Yielders.done(null, yielder); + return Yielders.done(retVal, yielder); } else { finalYielder = Yielders.done(null, yielder); } } else { - return Yielders.done(null, yielder); + return Yielders.done(combiningAccumulator.getRetVal(), yielder); } } diff --git a/common/src/test/java/io/druid/common/guava/CombiningSequenceTest.java b/common/src/test/java/io/druid/common/guava/CombiningSequenceTest.java index c280f15d991..728d5701cc0 100644 --- a/common/src/test/java/io/druid/common/guava/CombiningSequenceTest.java +++ b/common/src/test/java/io/druid/common/guava/CombiningSequenceTest.java @@ -20,6 +20,8 @@ package io.druid.common.guava; import com.google.common.base.Predicate; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Iterables; import com.google.common.collect.Iterators; import com.google.common.collect.Lists; import com.google.common.collect.Ordering; @@ -36,6 +38,7 @@ import org.junit.runners.Parameterized; import javax.annotation.Nullable; import java.io.IOException; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Iterator; @@ -271,4 +274,83 @@ public class CombiningSequenceTest Assert.assertFalse(expectedVals.hasNext()); yielder.close(); } + + @Test + public void testComplexSequence() + { + List combined = Sequences.toList(getComplexSequence(), new ArrayList()); + Assert.assertEquals(8, Iterables.getOnlyElement(combined).intValue()); + + Yielder yielder = getComplexSequence().toYielder( + null, + new YieldingAccumulator() + { + @Override + public Integer accumulate(Integer accumulated, Integer in) + { + yield(); + return in; + } + } + ); + + List combinedByYielder = new ArrayList<>(); + while (!yielder.isDone()) { + combinedByYielder.add(yielder.get()); + yielder = yielder.next(null); + } + + Assert.assertEquals(8, Iterables.getOnlyElement(combinedByYielder).intValue()); + } + + private Sequence getComplexSequence() + { + Ordering alwaysSame = new Ordering() + { + @Override + public int compare(Integer left, Integer right) + { + return 0; + } + }; + + BinaryFn plus = new BinaryFn() + { + @Override + public Integer apply(Integer arg1, Integer arg2) + { + if (arg1 == null) { + return arg2; + } + + if (arg2 == null) { + return arg1; + } + + return arg1 + arg2; + } + }; + + return CombiningSequence.create( + Sequences.concat( + ImmutableList.>of( + CombiningSequence.create( + Sequences.simple(ImmutableList.of(3)) + , + alwaysSame, + plus + ) + , + CombiningSequence.create( + Sequences.simple(ImmutableList.of(5)) + , + alwaysSame, + plus + ) + ) + ), + alwaysSame, + plus + ); + } } From 1b3fd8a8aabbec60aadf03cae23d71cb01c98f65 Mon Sep 17 00:00:00 2001 From: "navis.ryu" Date: Wed, 9 Mar 2016 02:08:52 +0900 Subject: [PATCH 2/2] added more tests and fixed concat+combine --- .../druid/common/guava/CombiningSequence.java | 1 + .../common/guava/CombiningSequenceTest.java | 82 ------------- .../common/guava/ComplexSequenceTest.java | 116 ++++++++++++++++++ 3 files changed, 117 insertions(+), 82 deletions(-) create mode 100644 common/src/test/java/io/druid/common/guava/ComplexSequenceTest.java diff --git a/common/src/main/java/io/druid/common/guava/CombiningSequence.java b/common/src/main/java/io/druid/common/guava/CombiningSequence.java index da021fd936b..6ff60ca5b42 100644 --- a/common/src/main/java/io/druid/common/guava/CombiningSequence.java +++ b/common/src/main/java/io/druid/common/guava/CombiningSequence.java @@ -123,6 +123,7 @@ public class CombiningSequence implements Sequence @Override public Yielder next(OutType initValue) { + combiningAccumulator.reset(); return makeYielder(finalYielder, combiningAccumulator, finalFinalValue); } diff --git a/common/src/test/java/io/druid/common/guava/CombiningSequenceTest.java b/common/src/test/java/io/druid/common/guava/CombiningSequenceTest.java index 728d5701cc0..c280f15d991 100644 --- a/common/src/test/java/io/druid/common/guava/CombiningSequenceTest.java +++ b/common/src/test/java/io/druid/common/guava/CombiningSequenceTest.java @@ -20,8 +20,6 @@ package io.druid.common.guava; import com.google.common.base.Predicate; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.Iterables; import com.google.common.collect.Iterators; import com.google.common.collect.Lists; import com.google.common.collect.Ordering; @@ -38,7 +36,6 @@ import org.junit.runners.Parameterized; import javax.annotation.Nullable; import java.io.IOException; -import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Iterator; @@ -274,83 +271,4 @@ public class CombiningSequenceTest Assert.assertFalse(expectedVals.hasNext()); yielder.close(); } - - @Test - public void testComplexSequence() - { - List combined = Sequences.toList(getComplexSequence(), new ArrayList()); - Assert.assertEquals(8, Iterables.getOnlyElement(combined).intValue()); - - Yielder yielder = getComplexSequence().toYielder( - null, - new YieldingAccumulator() - { - @Override - public Integer accumulate(Integer accumulated, Integer in) - { - yield(); - return in; - } - } - ); - - List combinedByYielder = new ArrayList<>(); - while (!yielder.isDone()) { - combinedByYielder.add(yielder.get()); - yielder = yielder.next(null); - } - - Assert.assertEquals(8, Iterables.getOnlyElement(combinedByYielder).intValue()); - } - - private Sequence getComplexSequence() - { - Ordering alwaysSame = new Ordering() - { - @Override - public int compare(Integer left, Integer right) - { - return 0; - } - }; - - BinaryFn plus = new BinaryFn() - { - @Override - public Integer apply(Integer arg1, Integer arg2) - { - if (arg1 == null) { - return arg2; - } - - if (arg2 == null) { - return arg1; - } - - return arg1 + arg2; - } - }; - - return CombiningSequence.create( - Sequences.concat( - ImmutableList.>of( - CombiningSequence.create( - Sequences.simple(ImmutableList.of(3)) - , - alwaysSame, - plus - ) - , - CombiningSequence.create( - Sequences.simple(ImmutableList.of(5)) - , - alwaysSame, - plus - ) - ) - ), - alwaysSame, - plus - ); - } } diff --git a/common/src/test/java/io/druid/common/guava/ComplexSequenceTest.java b/common/src/test/java/io/druid/common/guava/ComplexSequenceTest.java new file mode 100644 index 00000000000..7848eb23931 --- /dev/null +++ b/common/src/test/java/io/druid/common/guava/ComplexSequenceTest.java @@ -0,0 +1,116 @@ +/* + * Licensed to Metamarkets Group Inc. (Metamarkets) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. Metamarkets licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package io.druid.common.guava; + +import com.google.common.collect.Ordering; +import com.google.common.primitives.Ints; +import com.metamx.common.guava.Sequence; +import com.metamx.common.guava.Sequences; +import com.metamx.common.guava.Yielder; +import com.metamx.common.guava.YieldingAccumulator; +import com.metamx.common.guava.nary.BinaryFn; +import org.junit.Assert; +import org.junit.Test; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; + +public class ComplexSequenceTest +{ + @Test + public void testComplexSequence() + { + Sequence complex; + check("[3, 5]", complex = concat(combine(simple(3)), combine(simple(5)))); + check("[8]", complex = combine(complex)); + check("[8, 6, 3, 5]", complex = concat(complex, concat(combine(simple(2, 4)), simple(3, 5)))); + check("[22]", complex = combine(complex)); + check("[22]", concat(complex, simple())); + } + + private void check(String expected, Sequence complex) + { + List combined = Sequences.toList(complex, new ArrayList()); + Assert.assertEquals(expected, combined.toString()); + + Yielder yielder = complex.toYielder( + null, + new YieldingAccumulator() + { + @Override + public Integer accumulate(Integer accumulated, Integer in) + { + yield(); + return in; + } + } + ); + + List combinedByYielder = new ArrayList<>(); + while (!yielder.isDone()) { + combinedByYielder.add(yielder.get()); + yielder = yielder.next(null); + } + + Assert.assertEquals(expected, combinedByYielder.toString()); + } + + private Sequence simple(int... values) + { + return Sequences.simple(Ints.asList(values)); + } + + private Sequence combine(Sequence sequence) + { + return CombiningSequence.create(sequence, alwaysSame, plus); + } + + private Sequence concat(Sequence... sequences) + { + return Sequences.concat(Arrays.asList(sequences)); + } + + private final Ordering alwaysSame = new Ordering() + { + @Override + public int compare(Integer left, Integer right) + { + return 0; + } + }; + + private final BinaryFn plus = new BinaryFn() + { + @Override + public Integer apply(Integer arg1, Integer arg2) + { + if (arg1 == null) { + return arg2; + } + + if (arg2 == null) { + return arg1; + } + + return arg1 + arg2; + } + }; +}