Test cost-based decisions on TPCH, TPCDS queries and CBO improvements by findepi · Pull Request #11267 · prestodb/presto

findepi · 2018-08-14T07:30:53Z

No description provided.

rschlussel · 2018-08-16T14:34:07Z

presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java

This was renamed to PARTITIONED

rschlussel · 2018-08-16T16:15:17Z

presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/PlanNodeWithCost.java

Maybe extract JoinEnumerationResult from ReorderJoins instead.

+1. Or better use this class in ReorderJoins

The classes are simple and yet different -- join reordering result needs Optional<PlanNode>. IMO reusing single class wouldn't improve readability.
For now, i just moved this class as an private member of DetermineJoinDistributionType.

rschlussel · 2018-08-16T16:34:22Z

.../main/java/com/facebook/presto/sql/planner/iterative/rule/DetermineJoinDistributionType.java

This is hard to follow and I don't like that it relies on knowing what makes something shouldRepartition and mustBroadcast (i.e. when flipping will be a problem)
Instead try:

if(!mustBroadcast(joinNode)) { possibleJoinNodes.add(getJoinNodeWithCost(costProvider, joinNode.withDistributionType(PARTITIONED)) } if(!mustRepartition(joinNode)) { possibleJoinNodes.add(getJoinNodeWithCost(costProvider, joinNode.withDistributionType(REPLICATED)) } JoinNode flipped = joinNode.flipChildren(); if(!mustBroadcast(flipped)) { possibleJoinNodes.add(getJoinNodeWithCost(costProvider, flipped.withDistributionType(PARTITIONED)) } if(!mustRepartition(flipped)) { possibleJoinNodes.add(getJoinNodeWithCost(costProvider, flipped.withDistributionType(REPLICATED)) }

And then have

private static boolean mustRepartition(JoinNode node) { JoinNode.Type type = joinNode.getType(); return type == RIGHT || type == FULL }

rschlussel · 2018-08-16T16:38:13Z

.../main/java/com/facebook/presto/sql/planner/iterative/rule/DetermineJoinDistributionType.java

if(mustRepartition(node) || (!mustBroadcast(node) && joinDistributionType.canRepartition())) { return node.withDistributionType(PARTITIONED) } return node.withDistributionType(REPLICATED);

OR

private PlanNode getSyntacticOrderJoin(JoinNode node, Context context, JoinDistributionType joinDistributionType) { if (mustRepartition(node)) { return node.withDistributionType(PARTITIONED); } if (mustBroadcast(node)) { return node.withDistributionType(REPLICATED); } if (isAtMostScalar(node.getRight(), context.getLookup())) { return node.withDistributionType(REPLICATED); } if (joinDistributionType.canPartition()) { return node.withDistributionType(PARTITIONED); } return node.withDistributionType(REPLICATED); } private static boolean mustRepartition(JoinNode node) { // With REPLICATED, the unmatched rows from right-side would be duplicated. return node.getType() == RIGHT || node.getType() == FULL; } private static boolean mustBroadcast(JoinNode node) { return node.getCriteria().isEmpty(); }

rschlussel · 2018-08-16T16:44:38Z

.../main/java/com/facebook/presto/sql/planner/iterative/rule/DetermineJoinDistributionType.java

I think it's pretty confusing what shouldRepartition means. for some things it's "must" and for some things it's "can". As suggested above, I'd replace this with a mustRepartition method and put the other logic in the calling code in getSyntacticOrderJoin (we don't even care about those things for getCostBasedJoin).

i rewrite all the logic in DetermineJoinDistributionType according to your suggestions.

rschlussel · 2018-08-16T18:45:30Z

presto-main/src/main/java/com/facebook/presto/cost/CachingStatsProvider.java

why only joins? seems like it would be equally useful for other kinds of nodes. and in the future we might generate multiple plans that differ in things other than join nodes.

Determining whether plans describe the same relation is not an easy problem and for caching purposes, this needs to be computed efficiently. Note also, that we not always need to find out equivalent plans. Memo equivalence groups contain equivalent plans (today, only 1 plan, i know), without need for any computations.

rschlussel · 2018-08-16T18:58:32Z

presto-main/src/test/java/com/facebook/presto/cost/TestFilterStatsCalculator.java

remove? this looks left over from manual testing.

rschlussel · 2018-08-16T20:21:30Z

presto-main/src/main/java/com/facebook/presto/cost/FilterStatsCalculator.java

literalEncoder.toExpression() already returns the Expression if the value is an instanceof Expression, and a NullLiteral if it equals null.

I would get rid of the if statements and always return literalEncoder.toExpression(value,BOOLEAN). And add a visitNullLiteral to FilterExpressionStatsCalculatingVisitor.

literalEncoder.toExpression(value,BOOLEAN) returns CAST(NULL AS boolean) which cannot be handled as nicely as defining visitNullLiteral().

rschlussel · 2018-08-16T20:31:08Z

...-main/src/test/java/com/facebook/presto/sql/planner/optimizations/BaseCostBasedPlanTest.java

nit: s/join/JOIN/

rschlussel · 2018-08-16T20:40:52Z

...-main/src/test/java/com/facebook/presto/sql/planner/optimizations/BaseCostBasedPlanTest.java

I'm obviously not going through all the plan output files for tpc-h and tpc-ds so I'm just going to assume someone verified that the plans are good.

I have some concerns about this testing style though. It seems very brittle and frustrating to update for intentional changes. I like the idea of having a quick way to know whether you possibly broke tpch or tpcds, but I think it will become more trouble than it's worth.

I'm also not sure why this doesn't use the existing plan matching framework. Is it because this is simpler and someone autogenerated the result files?

I'm also a little bit afraid of s burden introduced by this PR. If something changes in the plan structure we would need to analyze all the failures, and fix over 9000 text files.

Or, as it will happen more likely - we will be just blindly regenerating the test results (we end up doing this for plan matching tests in product tests at Teradata). That kinda kills the purpose of a test.

I would really try to hard about whats the purpose of this test. And if the value worth the maintenance costs.

The plans are not well-covered by other existing test. We created this tests because for an innocent change we don't really know if it causes any (unintentional) plan changes or not and it's not always possible to run a change on a benchmarking cluster.

@kokosing @sopel39 what's your opinion?

Those tests are valuable as they allow us to catch real world regressions with supposedly innocent CBO/stats changes. Consider them as product tests that test CBO end-to-end. In general, any CBO changes should be benchmarked. We've seen changes that should improve stats calculations, but caused performance regressions.

Without framework here you would need to benchmark some set of queries to see if performance has changed. With this PR, you can focus on just a subset of queries and you can also validate if changed plans are rational. This greatly simplifies process of evaluating CBO modifications but also provides an automated way of catching potential regressions (we've been kicked with such case once).

One drawback though is that in order to validate performance changes one need to recreate real environment used for generating this plans. Currently it assumes 9 machines and we run actually benchmarks against them. So the question is how we provide standardized performance testbed so that we can validate CBO decisions against real query times.

I don't have a strong opinion. Perhaps you can keep them for now. Let's see how often will we need to change them in practice.

I would rather not have them here until it's easily actionable for people if they change. If I don't have a way to evaluate whether a plan change caused a regression, I'm just going to blindly change the results.

That's ok as long as you know what you're doing (you will know) and also, you know that you are doing this (you will know because this of this test).

but i wouldn't actually check for a regression because I wouldn't have the cluster set up to to do that, so it's just adding overhead. It's one (or 100) more things to change without really helping with identifying problems.

arhimondr · 2018-08-17T17:03:10Z

presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/PlanNodeWithCost.java

+1. Or better use this class in ReorderJoins

arhimondr · 2018-08-17T17:04:20Z

presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/PlanNodeWithCost.java

arhimondr · 2018-08-17T17:04:28Z

presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/PlanNodeWithCost.java

arhimondr · 2018-08-18T13:26:01Z

.../main/java/com/facebook/presto/sql/planner/iterative/rule/DetermineJoinDistributionType.java

Why not just node.getCriteria().isEmpty()? If there is no equi criteria - the type of join doesn't matter much.

You cannot replicate RIGHT join, otherwise unmatched RIGHT rows would be duplicated.

But if there is no equi criteria, how can you partition the RIGHT join? Should we fail saying that right non equi join is not supported?

it will get partitioned on "no symbols", i.e. a constant, single node.

arhimondr · 2018-08-18T13:30:30Z

.../main/java/com/facebook/presto/sql/planner/iterative/rule/DetermineJoinDistributionType.java

This is actually looks more like a should

Because it still can be executed as a repartitioned join (very skewed one though).

arhimondr · 2018-08-18T14:40:13Z

...-main/src/test/java/com/facebook/presto/sql/planner/optimizations/BaseCostBasedPlanTest.java

Files.asCharSource(file, UTF8).read()

arhimondr · 2018-08-18T14:43:16Z

...-main/src/test/java/com/facebook/presto/sql/planner/optimizations/BaseCostBasedPlanTest.java

Why path is not included?

It's used by IntelliJ to annotate test executions with @DataProvider

Yes, you are right

arhimondr · 2018-08-18T14:43:24Z

...-main/src/test/java/com/facebook/presto/sql/planner/optimizations/BaseCostBasedPlanTest.java

queryFile ?

arhimondr · 2018-08-18T14:44:14Z

...-main/src/test/java/com/facebook/presto/sql/planner/optimizations/BaseCostBasedPlanTest.java

The name of this method is confusion. How about generateQueryPlan, and change the parameter name queryFilePath

arhimondr · 2018-08-18T14:53:51Z

.../main/java/com/facebook/presto/sql/planner/iterative/rule/DetermineJoinDistributionType.java

Do we need to have a similar one for the SemiJoin?

Hm... we should. But why now?

Yeah, we can introduce it as a separate PR. Just in case you can have it stashed somewhere, we can include it here.

arhimondr · 2018-08-18T15:03:27Z

Please create a separate PR for Add Exchange before GroupId to improve Partial Aggregation.

Also i would recommend dropping the Add test for CBO decisions on TPC-H, TPC-DS queries (and the Introduce partitioning switch in TPC-H connector). IMFO this type of testing introduces more burden that value.

Also i would think twice about the Cache stats for equivalent join nodes. It introduces additional "brittleness" for a really little value.

arhimondr · 2018-08-18T15:05:08Z

@sopel39 could you please add more details to the Remove min/max intersection heuristics from equality stats calculator commit message?

findepi · 2018-08-18T19:52:16Z

Please create a separate PR for Add Exchange before GroupId to improve Partial Aggregation.

@arhimondr @rschlussel ok, but why so?
This is a cost-based (actually: stats-based) rule for some particular use of partial aggregations.
Also, it's relation to the rest of the PR is fundamental -- in some tpc-ds queries (two or three), ReoderJoins makes optimal decision (as long as joins are concerns), but causing a regression overall (because the plan above joins benefited from the fact the data was being repartitioned for the sake of the joins).

sopel39 · 2018-08-20T09:23:59Z

@arhimondr I think the description for Remove min/max intersection heuristics from equality stats calculator should be along the lines:

During TPCH/TPCDS benchmarking this optimization proved to performed worse that without it.
After investigation we found that cross-column correlation is not accounted for when applying
filtering for lo-max ranges (e.g: when filtering dimension table on date, we don't adjust row id
column range which is tightly correlated to date column). This caused great underestimation
of filtered rows.

findepi · 2018-08-20T14:00:13Z

AC

arhimondr · 2018-08-20T17:52:43Z

This is a cost-based (actually: stats-based) rule for some particular use of partial aggregations.
Also, it's relation to the rest of the PR is fundamental -- in some tpc-ds queries (two or three), ReoderJoins makes optimal decision (as long as joins are concerns), but causing a regression overall (because the plan above joins benefited from the fact the data was being repartitioned for the sake of the joins).

It has to be disabled by default at least. We do not enable other cost based rules by default not because we are not sure in rules correctness itself, but rather because our estimates might not be accurate enough.

Also the commit itself is huge. It might be easier to have it as a separate PR.

findepi · 2018-08-21T09:02:07Z

i will drop Add Exchange before GroupId to improve Partial Aggregation from this PR in the next squash/rebase.

Should I squash the fixups now?

arhimondr · 2018-08-21T14:23:51Z

Should I squash the fixups now?

Yup

findepi · 2018-08-28T09:04:42Z

squashed & rebased

findepi · 2018-08-28T10:01:25Z

i dropped Add Exchange before GroupId to improve Partial Aggregation (will be in separate PR) and Cache stats for equivalent join nodes commits.

rschlussel · 2018-08-28T20:32:20Z

...o-main/src/main/java/com/facebook/presto/sql/planner/optimizations/QueryCardinalityUtil.java

why get rid of the override?

i don't understand?

wow i totally misread that. you added it.

rschlussel

I still don't think we should have the plan tests for tpc-h and tpc-ds queries, but everything else looks good.

kokosing · 2018-08-29T06:47:37Z

I still don't think we should have the plan tests for tpc-h and tpc-ds queries, but everything else looks good.

This is great feature. Image you are changing some heuristic. To test your change you would need to run whole benchmark suite. Having tests you could easily find queries which are affected, so you can only tests them.

rschlussel · 2018-08-29T14:52:43Z

@kokosing I agree it's very useful if you're then going to run benchmarks. However, right now nobody other than you guys has the set up to run those benchmarks. That means people will just blindly change test files without getting any extra signal. That's why I feel that we shouldn't have this feature until there's a test set up the community can use.

rschlussel · 2018-08-29T14:54:44Z

And it will impact planner changes unrelated to CBO heuristics, which could become burdensome. However, I'm not going to hold up the PR for this. We can always re-evalute if we find it does become burdensome.

kokosing · 2018-08-30T06:23:00Z

That means people will just blindly change test files without getting any extra signal.

But at least we will be aware that plans got changed. We will know what change affect plans and what is not. It is very easy to regenerate these plans and do a review plan changes. Notice that plan format is much simpler and it is should not change so frequently like output of the EXPLAIN.

Formatted dates were added in fc5419d, but the reading code was dropped as "unused" during review process.

This is often necessary in tests to have VALUES filled with some data rows, otherwise `QueryCardinalityUtil` will deem VALUES as scalar, affecting testing conditions.

When join reordering is disabled, `ReorderJoins` doesn't select join distribution. This commit makes `DetermineJoinDistributionType` rule to make this decision on cost basis, if only table statistics are available.

During TPCH/TPCDS benchmarking this optimization proved to performed worse that without it. After investigation we found that cross-column correlation is not accounted for when applying filtering for lo-max ranges (e.g: when filtering dimension table on date, we don't adjust row id column range which is tightly correlated to date column). This caused great underestimation of filtered rows.

Since an optimizer rule may create new expressions (new filters, new joins) and call cost/stats estimation on that, `FilterStatsCalculator` should not assume that all expressions have been simplified earlier.

TPC-H connector has 2 tables with partitioning (`lineitem`, `orders`). By introducing a switch disabling this behavior, we can simulate tables that are not partitioned.

findepi · 2018-08-30T10:14:19Z

Let's postpone the discussion about merit of these tests until we have some real experience. Our (internal) experience was positive, but I acknowledge the mileage may vary.

I am going to merge this once tests pass.

arhimondr

Reviewed Add test for CBO decisions on TPC-H, TPC-DS queries. Everything else looks good.

arhimondr · 2018-08-31T00:26:01Z

presto-main/src/main/java/com/facebook/presto/util/MoreLists.java


+    public static <T> List<T> nElements(int n, IntFunction<T> function)
+    {
+        checkArgument(n >= 0);


add a message

arhimondr · 2018-08-31T00:27:21Z

presto-main/src/main/java/com/facebook/presto/util/MoreLists.java

                .collect(toImmutableList());
    }

+    public static <T> List<T> nElements(int n, IntFunction<T> function)


This one of these function that are being only "sometimes" reused even by the author itself. Someone who doesn't know that this very specific function particularly exist - never would try to lookup one, but instead will try to implement either a copy, or just inline the algorithm. I'm not even saying it is available only in a single module. I don't think it is worth adding such a functions.

arhimondr · 2018-08-31T00:40:09Z

...-main/src/test/java/com/facebook/presto/sql/planner/optimizations/BaseCostBasedPlanTest.java

+                            System.out.println(query.id);
+                        }
+                        catch (IOException e) {
+                            throw new RuntimeException(e);


UncheckedIOException

arhimondr · 2018-08-31T00:40:19Z

...-main/src/test/java/com/facebook/presto/sql/planner/optimizations/BaseCostBasedPlanTest.java

+            return Files.asCharSource(file.toFile(), UTF_8).read();
+        }
+        catch (IOException e) {
+            throw new RuntimeException(e);


UncheckedIOException

arhimondr · 2018-08-31T00:40:46Z

...-main/src/test/java/com/facebook/presto/sql/planner/optimizations/BaseCostBasedPlanTest.java

+
+    public static final class Query
+    {
+        private final String id;


nit: getters-settere even for the inner class are good

arhimondr · 2018-08-31T00:50:24Z

...-main/src/test/java/com/facebook/presto/sql/planner/optimizations/TestTpchCostBasedPlan.java

+    @Override
+    protected Path getExpectedJoinOrderingFile(String queryId)
+    {
+        return Paths.get(format("./presto-main/src/test/resources/tpch-join-ordering/%s.txt", queryId));


Ditto about path

This is one is more tricker. It is used for generation. It is easy to read from resources, but it is difficult to update them.

arhimondr · 2018-08-31T00:51:31Z

...pcds/src/test/java/com/facebook/presto/sql/planner/optimizations/TestTpcdsCostBasedPlan.java

+                .boxed()
+                .flatMap(i -> {
+                    String queryId = format("q%02d", i);
+                    if (i == 14 || i == 23 || i == 24 || i == 39) {


This looks like something very ugly

It looks, but this is how tpcds standard is defined. What do you propose to handle queries 14_1, 14_2, 23_1, 23_2...?

arhimondr · 2018-08-31T00:51:40Z

...pcds/src/test/java/com/facebook/presto/sql/planner/optimizations/TestTpcdsCostBasedPlan.java

+
+    private static Path tpcdsQuery(String queryId)
+    {
+        return Paths.get(format("./presto-benchto-benchmarks/src/main/resources/sql/presto/tpcds/%s.sql", queryId));


Ditto about paths

This is fixed

arhimondr · 2018-08-31T00:51:48Z

...pcds/src/test/java/com/facebook/presto/sql/planner/optimizations/TestTpcdsCostBasedPlan.java

+    @Override
+    protected Path getExpectedJoinOrderingFile(String queryId)
+    {
+        return Paths.get(format("./presto-tpcds/src/test/resources/tpcds-join-ordering/%s.txt", queryId));


Ditto about paths

It is needed to expected query plans generation.

arhimondr · 2018-08-31T00:54:41Z

...-main/src/test/java/com/facebook/presto/sql/planner/optimizations/BaseCostBasedPlanTest.java

+    private String generateQueryPlan(Path queryFile)
+    {
+        String sql = read(queryFile)
+                .replaceFirst(";", "")


Please just copy the queries here. After you have the queries in, introduce some convention, and remove all the weird methods for resolving the queries/results. Once you have them in a single folder, you can scan the classpath, run everything under this directory and compare it with the results stored in some other directory under the classpath.

It is about 125 queries which we modify from time to time. The goal was to test exactly what we benchmark. Once we copy them here it will be no longer guaranteed.

arhimondr · 2018-09-04T18:24:54Z

@findepi @kokosing I'm going to merge everything, but the last commit. You can address the comments for the last commit and merge it in separate.

arhimondr · 2018-09-04T18:43:34Z

Merged everything, but Add test for CBO decisions on TPC-H, TPC-DS queries

kokosing · 2018-10-09T07:36:17Z

Most of these was merged, remaining is superseded with: #11665

The idea was abandoned during prestodb#11267 review.

The idea was abandoned during prestodb/presto#11267 review.

findepi requested review from arhimondr, rschlussel and rschlussel-zz August 14, 2018 07:30

facebook-github-bot added the CLA Signed label Aug 14, 2018

findepi force-pushed the epic/cbo/pr/42 branch from fd9806a to e6f254e Compare August 14, 2018 08:09

findepi removed the request for review from rschlussel-zz August 14, 2018 12:34

findepi force-pushed the epic/cbo/pr/42 branch from e6f254e to 7e47c8a Compare August 14, 2018 13:47

findepi mentioned this pull request Aug 16, 2018

Propagate stats and cost in distributed explain plans #11268

Merged

rschlussel reviewed Aug 16, 2018

View reviewed changes

arhimondr reviewed Aug 18, 2018

View reviewed changes

findepi force-pushed the epic/cbo/pr/42 branch from 7e47c8a to 50305bb Compare August 20, 2018 13:57

findepi force-pushed the epic/cbo/pr/42 branch from 605cfe1 to 0f6ffb1 Compare August 28, 2018 09:04

findepi force-pushed the epic/cbo/pr/42 branch from 0f6ffb1 to fdea61c Compare August 28, 2018 09:59

findepi force-pushed the epic/cbo/pr/42 branch 2 times, most recently from f93ce09 to f5966cf Compare August 28, 2018 10:40

rschlussel reviewed Aug 28, 2018

View reviewed changes

rschlussel approved these changes Aug 28, 2018

View reviewed changes

findepi and others added 8 commits August 30, 2018 12:11

Accept human-readable dates as TPCH column stats

4ad1823

Formatted dates were added in fc5419d, but the reading code was dropped as "unused" during review process.

Create VALUES with row values

b629270

This is often necessary in tests to have VALUES filled with some data rows, otherwise `QueryCardinalityUtil` will deem VALUES as scalar, affecting testing conditions.

Make cost-based decision when selecting Join distribution type

8bd0e0d

When join reordering is disabled, `ReorderJoins` doesn't select join distribution. This commit makes `DetermineJoinDistributionType` rule to make this decision on cost basis, if only table statistics are available.

Extract isSingleValue(Expression) to simplify code

649b877

Do not assume expression normalization before FilterStatsCalculator

acb591e

Since an optimizer rule may create new expressions (new filters, new joins) and call cost/stats estimation on that, `FilterStatsCalculator` should not assume that all expressions have been simplified earlier.

Introduce partitioning switch in TPC-H connector

5c816b6

TPC-H connector has 2 tables with partitioning (`lineitem`, `orders`). By introducing a switch disabling this behavior, we can simulate tables that are not partitioned.

Add test for CBO decisions on TPC-H, TPC-DS queries

6653f68

findepi force-pushed the epic/cbo/pr/42 branch from f5966cf to 6653f68 Compare August 30, 2018 10:12

arhimondr requested changes Aug 31, 2018

View reviewed changes

findepi mentioned this pull request Sep 24, 2018

Optimize statistics aggregation for wide tables #11558

Closed

kokosing closed this Oct 9, 2018

kokosing mentioned this pull request Oct 9, 2018

Add test for CBO decisions on TPC-H, TPC-DS queries #11665

Merged

findepi deleted the epic/cbo/pr/42 branch October 9, 2018 09:29

kokosing pushed a commit to starburstdata/facebook-presto that referenced this pull request Dec 14, 2018

Remove TODO related to stats caching

696293e

The idea was abandoned during prestodb#11267 review.

kokosing pushed a commit to starburstdata/facebook-presto that referenced this pull request Dec 14, 2018

Remove TODO related to stats caching

cb29011

The idea was abandoned during prestodb#11267 review.

findepi added a commit to findepi/trino that referenced this pull request Jan 29, 2019

Remove TODO related to stats caching

ebe7108

The idea was abandoned during prestodb/presto#11267 review.

findepi added a commit to trinodb/trino that referenced this pull request Feb 1, 2019

Remove TODO related to stats caching

0c72574

The idea was abandoned during prestodb/presto#11267 review.

findepi added a commit to starburstdata/trino that referenced this pull request Feb 1, 2019

Remove TODO related to stats caching

2538edc

The idea was abandoned during prestodb/presto#11267 review.

Conversation

findepi commented Aug 14, 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!

rschlussel Aug 16, 2018 • 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!

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!

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

rschlussel Aug 16, 2018 •

edited

Loading

sopel39 commented Aug 20, 2018 •

edited

Loading