Collect column statistics on write by arhimondr · Pull Request #10617 · prestodb/presto

arhimondr · 2018-05-15T22:05:58Z

No description provided.

sopel39 · 2018-05-16T08:54:51Z

presto-main/src/main/java/com/facebook/presto/sql/planner/LocalExecutionPlanner.java

you could use https://docs.oracle.com/javase/8/docs/api/java/util/stream/Stream.html#peek-java.util.function.Consumer-
to validate values withing stream. Should be cleaner

sopel39 · 2018-05-16T08:55:37Z

presto-main/src/main/java/com/facebook/presto/sql/planner/LocalExecutionPlanner.java

When this can happen?

sopel39 · 2018-05-16T09:00:20Z

no analyze syntax yet, right? Will it be part of this PR?

arhimondr · 2018-05-16T13:37:52Z

no analyze syntax yet, right? Will it be part of this PR?

This PR is about collecting column statistics on WRITE. The ANALYZE statement will be introduced in a separate PR.

arhimondr · 2018-06-29T22:02:37Z

presto-hive/src/main/java/com/facebook/presto/hive/metastore/file/FileHiveMetastore.java

I just realized that what i'm doing here doesn't make much sense. I should handle a proper schema migration as we do in PageSource and PageSink. Please ignore this commit for now. I'm going to implement proper schema migration as a separate PR.

electrum · 2018-06-30T00:40:48Z

presto-spi/src/main/java/com/facebook/presto/spi/statistics/ColumnStatisticType.java

Or perhaps NUMBER_OF_TRUE_VALUES to be consistent with the other _VALUES constants

electrum · 2018-06-30T23:41:40Z

presto-spi/src/main/java/com/facebook/presto/spi/connector/ConnectorMetadata.java

Nit: "statistics" and end in period. Same for other method.

electrum · 2018-06-30T23:42:34Z

presto-spi/src/main/java/com/facebook/presto/spi/connector/ConnectorMetadata.java

No need to capitalize "insert" (we are describing the logical operation, not the SQL syntax)

electrum · 2018-06-30T23:43:43Z

presto-spi/src/main/java/com/facebook/presto/spi/statistics/TableStatisticsMetadata.java

I think it's cleaner to make this private and have a static empty() method so that callers do TableStatisticsMetadata.empty()

electrum · 2018-06-30T23:44:28Z

presto-spi/src/main/java/com/facebook/presto/spi/statistics/TableStatisticsMetadata.java

Copy the inner lists

this.groupingSets = unmodifiableSet(requireNonNull(groupingSets, "groupingSets is null").stream() .map(list -> unmodifiableList(new ArrayList<>(requireNonNull(list, "groupingSets list is null"))) .collect(toSet()));

electrum · 2018-07-01T01:00:53Z

presto-main/src/main/java/com/facebook/presto/sql/planner/LogicalPlanner.java

Nit: "statistic type"

electrum · 2018-07-01T01:01:05Z

presto-main/src/main/java/com/facebook/presto/sql/planner/LogicalPlanner.java

This can be removed now

electrum · 2018-07-01T01:01:35Z

presto-main/src/main/java/com/facebook/presto/sql/planner/LogicalPlanner.java

Could move this to the initializer

electrum · 2018-07-01T01:05:00Z

presto-main/src/main/java/com/facebook/presto/sql/planner/LogicalPlanner.java

LogicalPlanner is already fairly long. See if you can move these methods and result callers into a separate helper class (in a different file)

Extracted to StatisticsAggregationsPlanner

electrum · 2018-07-01T01:10:45Z

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

I tried to follow the planning bits but I don't understand this at all. Ask @martint, @kokosing or @sopel39 to review this commit.

electrum · 2018-07-01T16:53:34Z

presto-spi/src/main/java/com/facebook/presto/spi/statistics/ComputedStatistics.java

Should we enforce that these blocks all have exactly one value?

It might not be true for the advanced statistics.

electrum · 2018-07-01T16:56:11Z

presto-spi/src/main/java/com/facebook/presto/spi/statistics/ComputedStatistics.java

Should we enforce that all blocks have the same value count? Or make this a Page?

note: Page doesn't enforce that

electrum · 2018-07-01T16:59:39Z

presto-spi/src/main/java/com/facebook/presto/spi/statistics/ComputedStatistics.java

We need to copy the collection values. For example, this wouldn't work (both would have the same values):

Builder builder = new ComputedStatistics.Builder(...); builder.add(...); ComputedStatistics stats1 = builder.build(); builder.add(...); ComputedStatistics stats2 = builder.build();

or prevent builder from being used after .build() is called

Decided to go with a copy technique. We use it in the other places.

electrum · 2018-07-01T17:02:21Z

presto-main/src/main/java/com/facebook/presto/operator/NoOpOperator.java

How about naming this BlackHoleOperator? NoOp is a bit confusing (especially in query plans) since this is the /dev/null operator that consumes all input with no output. Let's add a Javadoc as well

/** * This operator consumes all input and produces no output. */

BlackHole could be confusing too, because it may refer to the connector name. DevNullOperator?

DoNothingOperator?

Let's call it DevNullOperator

electrum · 2018-07-01T17:11:08Z

presto-main/src/main/java/com/facebook/presto/operator/TableFinishOperator.java

Agreed. Since most of this commit is just refactoring to add the parameter, let's extract that to a separate commit before the execution part. Then this commit will be only the important part: using the stats in the Hive connector.

electrum · 2018-07-01T18:40:20Z

presto-main/src/main/java/com/facebook/presto/sql/planner/LocalExecutionPlanner.java

Nit: stream on previous line

electrum · 2018-07-01T18:40:29Z

presto-main/src/main/java/com/facebook/presto/sql/planner/LocalExecutionPlanner.java

Nit: else is redundant

electrum · 2018-07-01T18:41:52Z

presto-main/src/main/java/com/facebook/presto/sql/planner/LocalExecutionPlanner.java

Maybe add a empty() method to StatisticAggregationsDescriptor

electrum · 2018-07-01T18:42:31Z

presto-main/src/main/java/com/facebook/presto/sql/planner/LocalExecutionPlanner.java

Wrap arguments to be consistent with above

electrum · 2018-07-01T18:44:53Z

presto-main/src/test/java/com/facebook/presto/operator/TestTableWriterOperator.java

Nit: static import

electrum

Overall looks good. Can you add test coverage to TestHiveIntegrationSmokeTest that validates statistics after CREATE TABLE AS and INSERT for partitioned and non-partitioned? Product tests are hard to run and debug, so we'd like to see as much functionality as possible covered by the smoke test.

Have @martint, @kokosing or @sopel39 review the planning bits, especially the optimizer changes.

electrum · 2018-07-01T19:00:11Z

presto-spi/src/main/java/com/facebook/presto/spi/connector/ConnectorMetadata.java

Make these Collection since the ordering doesn't matter

Use Refactor -> Change Signature to do this easily

electrum · 2018-07-01T19:00:15Z

presto-spi/src/main/java/com/facebook/presto/spi/connector/ConnectorMetadata.java

electrum · 2018-07-01T19:02:48Z

presto-main/src/main/java/com/facebook/presto/operator/TableFinishOperator.java

Make this Collection here, too

electrum · 2018-07-01T19:04:20Z

presto-main/src/main/java/com/facebook/presto/metadata/Metadata.java

Make Collection here, too

electrum · 2018-07-01T19:10:21Z

presto-hive/src/test/java/com/facebook/presto/hive/util/TestStatistics.java

I think this method actually hurts readability, since it's not clear what is happening or which is the expected output. Try inlining it.

The idea of this method is to check that the merge is commutative. No idea how did i end up with a single assert within.

electrum · 2018-07-01T19:55:50Z

presto-hive/src/main/java/com/facebook/presto/hive/util/Statistics.java

I'd inline this since it's short and the variable doesn't add much value

electrum · 2018-07-01T19:56:47Z

presto-hive/src/main/java/com/facebook/presto/hive/util/Statistics.java

Maybe do this as

return MILLISECONDS.toSeconds(...)

add something like TODO #7122 here

electrum · 2018-07-01T19:59:01Z

presto-hive/src/main/java/com/facebook/presto/hive/util/Statistics.java

Nit: use first.getClass().getName() to avoid the "class " prefix from the toString() method

nit: This isn't strictly correct, since a you could have two classes implementing Comparable<SharedInterface>

The method was removed

electrum · 2018-07-01T20:00:18Z

presto-hive/src/main/java/com/facebook/presto/hive/util/Statistics.java

Might be better to use >= so that we return first on tie (even though they should be equal ... returning first seems more natural)

electrum · 2018-07-01T20:00:58Z

presto-hive/src/main/java/com/facebook/presto/hive/util/Statistics.java

Nit: add trailing comma

arhimondr · 2018-07-02T20:58:07Z

presto-hive/src/main/java/com/facebook/presto/hive/metastore/HiveColumnStatistics.java

Going to add the null checks as part of this commit

The fact that HiveBasicStatistics are represented as a partition parameters is metastore specific. Other metastore implementation must be free to store these statistics in any other way. Thus the ThriftMetastoreUtil class is a better place for those methods.

Return both, basic and column statistics at once. The fact that the basic statistics are serialized as table parameters is specific to the opensource HiveMetastore.

These methods are intended to be used for both basic statistics and column statistics

This method is needed to create partition values for statistics

connector/session/table properties

Sample output: https://gist.github.com/arhimondr/f32fca68e84ff098f67c84480f148d72

kokosing

I think we need to divide this PR into several smaller ones.

So far I reached the planner changes.

kokosing · 2018-07-03T06:00:36Z

presto-hive/src/main/java/com/facebook/presto/hive/metastore/thrift/ThriftHiveMetastore.java

-    public Set<ColumnStatisticsObj> getTableColumnStatistics(String databaseName, String tableName, Set<String> columnNames)
+    public Map<String, HiveColumnStatistics> getTableColumnStatistics(String databaseName, String tableName)
    {
+        Table table = getTable(databaseName, tableName)


this table won't be cached, every call to for statistics will retrieve a table even if table was already retrieved in current session (transaction)

Yes, but after that the statistics object will be cached, and no further calls will be needed. Also this is a constant cost. It doesn't increase with a number of partition we fetch the statistics for.

The other way around is to have a column list as an argument. But it makes the interface a way more harder to understand and complicates the caching layer.

Also I'm going to do the schema migration for the statistics. Similar as we do for the data reads and writes. This is when the table schema is changed, but the partition schema remains the same. Than it is unclear what column name to pass. Whether the column names stored in partition, or the column names stored in the table.

Yes, but after that the statistics object will be cached, and no further calls will be needed.

We ask for a table many times per query already.

What if you would accept a Table as parameter?

I thought about that. It will make the CachingHiveMetastore messy. It would be needed to pass the Table object over the caching layer without including it to the caching key.

kokosing · 2018-07-03T06:02:29Z

presto-hive/src/main/java/com/facebook/presto/hive/metastore/thrift/HiveMetastore.java

 */
 package com.facebook.presto.hive.metastore.thrift;

+import com.facebook.presto.hive.metastore.HiveColumnStatistics;


Please correct me if I am wrong. HiveMetastore operates on low level communication with metastore using hive objects, while the HiveMetastoreExtended is on top of that and it is using presto objects (mostly "wrappers" on these returned from HiveMetastore.

Here you are breaking the above contract, by returning Presto object directly from HiveMetastore. If a difference between HiveMetastore and HiveMetastoreExtended shrinks, then question arises what is the purpose of this separation, is it still valid, maybe we should just merge them?

CC: @electrum @findepi

Our internal connector overrides the HiveMetastore, and does not override the ExtendedHiveMetastore. To do not make a mess it was decided to move this abstraction down.

CC: @electrum @haozhun

kokosing · 2018-07-03T06:03:52Z

presto-hive/src/main/java/com/facebook/presto/hive/metastore/thrift/ThriftHiveMetastore.java

        }
        catch (NoSuchObjectException e) {
-            return ImmutableSet.of();
+            throw new TableNotFoundException(new SchemaTableName(databaseName, tableName));


it is not true, you got a table in 238

client.getTableColumnStatistics may throw it in case of a race

kokosing · 2018-07-03T06:04:07Z

presto-hive/src/main/java/com/facebook/presto/hive/metastore/thrift/ThriftHiveMetastore.java

-    public Map<String, Set<ColumnStatisticsObj>> getPartitionColumnStatistics(String databaseName, String tableName, Set<String> partitionNames, Set<String> columnNames)
+    public Map<String, Map<String, HiveColumnStatistics>> getPartitionColumnStatistics(String databaseName, String tableName, Set<String> partitionNames)
    {
+        Table table = getTable(databaseName, tableName)


same as above

The same answer + it might be replaced with a Partition once the schema evolution is implemented.

kokosing · 2018-07-03T06:09:50Z

presto-hive/src/main/java/com/facebook/presto/hive/metastore/CachingHiveMetastore.java

Yes it does. To me it is kind of bug you for which you can create a separate PR and merge right away.

kokosing · 2018-07-03T06:52:56Z

presto-spi/src/main/java/com/facebook/presto/spi/connector/ConnectorMetadata.java

        return Optional.of(new ConnectorNewTableLayout(partitioningHandle, partitionColumns));
    }

+    /**


can you please extract SPI changes to another commit?

kokosing · 2018-07-03T06:56:41Z

presto-spi/src/main/java/com/facebook/presto/spi/statistics/ColumnStatisticType.java

If we are going to change that in the near future, it would be better to change it now to limit number of changes in external connectors

kokosing · 2018-07-03T07:06:19Z

presto-main/src/main/java/com/facebook/presto/metadata/Metadata.java

@@ -30,6 +30,7 @@
 import com.facebook.presto.spi.security.GrantInfo;


I think I am missing tests like TestLogicalPlanner to see what plans are generated for CREATE and INSERT queries

I was thinking about such a test, but finally i decided to do not implement one. Statistic aggregations don't change the plan shape but just add additional output symbols.

Currently BasePlanTest uses the LocalQueryRunner with the TPCH connector installed. TPCH connector doesn't ask the engine to compute any statistics. So to do that i would have to change the TPCH connector to request statistics for some tables. Also i would need to have additional tables that request statistics to be pre-grouped.

I found that test to be too tedious for the value it introduces. What this test can actually verify, is that the planner creates the correct aggregations for given column-statistics pairs. The regression caused by modifying that code will be caught by the product/integration tests that have all possible aggregation for the all possible types covered. We just may spend a little bit longer trying to figure out what has happened.

kokosing · 2018-07-03T07:07:52Z

presto-main/src/main/java/com/facebook/presto/operator/NoOpOperator.java

BlackHole could be confusing too, because it may refer to the connector name. DevNullOperator?

kokosing · 2018-07-03T07:08:44Z

presto-main/src/main/java/com/facebook/presto/operator/TableFinishOperator.java

        private final int operatorId;
        private final PlanNodeId planNodeId;
        private final TableFinisher tableFinisher;
+        private final OperatorFactory statisticsAggregation;


s/statisticsAggregation/operatorFactory?

I think statisticsAggregation better describes what it is in the context of the TableFinishOperator

arhimondr · 2018-07-03T13:21:45Z

I think we need to divide this PR into several smaller ones.

It has been already reviewed as a whole. Separating it now will make a muddle.

arhimondr · 2018-07-03T17:46:15Z

I addressed all the comments from the refactor part (first 7 commits), and extracted them as a separate PR.

@electrum @kokosing Please have a final look:

#10972

findepi · 2018-07-13T13:48:50Z

presto-main/src/main/java/com/facebook/presto/metadata/Metadata.java

+    /**
+     * Describes statistics that must be collected for an existing table during the INSERT operation
+     */
+    TableStatisticsMetadata getInsertStatisticsMetadata(Session session, TableHandle tableHandle);


Do we need both getNewTableStatisticsMetadata and getInsertStatisticsMetadata?

Actually i don't think we need this. We can resolve the ConnectorTableMetadata in the planner, and use the getNewTableStatisticsMetadata.

I followed the approach used for the getInsertLayout, what seems to be extra in this situation. We are not going to have different statistics set for the INSERT and the CREATE TABLE cases.

I'm going to remove this method, and rename the getNewTableStatisticsMetadata to getStatisticsMetadata

upd: getStatisticsCollectionMetadata

findepi · 2018-07-13T13:50:54Z

presto-main/src/main/java/com/facebook/presto/sql/planner/LogicalPlanner.java

            List<String> columnNames,
-            Optional<NewTableLayout> writeTableLayout)
+            Optional<NewTableLayout> writeTableLayout,
+            TableStatisticsMetadata statistics)


statisticsMetadata, as in the callers

findepi · 2018-07-13T13:57:47Z

presto-main/src/main/java/com/facebook/presto/sql/planner/StatisticsAggregationPlanner.java

+
+    private static boolean isApproxDistinctAvailable(Type inputType)
+    {
+        return inputType.equals(BIGINT) || inputType.equals(DOUBLE) || isVarcharType(inputType) || inputType.equals(VARBINARY);


It would be better to ask FunctionRegistry about that. With hard-coded list of types, no-one will remember to update this (and no test will fail).

Also, this needs an update already due to #10899

Since we got the approx_distinct for all the types we need, i'm just going to remove this.

findepi · 2018-07-13T14:01:47Z

presto-main/src/main/java/com/facebook/presto/sql/planner/plan/StatisticAggregations.java

+        return groupingSymbols;
+    }
+
+    public Parts split(SymbolAllocator symbolAllocator, FunctionRegistry functionRegistry)


splitAggregations? createIntermediateAggregations?

createIntermediateAggregations

findepi · 2018-07-13T14:02:49Z

presto-main/src/main/java/com/facebook/presto/sql/planner/plan/StatisticAggregations.java

+                            Optional.empty()));
+        }
+        return new Parts(
+                new StatisticAggregations(intermediateAggregation.build(), groupingSymbols),


is intermediateAggregation actually intermediate or partial?
in Parts this is called partial.. which seems correct

I think the correct name is intermediate. In aggregation function declaration we have finalType and intermediateType.

Have a look at com.facebook.presto.sql.planner.plan.AggregationNode.Step enum.

PARTIAL creates intermediate results

INTERMEDIATE reduces intermediate results into intermediate results

FINAL reduces intermediate results into aggregation's final result.

(Obviously, INTERMEDIATE is optional.)

Thus, what you do here, is to create_partial_ aggregation + final aggregation.

findepi · 2018-07-13T14:59:05Z

...-hive/src/main/java/com/facebook/presto/hive/statistics/MetastoreHiveStatisticsProvider.java

        }
    }
+
+    private boolean isMinMaxSupportedForType(Type type)


this method looks like belonging to Move MIN/MAX related statistic utilities commit (it's now in Collect column statistics on table write: Hive Connector)

I removed this method

findepi · 2018-07-13T15:03:39Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveClientConfig.java

+    {
+        ENABLED,
+        ENABLED_FOR_MARKED_TABLES,
+        DISABLED


put DISABLED first, end all options with ,

I removed the enum

findepi · 2018-07-13T15:03:58Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveClientConfig.java

+    public enum CollectColumnStatisticsOnWriteOption
+    {
+        ENABLED,
+        ENABLED_FOR_MARKED_TABLES,


add a javadoc linking to com.facebook.presto.hive.HiveTableProperties#COLLECT_COLUMN_STATISTICS_ON_WRITE_ENABLED

I removed the enum

findepi · 2018-07-13T15:05:38Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java

        Optional<String> comment = Optional.ofNullable(table.get().getParameters().get(TABLE_COMMENT));

+        String collectColumnStatisticsOnWrite = table.get().getParameters().get(COLLECT_COLUMN_STATISTICS_ON_WRITE_ENABLED_KEY);
+        if (collectColumnStatisticsOnWrite != null && Boolean.valueOf(collectColumnStatisticsOnWrite)) {


I don't like Boolean.valueOf because it accepts "yes" but returns false

findepi · 2018-07-13T15:08:04Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveTableProperties.java

This appears to be presto-specific property, maybe prefix it with presto_?

arhimondr · 2018-07-16T02:45:16Z

Superseded by: #11054

facebook-github-bot added the CLA Signed label May 15, 2018

arhimondr force-pushed the column-hive-stats branch from 2cc5fee to b928550 Compare May 15, 2018 23:12

sopel39 reviewed May 16, 2018

View reviewed changes

arhimondr force-pushed the column-hive-stats branch 13 times, most recently from d938477 to 672e8d9 Compare May 22, 2018 20:58

arhimondr force-pushed the column-hive-stats branch 3 times, most recently from 47bf413 to 39d50d0 Compare May 29, 2018 16:57

arhimondr force-pushed the column-hive-stats branch 8 times, most recently from ed4eff9 to d7eb5fa Compare June 6, 2018 16:15

arhimondr force-pushed the column-hive-stats branch from fcb183a to 6ea0a10 Compare June 7, 2018 02:01

arhimondr commented Jun 29, 2018

View reviewed changes

electrum reviewed Jul 1, 2018

View reviewed changes

arhimondr force-pushed the column-hive-stats branch from ae4c717 to a1d2a34 Compare July 2, 2018 18:10

arhimondr commented Jul 2, 2018

View reviewed changes

arhimondr and others added 15 commits July 2, 2018 17:59

Return HiveColumnStatistics from the HiveMetastore interface

c1e3eb2

Change getStatistics signature for HiveMetastore interface

ace432c

Return both, basic and column statistics at once. The fact that the basic statistics are serialized as table parameters is specific to the opensource HiveMetastore.

Introduce updateStatistics methods in the HiveMetastore

253a5a5

These methods are intended to be used for both basic statistics and column statistics

Use updateStatistics methods to store basic statistics

029873e

Move MIN/MAX related statistic utilities

bef760c

Move createPartitionValues method to a utility class

b0e4405

This method is needed to create partition values for statistics

Collect column statistics on table write: Planner

8c85fd2

Collect column statistics on table write: Execution

4c8d740

Collect column statistics on table write: Hive Connector

6147083

Add properties for column statistics collect

a7bc650

connector/session/table properties

Collect column statistics on table write: Product Tests

0651392

Add information about collected statistcs to explain output

8914741

Sample output: https://gist.github.com/arhimondr/f32fca68e84ff098f67c84480f148d72

Collect column statistics on table write: Documentation

be35b5e

Migrate column statistics on drop and rename column

d27e219

arhimondr force-pushed the column-hive-stats branch from a1d2a34 to d27e219 Compare July 3, 2018 00:14

kokosing reviewed Jul 3, 2018

View reviewed changes

arhimondr mentioned this pull request Jul 3, 2018

Preliminary column statistics refactorings #10972

Merged

findepi reviewed Jul 13, 2018

View reviewed changes

arhimondr closed this Jul 16, 2018

arhimondr deleted the column-hive-stats branch July 16, 2018 02:45

		@@ -30,6 +30,7 @@
		import com.facebook.presto.spi.security.GrantInfo;

Conversation

arhimondr commented May 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sopel39 commented May 16, 2018

Uh oh!

arhimondr commented May 16, 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!

arhimondr Jul 14, 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!

electrum 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!

arhimondr Jul 14, 2018 •

edited

Loading