SQL compatible Null Handling Part 2 - Processing Layer and Druid-SQL changes by nishantmonu51 · Pull Request #5452 · apache/druid

nishantmonu51 · 2018-03-01T18:22:42Z

Part 2 of changes for SQL Compatible Null Handling -
This PR contains changes to improve handling of null values in druid by treating nulls as missing values which don’t actually exist. This will make it more compatible to SQL standard and help with integration with other existing BI systems which support ODBC/JDBC. (Detailed description in proposal - #4349)

Nullability for Aggregators/Metrics

As per the current implementation, most aggregators are coded to replace null values with default e.g. sum treats them as zeroes, min treats them as positive infinity, etc.
To match the new semantics and make aggregators nullable, where if an aggregator encounters only the null values the result will be null value following changes are made -

Aggrgator/BufferAggregator - Added a new method boolean isNull() which returns false by default. aggregators that support nullability can choose to override this and return true if the aggregated result is null.
Added a NullableAggregator/NullableBufferAggregator which is be used as a decorator for existing aggregator implementations to make them nullable. This will return null values if all the values aggregated are null, If any of the aggregated value is non-null, it will have same behavior as the delegate aggregator.
If new null behavior is enabled, aggregator factories will decorate the aggregators with NullableAggregator/NullableBufferAggregator.
Added a new abstract class NullableAggregatorFactory which helps in reducing duplicate code and decide to switch between nullable and non-nullable aggregators.
Cardinality and HyperUnique aggregators ignore null values and only count non-null values, If all the encountered values are null, the result will be 0. This is different from current behavior where null values are also counted.
Count aggregator - since count aggregator is not associated with any column and just a count of the number of rows, it has the same behavior as before.

Math Expressions Null Handling

For general calculations like sum, full expression is be considered as null if any of the components is null.
StringLiteral now supports null and and expressions containing non-quoted null is parsed as StringLiteral with null
Specifying a default value for null is supported by the use of NVL or IF clause to assign default values at query time.

Filtering on Null values

SelectorDimFilter currently specifies filtering on null values but the implementation assumes null and empty strings as equivalent. The implementation is changed to consider null and empty string differently. Generation of cache key for selectorDimFilter is also modified to support null.
InFilter/ExpressionFilter/LikeFilter are also modified to not treat nulls as empty strings.

Changes to Druid build-in SQL layer

NULL and empty strings are treated differently.
IS NULL and IS NOT NULL filters against null values instead of empty strings.
NVL, IFNULL and COALESCE work as per SQL standard.
count now skips null values and works as per SQL standard.

Backwards Compatibility

"druid.generic.useDefaultValueForNull" is added as a new config. The default value is set to true for backwards compatibility.
code places in query/ingestion where we converted emptyToNull or nullToEmpty now use NullHandlingHelper which only does the conversion if backwards compatibility is enabled.
Storage layer changes are backwards compatible for GenericIndexed/String columns.
For Numeric columns new V2 serde implementations are introduced which also supports serde of nullRowsBitmaps. Previous implementations are kept unchanged to read any existing segments.

Testing

Most of the unit tests that tested nulls are modified to have two branches of expected results to test new/old behavior both.
For CI have added new runs to test matrix to test new behavior for nulls.
To run the tests with new behavior run "-Ddruid.generic.useDefaultValueForNull=false"

This change is

fix more tests more test fixes fix more tests more test fixes Fix more tests

fix test failures more test fixes

fix format Fix test Fix tests more test fix more test failures fix formatting

remove unused import Uniformly set Calcite systemProperties in all tests formatting fix fix checkstyle extract CalciteSetSysteProperties at a common place

b-slim · 2018-03-02T17:57:29Z

Reviewed 45 of 167 files at r1.
Review status: 45 of 167 files reviewed at latest revision, 1 unresolved discussion.

processing/src/main/java/io/druid/guice/GuiceInjectors.java, line 45 at r1 (raw file):

        new PropertiesModule(Arrays.asList("common.runtime.properties", "runtime.properties")),
        new ConfigModule(),
        new NullHandlingModule(),

I thought this was done part of the previous patch?

Comments from Reviewable

nishantmonu51 · 2018-03-07T10:41:06Z

@jon-wei @gianm : Can one of you please review this ?

leventov · 2018-03-09T11:36:21Z

    // Result of any Binary expressions is null if any of the argument is null.
    // e.g "select null * 2 as c;" or "select null + 1 as c;" will return null as per Standard SQL spec.
-    if (NullHandling.sqlCompatible() && (leftVal.isNull() || rightVal.isNull())) {
+    if (NullHandling.sqlCompatible() && (leftVal.value() == null || rightVal.value() == null)) {


Why this change?

Reason is that for String ExprEval isNull method now checks that the expression can be parsed as a valid long/double/float or not instead of checking the nullability.
Have added a comment on the ExprEval isNull method that explains this change.

leventov · 2018-03-09T11:41:02Z

  public static boolean asBoolean(String x)
  {
-    return !Strings.isNullOrEmpty(x) && Boolean.valueOf(x);
+    return !NullHandling.isNullOrEquivalent(x) && Boolean.valueOf(x);


Could you implement this thing: #5278 (comment) in this PR?

sure, will add a checkstyle rule.

leventov · 2018-03-09T11:49:11Z

@@ -127,7 +128,7 @@ private abstract static class NumericExprEval extends ExprEval<Number>

    private NumericExprEval(Number value)


@Nullable

leventov · 2018-03-09T11:50:16Z

@@ -99,6 +97,9 @@ public Object value()
    return value;


This field should be annotated @Nullable.

leventov · 2018-03-09T11:51:19Z

@@ -153,7 +154,7 @@ private static class DoubleExprEval extends NumericExprEval
  {
    private DoubleExprEval(Number value)


@Nullable. @nishantmonu51 could you please self-review this PR and fix all missing @Nullable annotations in this PR?

Added @nullable to these places, I already did one pass and added @nullable at places, but its easy to miss the @nullable annotations, Do you have any easy way that i can use to check the missing @nullable annotations ?

IntelliJ usually highlights such places with yellow (e. g. when you return null from a method not annotated @Nullable, or check for non-null an argument not annotated @Nullable)

leventov · 2018-03-09T12:20:32Z

+    return NullHandling.replaceWithDefault() ? aggregator : new NullableBufferAggregator(aggregator, selector);
+  }
+
+  protected abstract BufferAggregator factorizeBuffered(


Move below ABSTRACT METHODS BELOW

leventov · 2018-03-09T12:21:06Z

+ * Implementations of {@link AggregatorFactory} which needs to Support Nullable Aggregations are encouraged
+ * to extend this class.
+ */
+public abstract class NullableAggregatorFactory extends AggregatorFactory


@ExtensionPoint

leventov · 2018-03-09T12:21:29Z

+    return getMaxIntermediateSize2() + (NullHandling.replaceWithDefault() ? 0 : Byte.BYTES);
+  }
+
+  // ---- ABSTRACT METHODS BELOW ------


Please add docs to those methods, for extension writers.

leventov · 2018-03-09T12:23:31Z

+ */
+public class NullableBufferAggregator implements BufferAggregator
+{
+  private static final byte IS_NULL_BYTE = (byte) 1;


I would switch 1 and 0 values. NULL = 0, NOT_NULL = 1 is more natural, plus comparisons with 0 (done below) are cheaper.

leventov · 2018-03-09T12:24:51Z

+/**
+ * The result of a NullableBufferAggregator will be null if all the values to be aggregated are null values or no values are aggregated at all.
+ * If any of the value is non-null, the result would be the aggregated value of the delegate aggregator.
+ * Note that the delegate aggregator is not required to perform check for isNull on the columnValueSelector as only non-null values


Please make a javadoc ref

b-slim · 2018-03-13T04:20:23Z

Reviewed 123 of 167 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 29 unresolved discussions, some commit checks failed.

processing/src/main/java/io/druid/query/aggregation/last/DoubleLastAggregatorFactory.java, line 154 at r2 (raw file):

            if (pair.lhs >= lastTime) {
              buf.putLong(position, pair.lhs);
              buf.putDouble(position + Long.BYTES, pair.rhs);

why are we changing this? I know it is the same size but not really needed.

processing/src/main/java/io/druid/query/filter/SelectorDimFilter.java, line 79 at r2 (raw file):

    byte[] extractionFnBytes = extractionFn == null ? new byte[0] : extractionFn.getCacheKey();

    return ByteBuffer.allocate(6 + dimensionBytes.length + valueBytes.length + extractionFnBytes.length)

5 ?

sql/src/main/java/io/druid/sql/calcite/planner/Calcites.java, line 108 at r2 (raw file):

  public static String escapeStringLiteral(final String s)
  {
    Preconditions.checkNotNull(s);

this will throw an exception, is that what should be done? the old code seems to be fine with nulls?

sql/src/main/java/io/druid/sql/calcite/planner/DruidOperatorTable.java, line 121 at r2 (raw file):

          .add(new UnaryPrefixOperatorConversion(SqlStdOperatorTable.NOT, "!"))
          .add(new UnaryPrefixOperatorConversion(SqlStdOperatorTable.UNARY_MINUS, "-"))
          .add(new UnaryFunctionOperatorConversion(SqlStdOperatorTable.IS_NULL, "isnull"))

why not == null or != null ?

Comments from Reviewable

nishantmonu51 · 2018-03-15T22:47:40Z

Review status: 128 of 194 files reviewed at latest revision, 29 unresolved discussions.

processing/src/main/java/io/druid/query/aggregation/last/DoubleLastAggregatorFactory.java, line 154 at r2 (raw file):

Previously, b-slim (Slim) wrote…

why are we changing this? I know it is the same size but not really needed.

fixed.

processing/src/main/java/io/druid/query/filter/SelectorDimFilter.java, line 79 at r2 (raw file):

Previously, b-slim (Slim) wrote…

5 ?

fixed.

sql/src/main/java/io/druid/sql/calcite/planner/Calcites.java, line 108 at r2 (raw file):

Previously, b-slim (Slim) wrote…

this will throw an exception, is that what should be done? the old code seems to be fine with nulls?

yes, we never expect null here now.

sql/src/main/java/io/druid/sql/calcite/planner/DruidOperatorTable.java, line 121 at r2 (raw file):

Previously, b-slim (Slim) wrote…

why not == null or != null ?

== and != are binary operators in SQL which when comparing nulls return false as per sql standard.
e.g. Select null == null; will return false
and Select isNull(null) will return true;

Comments from Reviewable

nishantmonu51 · 2018-03-15T22:48:25Z

@leventov @b-slim fixed review comments.

Will Add Unit tests in subsequent PR.

…ling-pr-2

nishantmonu51 · 2018-05-22T14:29:30Z

@leventov: Updated PR based on your comments, please check.

… UTs * Extract NullableGroupBycolumnSelectorStrategy - only used when null handling is enabled, fix failing UTs * fix test after making extracting groupby changes * Set AWS region in travis

nishantmonu51 · 2018-05-29T13:13:39Z

@leventov: Any more comments here ?
Can we get this merged if there are no more blockers ?

leventov · 2018-05-29T15:27:52Z

@nishantmonu51 I haven't finished my review. I will continue the review soon

leventov · 2018-05-30T14:51:43Z

      final DelimitedParser delegate = new DelimitedParser(
-          Strings.emptyToNull(delimiter),
-          Strings.emptyToNull(listDelimiter),
+              StringUtils.emptyToNullNonDruidDataString(delimiter),


Wrong indentation, and on the next line

leventov · 2018-05-30T14:54:25Z

    @Override
    public String call()
    {
+      // When SQL based null handling is disabled,


based -> compatible

leventov · 2018-05-30T14:59:30Z

    // Do some queries.
-    Assert.assertEquals(2000, sumMetric(task, null, "rows"));
-    Assert.assertEquals(2000, sumMetric(task, null, "met1"));
+    Assert.assertEquals(2000, sumMetric(task, null, "met1").longValue());


Why rows -> met1?

leventov · 2018-05-30T14:59:44Z

    // Do some queries.
-    Assert.assertEquals(3, sumMetric(task, null, "rows"));
-    Assert.assertEquals(3, sumMetric(task, null, "met1"));
+    Assert.assertEquals(3, sumMetric(task, null, "met1").longValue());


Why rows -> met1?

leventov · 2018-05-30T15:02:21Z

   * See io.druid.guice.NullHandlingModule for details.
   * It does not take effect in all unit tests since we don't use Guice Injection.
-   * For tests default system property is supposed to be used only in tests
+   * default system property is supposed to be used only in tests


This doc comment has proper sentences (starting with capital and ending with dot), please follow this within a comment.

leventov · 2018-06-02T17:16:26Z

+  @Override
+  public long getLong(ByteBuffer buf, int position)
+  {
+    if (isNull(buf, position)) {


leventov · 2018-06-02T17:16:33Z

+  @Override
+  public double getDouble(ByteBuffer buf, int position)
+  {
+    if (isNull(buf, position)) {


leventov · 2018-06-02T17:17:32Z

 import java.util.Objects;

-public abstract class SimpleDoubleAggregatorFactory extends AggregatorFactory
+public abstract class SimpleDoubleAggregatorFactory extends NullableAggregatorFactory


If NullableAggregatorFactory is generified, it's going to be extends NullableAggregatorFactory<BaseDoubleColumnValueSelector>

leventov · 2018-06-02T18:10:40Z

  {
    Assert.assertEquals(null, loadingLookup.apply(null));
-    Assert.assertEquals(null, loadingLookup.apply(""));
+    if (!NullHandling.replaceWithDefault()) {


This is NullHandling.sqlCompatible()

leventov · 2018-06-02T18:11:03Z

+        // SQL standard spec does not count null values,
+        // Skip counting null values when we are not replacing null with default value.
+        // A special value for null in case null handling is configured to use empty string for null.
+        if (!NullHandling.replaceWithDefault() && !hasNonNullValue && value != null) {


NullHandling.sqlCompatible()

…ling-pr-2

nishantmonu51 · 2018-06-08T03:34:08Z

@leventov: Please review. made NullableAggregatorFactory and NullableAggregatorCombiner generic and handled other comments.

…ling-pr-2

leventov · 2018-06-17T14:28:21Z

 import java.util.Objects;

-public class FloatFirstAggregatorFactory extends NullableAggregatorFactory
+public class FloatFirstAggregatorFactory extends NullableAggregatorFactory<ColumnValueSelector>


Why ColumnValueSelector rather than BaseFloatColumnValueSelector?

It returns a SerializablePair<Long, Float> instead of simple Float

The parameter of NullableAggregatorFactory is not the output selector type, it's the input selector type. So it should be BaseFloatColumnValueSelector for FloatFirstAggregatorFactory. The reason why this refactoring couldn't be done right now is that getCombiningFactory() returns an anonymous subclass of FloatFirstAggregatorFactory, that IMO wrong. It seems to me that it should be a separate class FloatFirstCombiningAggregatorFactory extends NullableAggregatorFactory<ColumnValueSelector<SerializablePair<Long, Float>>>

leventov · 2018-06-17T14:28:46Z

 import java.util.Objects;

-public class DoubleFirstAggregatorFactory extends NullableAggregatorFactory
+public class DoubleFirstAggregatorFactory extends NullableAggregatorFactory<ColumnValueSelector>


Why ColumnValueSelector rather than BaseDoubleColumnValueSelector?

It returns a SerializablePair<Long, Double> instead of simple Double

leventov · 2018-06-17T14:29:30Z

 import java.util.Objects;

-public class DoubleLastAggregatorFactory extends NullableAggregatorFactory
+public class DoubleLastAggregatorFactory extends NullableAggregatorFactory<ColumnValueSelector>


It returns a Pair instead of primitive object.

leventov · 2018-06-17T14:29:35Z

 import java.util.Objects;

-public class LongLastAggregatorFactory extends NullableAggregatorFactory
+public class LongLastAggregatorFactory extends NullableAggregatorFactory<ColumnValueSelector>


It returns a Pair instead of primitive object

leventov · 2018-06-17T14:29:48Z

 import java.util.Objects;

-public class FloatLastAggregatorFactory extends NullableAggregatorFactory
+public class FloatLastAggregatorFactory extends NullableAggregatorFactory<ColumnValueSelector>


It returns a Pair instead of primitive object

leventov · 2018-06-17T15:20:21Z

+
+  @Override
+  public void processValueFromGroupingKey(
+      GroupByColumnSelectorPlus selectorPlus, ByteBuffer key, Map<String, Object> resultMap, int keyBufferPosition


Separate lines, and below in this class

leventov · 2018-06-17T15:21:55Z

+    if (metricVal == null || value == null) {
+      return metricVal == null && value == null;
+    }
    return HavingSpecMetricComparator.compare(row, aggregationName, value, aggregators) == 0;


Could it optimize to not perform row.getRaw() twice?

leventov · 2018-06-17T15:22:12Z

+    if (metricVal == null || value == null) {
+      return false;
+    }
    return HavingSpecMetricComparator.compare(row, aggregationName, value, aggregators) > 0;


leventov · 2018-06-17T15:22:25Z

+    if (metricVal == null || value == null) {
+      return false;
+    }
    return HavingSpecMetricComparator.compare(row, aggregationName, value, aggregators) < 0;


leventov · 2018-06-17T15:22:46Z

  private Ordering<Row> metricOrdering(final String column, final Comparator comparator)
  {
-    return Ordering.from(Comparator.comparing((Row row) -> row.getRaw(column), Comparator.nullsLast(comparator)));
+    return Ordering.from(Comparator.comparing((Row row) -> row.getRaw(column), Comparator.nullsFirst(comparator)));


in case of SQL compatibility we need nullsFirst, have modified to not change the behavior for non-sql compatible case to older one.

Explain this in a code comment

…ling-pr-2

nishantmonu51 · 2018-06-23T00:01:39Z

@leventov : handled comments, do you have any more comments ?

nishantmonu51 · 2018-07-01T03:31:11Z

@leventov : have you finished your review ? can this be merged ?

leventov · 2018-07-05T12:56:13Z

-      return ((Double) o).compareTo((Double) o1);
-    }
-  };
+  private static final Comparator<Number> COMPARATOR = Comparator.nullsFirst(Comparator.comparingDouble(Number::doubleValue));


Line longer than 120 cols. Might also be in other similar classes, I didn't check.

leventov · 2018-07-05T12:58:32Z

+import javax.annotation.Nullable;
+
+/**
+ * The result of a NullableAggregator will be null if all the values to be aggregated are null values or no values are aggregated at all.


leventov · 2018-07-05T12:58:44Z

+/**
+ * The result of a NullableAggregator will be null if all the values to be aggregated are null values or no values are aggregated at all.
+ * If any of the value is non-null, the result would be the aggregated value of the delegate aggregator.
+ * Note that the delegate aggregator is not required to perform check for {@link BaseNullableColumnValueSelector#isNull()} on the selector as only non-null values


leventov · 2018-07-05T12:59:27Z

+  );
+
+  /**
+   * Creates an {@link AggregateCombiner} to fold rollup aggregation results from serveral "rows" of different indexes during


leventov · 2018-07-05T13:01:45Z

@nishantmonu51 after addressing latest comments please squash all commits and create a new PR, it's impossible to work with this one anymore.

nishantmonu51 · 2018-07-06T19:08:30Z

Closing in favor of #5958

nishantmonu51 added Feature Area - Batch Ingestion Area - Querying Area - SQL labels Mar 1, 2018

nishantmonu51 added this to the 0.13.0 milestone Mar 1, 2018

nishantmonu51 requested review from b-slim, gianm, jon-wei and leventov March 2, 2018 14:56

nishantmonu51 added 8 commits March 2, 2018 22:41

initial cherry-pick

1887f9f

review comment - aggregator method naming, avoid using Pair

51be891

More fixes

f1f655e

fix more tests more test fixes fix more tests more test fixes Fix more tests

druid-sql Null Handling

92b8451

fix test

fd570b3

fix test failures more test fixes

enable more tests

14a8148

Fix ExpressionFilterTest

ced5c24

fix format Fix test Fix tests more test fix more test failures fix formatting

fix tests

a5b6f9b

remove unused import Uniformly set Calcite systemProperties in all tests formatting fix fix checkstyle extract CalciteSetSysteProperties at a common place

nishantmonu51 force-pushed the null-handling-pr-2 branch from 1198058 to a5b6f9b Compare March 2, 2018 17:13

revert unintentional change

97f6114

leventov added the Design Review label Mar 9, 2018

leventov reviewed Mar 9, 2018

View reviewed changes

nishantmonu51 added 4 commits March 16, 2018 03:31

review comments

351aefb

Merge remote-tracking branch 'upstream/master' into null-handling-pr-2

a7418d7

review comments

15000cb

review comment - cache numeric val in StringExprEval

8ae2c97

nishantmonu51 added 3 commits May 22, 2018 17:44

More fixes to Druid Expressions found during testing

0fdecc4

Will Add Unit tests in subsequent PR.

Merge remote-tracking branch 'remotes/upstream/master' into null-hand…

06078ec

…ling-pr-2

review comments - Extract out null handling for GroupBySerdeHelpers

78e3b08

nishantmonu51 added 2 commits May 23, 2018 03:30

Extract NullableGroupBycolumnSelectorStrategy, Null Bytes fix failing…

a9e9179

… UTs * Extract NullableGroupBycolumnSelectorStrategy - only used when null handling is enabled, fix failing UTs * fix test after making extracting groupby changes * Set AWS region in travis

review comments

4f1f2ca

leventov added the Area - Null Handling label May 30, 2018

nishantmonu51 added the Development Blocker label May 31, 2018

leventov reviewed Jun 2, 2018

View reviewed changes

nishantmonu51 added 2 commits June 8, 2018 08:52

review comments

1c081f4

Merge remote-tracking branch 'remotes/upstream/master' into null-hand…

d9c557d

…ling-pr-2

Merge remote-tracking branch 'remotes/upstream/master' into null-hand…

d2c0084

…ling-pr-2

leventov requested changes Jun 17, 2018

View reviewed changes

nishantmonu51 added 3 commits June 18, 2018 16:33

review comments

00e14b7

Review comments

ef0e725

Merge remote-tracking branch 'remotes/upstream/master' into null-hand…

fc3b129

…ling-pr-2

nishantmonu51 added 2 commits June 22, 2018 17:02

add comments to code

cd8b124

Merge remote-tracking branch 'upstream/master' into null-handling-pr-2

983af33

leventov reviewed Jul 5, 2018

View reviewed changes

nishantmonu51 added 2 commits July 7, 2018 00:28

review comments

7074b69

Merge remote-tracking branch 'upstream/master' into null-handling-pr-2

cc0066d

nishantmonu51 closed this Jul 6, 2018

leventov mentioned this pull request Jul 14, 2018

Part 2 of changes for SQL Compatible Null Handling #5958

Merged

		@@ -127,7 +128,7 @@ private abstract static class NumericExprEval extends ExprEval<Number>

		private NumericExprEval(Number value)

		@@ -153,7 +154,7 @@ private static class DoubleExprEval extends NumericExprEval
		{
		private DoubleExprEval(Number value)

Conversation

nishantmonu51 commented Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Nullability for Aggregators/Metrics

Math Expressions Null Handling

Filtering on Null values

Changes to Druid build-in SQL layer

Backwards Compatibility

Testing

Uh oh!

b-slim commented Mar 2, 2018

Uh oh!

nishantmonu51 commented Mar 7, 2018

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!

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!

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!

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!

b-slim commented Mar 13, 2018

Uh oh!

nishantmonu51 commented Mar 15, 2018

Uh oh!

nishantmonu51 commented Mar 15, 2018

Uh oh!

nishantmonu51 commented May 22, 2018

Uh oh!

nishantmonu51 commented May 29, 2018

Uh oh!

leventov commented May 29, 2018

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!

nishantmonu51 commented Mar 1, 2018 •

edited

Loading