Skip to content
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

Fix string first/last aggregator comparator #12773

Merged
merged 2 commits into from
Aug 1, 2022
Merged

Fix string first/last aggregator comparator #12773

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
071660d
Fix string first/last aggregator comparator
rohangarg Jul 4, 2022
361e712
fixup! Fix string first/last aggregator comparator
rohangarg Jul 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 38 additions & 2 deletions ...ing/src/main/java/org/apache/druid/query/aggregation/SerializablePairLongStringSerde.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@

import org.apache.druid.data.input.InputRow;
import org.apache.druid.java.util.common.StringUtils;
import org.apache.druid.query.aggregation.first.StringFirstAggregatorFactory;
import org.apache.druid.segment.GenericColumnSerializer;
import org.apache.druid.segment.column.ColumnBuilder;
import org.apache.druid.segment.data.GenericIndexed;
Expand All @@ -34,6 +33,7 @@

import javax.annotation.Nullable;
import java.nio.ByteBuffer;
import java.util.Comparator;

/**
* The SerializablePairLongStringSerde serializes a Long-String pair (SerializablePairLongString).
Expand All @@ -46,6 +46,42 @@ public class SerializablePairLongStringSerde extends ComplexMetricSerde
{

private static final String TYPE_NAME = "serializablePairLongString";
private static final Comparator<SerializablePairLongString> COMPARATOR = (o1, o2) -> {
int comparation;

// First we check if the objects are null
if (o1 == null && o2 == null) {
comparation = 0;
} else if (o1 == null) {
comparation = -1;
} else if (o2 == null) {
comparation = 1;
} else {
Copy link
Member

Choose a reason for hiding this comment

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

nit, should we just wrap this comparator in Comparator.nullsFirst so that this part only has to deal with non-null pairs?

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 the pointer, re-wrote the comparators in a consice manner 👍

regarding the ingestion part, I've kept the ingestion comparator to be the same as before. Further, I'm not sure if we provide ability to order rows by metric values. In any case, I think the effect should only be at query time.


// If the objects are not null, we will try to compare using timestamp
comparation = o1.lhs.compareTo(o2.lhs);

// If both timestamp are the same, we try to compare the Strings
if (comparation == 0) {

// First we check if the strings are null
if (o1.rhs == null && o2.rhs == null) {
comparation = 0;
} else if (o1.rhs == null) {
comparation = -1;
} else if (o2.rhs == null) {
comparation = 1;
} else {

// If the strings are not null, we will compare them
// Note: This comparation maybe doesn't make sense to first/last aggregators
comparation = o1.rhs.compareTo(o2.rhs);
}
}
}

return comparation;
};

@Override
public String getTypeName()
Expand Down Expand Up @@ -87,7 +123,7 @@ public ObjectStrategy getObjectStrategy()
@Override
public int compare(@Nullable SerializablePairLongString o1, @Nullable SerializablePairLongString o2)
{
return StringFirstAggregatorFactory.VALUE_COMPARATOR.compare(o1, o2);
return COMPARATOR.compare(o1, o2);
}

@Override
Expand Down
34 changes: 14 additions & 20 deletions .../src/main/java/org/apache/druid/query/aggregation/first/StringFirstAggregatorFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ public void aggregate(ByteBuffer buf, int position)
((SerializablePairLongString) o2).lhs
);

// used in comparing aggregation results amongst distinct groups. hence the comparison is done on the finalized
// result which is string/value part of the result pair.
public static final Comparator<SerializablePairLongString> VALUE_COMPARATOR = (o1, o2) -> {
int comparation;

Expand All @@ -98,26 +100,18 @@ public void aggregate(ByteBuffer buf, int position)
} else if (o2 == null) {
comparation = 1;
} else {

// If the objects are not null, we will try to compare using timestamp
comparation = o1.lhs.compareTo(o2.lhs);

// If both timestamp are the same, we try to compare the Strings
if (comparation == 0) {

// First we check if the strings are null
if (o1.rhs == null && o2.rhs == null) {
comparation = 0;
} else if (o1.rhs == null) {
comparation = -1;
} else if (o2.rhs == null) {
comparation = 1;
} else {

// If the strings are not null, we will compare them
// Note: This comparation maybe doesn't make sense to first/last aggregators
comparation = o1.rhs.compareTo(o2.rhs);
}
// First we check if the strings are null
if (o1.rhs == null && o2.rhs == null) {
comparation = 0;
} else if (o1.rhs == null) {
comparation = -1;
} else if (o2.rhs == null) {
comparation = 1;
} else {

// If the strings are not null, we will compare them
// Note: This comparation maybe doesn't make sense to first/last aggregators
comparation = o1.rhs.compareTo(o2.rhs);
}
}

Expand Down
27 changes: 27 additions & 0 deletions ...ng/src/test/java/org/apache/druid/query/aggregation/first/StringFirstAggregationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import org.junit.Test;

import java.nio.ByteBuffer;
import java.util.Comparator;

public class StringFirstAggregationTest extends InitializedNullHandlingTest
{
Expand Down Expand Up @@ -215,6 +216,32 @@ public void testStringFirstAggregateCombiner()
Assert.assertEquals(pairs[1], stringFirstAggregateCombiner.getObject());
}

@Test
@SuppressWarnings("EqualsWithItself")
public void testStringLastAggregatorComparator()
{
Comparator<SerializablePairLongString> comparator =
(Comparator<SerializablePairLongString>) stringFirstAggFactory.getComparator();
SerializablePairLongString pair1 = new SerializablePairLongString(1L, "Z");
SerializablePairLongString pair2 = new SerializablePairLongString(2L, "A");
SerializablePairLongString pair3 = new SerializablePairLongString(3L, null);

// check non null values
Assert.assertEquals(0, comparator.compare(pair1, pair1));
Assert.assertTrue(comparator.compare(pair1, pair2) > 0);
Assert.assertTrue(comparator.compare(pair2, pair1) < 0);

// check non null value with null value (null values first comparator)
Assert.assertEquals(0, comparator.compare(pair3, pair3));
Assert.assertTrue(comparator.compare(pair1, pair3) > 0);
Assert.assertTrue(comparator.compare(pair3, pair1) < 0);

// check non null pair with null pair (null pairs first comparator)
Assert.assertEquals(0, comparator.compare(null, null));
Assert.assertTrue(comparator.compare(pair1, null) > 0);
Assert.assertTrue(comparator.compare(null, pair1) < 0);
}

private void aggregate(
Aggregator agg
)
Expand Down
27 changes: 27 additions & 0 deletions ...sing/src/test/java/org/apache/druid/query/aggregation/last/StringLastAggregationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import org.junit.Test;

import java.nio.ByteBuffer;
import java.util.Comparator;

public class StringLastAggregationTest
{
Expand Down Expand Up @@ -217,6 +218,32 @@ public void testStringLastAggregateCombiner()
Assert.assertEquals(pairs[1], stringFirstAggregateCombiner.getObject());
}

@Test
@SuppressWarnings("EqualsWithItself")
public void testStringLastAggregatorComparator()
{
Comparator<SerializablePairLongString> comparator =
(Comparator<SerializablePairLongString>) stringLastAggFactory.getComparator();
SerializablePairLongString pair1 = new SerializablePairLongString(1L, "Z");
SerializablePairLongString pair2 = new SerializablePairLongString(2L, "A");
SerializablePairLongString pair3 = new SerializablePairLongString(3L, null);

// check non null values
Assert.assertEquals(0, comparator.compare(pair1, pair1));
Assert.assertTrue(comparator.compare(pair1, pair2) > 0);
Assert.assertTrue(comparator.compare(pair2, pair1) < 0);

// check non null value with null value (null values first comparator)
Assert.assertEquals(0, comparator.compare(pair3, pair3));
Assert.assertTrue(comparator.compare(pair1, pair3) > 0);
Assert.assertTrue(comparator.compare(pair3, pair1) < 0);

// check non null pair with null pair (null pairs first comparator)
Assert.assertEquals(0, comparator.compare(null, null));
Assert.assertTrue(comparator.compare(pair1, null) > 0);
Assert.assertTrue(comparator.compare(null, pair1) < 0);
}

private void aggregate(
Aggregator agg
)
Expand Down