Support name mapping resolution for parquet #830

chenjunjiedada · 2020-03-08T09:55:11Z

This fixes #715.

rdsr · 2020-03-08T19:30:13Z

parquet/src/main/java/org/apache/iceberg/parquet/ReadConf.java

    if (filter != null) {
-      statsFilter = new ParquetMetricsRowGroupFilter(expectedSchema, filter, caseSensitive);
-      dictFilter = new ParquetDictionaryRowGroupFilter(expectedSchema, filter, caseSensitive);
+      NameMapping nameMapping = MappingUtil.create(ParquetSchemaUtil.convert(fileSchema));


I think the nameMapping should be taken as a parameter in Parquet as part of the builder and passed in from there

Also, if the nameMapping is not passed in and file schema does not have ids. We can infer the nameMapping, similar to what we've done here. We had done this for Avro #580

It makes sense if we build name mapping from the expected schema, while here I need to build it from file schema so that we can use the metrics correctly.

rdsr · 2020-03-08T19:32:39Z

parquet/src/main/java/org/apache/iceberg/parquet/ReadConf.java

-        ParquetSchemaUtil.pruneColumns(fileSchema, expectedSchema) :
-        ParquetSchemaUtil.pruneColumnsFallback(fileSchema, expectedSchema);
+            ParquetSchemaUtil.pruneColumns(fileSchema, expectedSchema) :
+            ParquetSchemaUtil.pruneColumnsByName(fileSchema, expectedSchema);


For Avro we assigned ids to fileSchema based on the name mapping and projected ids, which we derived from expected schema. [Set<Integer> projectedIds = TypeUtil.getProjectedIds(expectedSchema)] . This was piggy backed on column pruning code for Avro see - AvroSchemaUtil.pruneColumns

I'm unsure whether the same general approach can be applied here as well. @rdblue Thoughts?

Here is a reversed way. I build name mapping based on fileSchema, and then project ids for expected schema according to names. The reason is the metrics are built from parquet footer so we have to bind the expression to fileSchema instead of expected schema in ParquetMetricsRowGroupFilter and ParquetDictionaryRowGroupFilter.

chenjunjiedada · 2020-03-10T16:49:06Z

@rdsr, I think again about your comments, that should be doable. I will submit an update later.

chenjunjiedada · 2020-03-10T17:00:46Z

@rdsr, ready for another review.

api/src/main/java/org/apache/iceberg/types/TypeUtil.java

chenjunjiedada · 2020-03-13T01:00:41Z

@rdsr, any new comments? The new commit uses the way you suggest.

@rdblue, could you please help to take a look at your convenience as well?

rdsr · 2020-03-13T15:01:37Z

@chenjunjiedada Been a little busy lately. I'll try to find time to look into it next week. Hope that's ok.

chenjunjiedada · 2020-03-14T01:24:12Z

@rdsr no problem, take you time:)

parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java

rdblue · 2020-04-07T00:00:20Z

parquet/src/main/java/org/apache/iceberg/parquet/ParquetDictionaryRowGroupFilter.java

+      struct = schema.asStruct();
+    }
+
+    this.expr = Binder.bind(struct, Expressions.rewriteNot(expr), caseSensitive);


This class doesn't need to know about the name mapping. The mapping should be used to add IDs to the file schema so that most classes don't need to add specific support.

Look at how the other fallback happens using ParquetSchemaUtil.addFallbackIds. I think this should mimic that fallback in all cases.

The added unit test failed if we only use the ID fallback strategy. How about we keep both the original logic and the name mapping logic?

We need to keep both ID assignment strategies, but we don't need to change these filters. Just pass the file schema with assigned IDs into this like before.

I changed to pass typeWithIds.

Why is the name mapping still passed in here?

The evaluation build metadata mapping with BlockMetadata read from the footer which may contain columns that not in the file schema passed in, so I use name mapping here to filter out them. The new commit changed to use a try/catch block.

The Parquet schema will match exactly because the IDs were added to the file schema. Adding IDs doesn't change the names, so meta.getPath() will be the same one used by fileSchema.getType(...).

rdblue · 2020-04-07T00:00:32Z

parquet/src/main/java/org/apache/iceberg/parquet/ParquetMetricsRowGroupFilter.java

+
+  public ParquetMetricsRowGroupFilter withNameMapping(NameMapping newNameMapping) {
+    this.nameMapping = newNameMapping;
+    return this;


This file also should not be modified.

rdblue · 2020-04-07T00:01:23Z

parquet/src/main/java/org/apache/iceberg/parquet/ParquetReadSupport.java

    MessageType projection = ParquetSchemaUtil.hasIds(fileSchema) ?
        ParquetSchemaUtil.pruneColumns(fileSchema, expectedSchema) :
-        ParquetSchemaUtil.pruneColumnsFallback(fileSchema, expectedSchema);
+        ParquetSchemaUtil.pruneColumnsByName(fileSchema, expectedSchema, MappingUtil.create(expectedSchema));


This should not replace the existing fallback strategy. If the mapping is present, it should be used. Otherwise, the existing position-based fallback strategy should be used.

rdblue · 2020-04-07T00:02:12Z

parquet/src/main/java/org/apache/iceberg/parquet/ReadConf.java

  private final boolean reuseContainers;
  @Nullable
  private final Integer batchSize;
+  @Nullable


No need for this annotation. Looks like we should have removed it in the other PR as well.

rdblue · 2020-04-07T00:03:48Z

parquet/src/main/java/org/apache/iceberg/parquet/ReadConf.java

-
    boolean hasIds = ParquetSchemaUtil.hasIds(fileSchema);
-    MessageType typeWithIds = hasIds ? fileSchema : ParquetSchemaUtil.addFallbackIds(fileSchema);
+    this.nameMapping = nameMapping == null ? MappingUtil.create(expectedSchema) : nameMapping;


This is not the right place to infer a name mapping. This class should apply the name mapping if it exists, and use a position-based fallback otherwise.

rdblue · 2020-04-07T00:05:01Z

parquet/src/main/java/org/apache/iceberg/parquet/ReadConf.java

-          statsFilter.shouldRead(typeWithIds, rowGroup) &&
-              dictFilter.shouldRead(typeWithIds, rowGroup, reader.getDictionaryReader(rowGroup)));
+          statsFilter.shouldRead(fileSchema, rowGroup) &&
+              dictFilter.shouldRead(fileSchema, rowGroup, reader.getDictionaryReader(rowGroup)));


What was the purpose of this change?

I reverted this back to typeWithIds

rdblue · 2020-04-07T00:11:12Z

Thanks for working on this, @chenjunjiedada!

A few high-level things that I think need to be fixed:

This should not replace the existing fallback that assigns IDs based on position. If the name mapping is configured, that should be used instead. Both are options. Feel free to do this by adding a name mapping to the existing ParquetSchemaUtil methods. We can do the fallback choice there.
This also needs to update ParquetSchemaUtil.hasIds so that any ID causes it to return true, instead of any missing ID. This should be okay since files either have IDs or do not, so it is not a behavior change.
As few classes should change as possible. Fallback ID assignment already works with the row group filters, so I don't think that we need to alter them. (Unless I'm missing some test case where it's failing!)

chenjunjiedada · 2020-04-07T12:40:58Z

@rdblue, I addressed some of the comments. The remaining needs a double check.

The ID fallback way cannot work when the iceberg table schema is different from the parquet file schema as the original issue mentioned. Also, the use case in the unit test (import partial data of existing parquet files as new iceberg table) would lead to the following exception which is related to metric row group filters. Therefore I removed the ID fallback way. I 'm not very sure that is the common use case, but if the name mapping way could handle more cases correctly, then it would be better to keep that way unless there is any case that it cannot handle. Does that make sense to you?

java.lang.ClassCastException: java.lang.Integer cannot be cast to java.lang.CharSequence
	at org.apache.iceberg.types.Comparators$CharSeqComparator.compare(Comparators.java:180)
	at org.apache.iceberg.types.Comparators$NullSafeChainedComparator.compare(Comparators.java:146)
	at org.apache.iceberg.parquet.ParquetMetricsRowGroupFilter$MetricsEvalVisitor.lt(ParquetMetricsRowGroupFilter.java:202)
	at org.apache.iceberg.parquet.ParquetMetricsRowGroupFilter$MetricsEvalVisitor.lt(ParquetMetricsRowGroupFilter.java:85)
	at org.apache.iceberg.expressions.ExpressionVisitors$BoundExpressionVisitor.predicate(ExpressionVisitors.java:122)
	at org.apache.iceberg.expressions.ExpressionVisitors.visitEvaluator(ExpressionVisitors.java:321)
	at org.apache.iceberg.expressions.ExpressionVisitors.visitEvaluator(ExpressionVisitors.java:336)
	at org.apache.iceberg.parquet.ParquetMetricsRowGroupFilter$MetricsEvalVisitor.eval(ParquetMetricsRowGroupFilter.java:108)
	at org.apache.iceberg.parquet.ParquetMetricsRowGroupFilter$MetricsEvalVisitor.access$000(ParquetMetricsRowGroupFilter.java:85)
	at org.apache.iceberg.parquet.ParquetMetricsRowGroupFilter.shouldRead(ParquetMetricsRowGroupFilter.java:79)
	at org.apache.iceberg.parquet.ReadConf.<init>(ReadConf.java:96)
	at org.apache.iceberg.parquet.ParquetReader.init(ParquetReader.java:64)
	at org.apache.iceberg.parquet.ParquetReader.iterator(ParquetReader.java:74)
	at org.apache.iceberg.spark.source.RowDataReader.open(RowDataReader.java:155)
	at org.apache.iceberg.spark.source.RowDataReader.open(RowDataReader.java:117)
	at org.apache.iceberg.spark.source.BaseDataReader.next(BaseDataReader.java:79)
	at org.apache.spark.sql.execution.datasources.v2.DataSourceRDD$$anon$1.hasNext(DataSourceRDD.scala:49)
	at org.apache.spark.InterruptibleIterator.hasNext(InterruptibleIterator.scala:37)
	at org.apache.spark.sql.catalyst.expressions.GeneratedClass$GeneratedIteratorForCodegenStage1.sort_addToSorter_0$(Unknown Source)
	at org.apache.spark.sql.catalyst.expressions.GeneratedClass$GeneratedIteratorForCodegenStage1.processNext(Unknown Source)
	at org.apache.spark.sql.execution.BufferedRowIterator.hasNext(BufferedRowIterator.java:43)
	at org.apache.spark.sql.execution.WholeStageCodegenExec$$anonfun$13$$anon$1.hasNext(WholeStageCodegenExec.scala:636)
	at org.apache.spark.sql.execution.SparkPlan$$anonfun$2.apply(SparkPlan.scala:255)
	at org.apache.spark.sql.execution.SparkPlan$$anonfun$2.apply(SparkPlan.scala:247)
	at org.apache.spark.rdd.RDD$$anonfun$mapPartitionsInternal$1$$anonfun$apply$24.apply(RDD.scala:836)
	at org.apache.spark.rdd.RDD$$anonfun$mapPartitionsInternal$1$$anonfun$apply$24.apply(RDD.scala:836)
	at org.apache.spark.rdd.MapPartitionsRDD.compute(MapPartitionsRDD.scala:52)
	at org.apache.spark.rdd.RDD.computeOrReadCheckpoint(RDD.scala:324)
	at org.apache.spark.rdd.RDD.iterator(RDD.scala:288)
	at org.apache.spark.scheduler.ResultTask.runTask(ResultTask.scala:90)
	at org.apache.spark.scheduler.Task.run(Task.scala:123)
	at org.apache.spark.executor.Executor$TaskRunner$$anonfun$10.apply(Executor.scala:408)
	at org.apache.spark.util.Utils$.tryWithSafeFinally(Utils.scala:1360)
	at org.apache.spark.executor.Executor$TaskRunner.run(Executor.scala:414)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)

rdblue · 2020-04-07T17:52:40Z

parquet/src/main/java/org/apache/iceberg/parquet/MessageTypeToType.java

  }

-  private int getId(org.apache.parquet.schema.Type type) {
+  protected int getId(org.apache.parquet.schema.Type type) {


Why was this changed?

This is changed so that we can override getid and use that to detect whether a schema has ID or not.

I think it might be better to make a new visitor for this, rather than reusing MessageTypeToType. That maintains independence between classes that don't have the same purpose. It was easy before to use MessageTypeToType without modification, but there's no need to do that if it is no longer easy.

OK, I will add a new one.

rdblue · 2020-04-07T17:54:04Z

parquet/src/main/java/org/apache/iceberg/parquet/ParquetSchemaUtil.java

+        protected int getId(org.apache.parquet.schema.Type type) {
+          org.apache.parquet.schema.Type.ID id = type.getId();
+          if (id != null) {
+            throw new IllegalStateException("at least one ID exists");


It is fine if not everything has an ID. Columns without IDs will be ignored and should not cause a read to fail.

Did I misunderstand the comment? I thought you meant file schema either has all columns with IDs or without ID. Do you mean that a file schema can have some of the columns with IDs and some without?

This also needs to update ParquetSchemaUtil.hasIds so that any ID causes it to return true, instead of any missing ID. This should be okay since files either have IDs or do not, so it is not a behavior change.

Yes, we expect schemas to either have all IDs or none, but we don't need to fail if that assumption is violated. If we detect any ID in the schema, then we should trust the file's IDs. If there are no IDs, then we should infer them using a name mapping or position-based fallback, whatever was configured.

rdblue · 2020-04-07T18:00:16Z

The remaining needs a double check.

I don't see replies to my comments. Can you reply so I know what you want me to double-check and why?

The ID fallback way cannot work when the iceberg table schema is different from the parquet file schema as the original issue mentioned

Non-Iceberg table schemas are maintained either by position (like CSV) or by name (like JSON). The fallback strategy needs to match how schemas have been maintained for a table. We implemented position-based fallback because before Iceberg we maintained by position so we could rename fields and add fields, but not drop or reorder. Support for name mapping supports the name-based approach for maintaining a schema. Both approaches are valid.

You're right that some files are incompatible with position-based, but that just shows that you have to maintain a schema consistently. You can't use both position-based and name-based in a table.

chenjunjiedada · 2020-04-08T01:47:02Z

@rdblue , thanks for the detail, I meant the comments about the ID fallback way. I didn't take the CSV and JSON format into consideration. Now I understood the idea and will add ID fallback strategy back.

rdblue · 2020-04-08T15:56:34Z

Great, thank you @chenjunjiedada!

chenjunjiedada · 2020-04-08T16:58:22Z

@rdblue , I saw your comments while committing. You might want to see the new commit (with some rebasing commits.). I will address your lastest comments tomorrow.

rdblue · 2020-04-09T17:11:14Z

core/src/main/java/org/apache/iceberg/avro/Avro.java

    }

-    public ReadBuilder nameMapping(NameMapping newNameMapping) {
+    public ReadBuilder withNameMapping(NameMapping newNameMapping) {


@rdsr, just want to confirm that you're okay with this change? If it is going to cause you problems because you've already deployed it, we can go with the original.

rdblue · 2020-04-09T17:15:09Z

@chenjunjiedada, it looks like lots of places have been changed to use the original file schema, rather than the file schema with fallback IDs added. I don't think that is correct.

Can you revert those changes, or help me understand why they are needed?

chenjunjiedada · 2020-05-29T03:33:17Z

@rdblue , I addressed the latest two comments about using the passing name mapping and allow parquet type without IDs. Would you please take another look? Thanks for your patience!

chenjunjiedada · 2020-06-03T01:16:43Z

@rdblue , This is ready for another review. Thanks

chenjunjiedada · 2020-06-16T14:17:11Z

Merged again. @rdblue, would you please take another look?

parquet/src/main/java/org/apache/iceberg/parquet/ParquetMetricsRowGroupFilter.java

rdblue · 2020-06-16T17:13:22Z

parquet/src/main/java/org/apache/iceberg/parquet/VectorizedParquetReader.java

-      Function<MessageType, VectorizedReader<?>> readerFunc,
-      Expression filter, boolean reuseContainers, boolean caseSensitive, int maxRecordsPerBatch) {
+      InputFile input, Schema expectedSchema, ParquetReadOptions options, Function<MessageType,
+      VectorizedReader<?>> readerFunc, NameMapping nameMapping, Expression filter, boolean reuseContainers,


Please don't split types across lines. I think moving Function<MessageType to the previous line should be reverted.

rdblue · 2020-06-16T17:17:37Z

parquet/src/test/java/org/apache/iceberg/parquet/TestParquetSchemaUtil.java

+        .asStructType().fields());
+    NameMapping nameMapping = MappingUtil.create(schema);
+    MessageType messageType = ParquetSchemaUtil.convert(schema, "complex_schema");
+    MessageType typeWithIdsFromNameMapping = ParquetSchemaUtil.applyNameMapping(messageType, nameMapping);


There's a problem here: applyNameMapping could be a noop and this would still work because the conversion to Parquet and back preserves IDs. For Avro, there is a test utility to remove IDs from the schema so that we can test adding them back with the name mapping. I think it would make sense to take the Avro name mapping tests and adapt them for Parquet as well. That can be done in a follow-up, but this test does need to be fixed before committing.

Make sense to me, let me file an issue to record this.

rdblue · 2020-06-16T17:31:02Z

parquet/src/main/java/org/apache/iceberg/parquet/PruneColumns.java

-        hasChange = true;
+      Integer fieldId = getId(originalField);
+      if (fieldId != null) {
+        if (selectedIds.contains(fieldId)) {


Why is this not if (fieldId != null && selectedIds.contains(fieldId))?

The else case is used when a sub-field is projected by ID. So the question is whether a sub-field can be projected if its parents aren't mapped. I think we should allow it because it would be confusing to have a value mapped, but still get nulls because a parent is not.

Make sense to me. Updated.

rdblue · 2020-06-16T17:31:17Z

parquet/src/main/java/org/apache/iceberg/parquet/PruneColumns.java

-        filteredFields.add(originalField);
-        hasChange = true;
+      Integer fieldId = getId(originalField);
+      if (fieldId != null) {


Same logic as above applies here.

And in the rest of the updates for this file.

rdblue · 2020-06-16T17:32:21Z

spark/src/main/java/org/apache/iceberg/spark/data/SparkParquetReaders.java

+          int id = fieldType.getId().intValue();
+          readersById.put(id, ParquetValueReaders.option(fieldType, fieldD, fieldReaders.get(i)));
+          typesById.put(id, fieldType);
+        }


Looks correct to me.

rdblue · 2020-06-16T17:33:44Z

spark/src/main/java/org/apache/iceberg/spark/source/BatchDataReader.java

+          .reuseContainers();
+
+      iter = nameMapping != null ?
+          builder.withNameMapping(NameMappingParser.fromJson(nameMapping)).build() : builder.build();


I'd prefer to keep the name mapping and building separate. There's no need to mix these together.

if (nameMapping != null) { builder.withNameMapping(NameMappingParser.fromJson(nameMapping)); } iter = builder.build();

rdblue · 2020-06-16T17:35:46Z

spark/src/test/java/org/apache/iceberg/spark/source/TestSparkTableUtil.java

  }
+
+  @Test
+  public void testImportWithNameMapping() throws Exception {


Can you add a test for name mapping with the vectorized read path? Just set the property on the table when you add the mapping and it should take that path.

Done, also updated the vectorized reader builder. Thanks for catching this

spark/src/test/java/org/apache/iceberg/spark/source/TestSparkTableUtil.java

rdblue · 2020-06-16T17:36:58Z

spark/src/main/java/org/apache/iceberg/spark/source/RowDataReader.java

+        .caseSensitive(caseSensitive);
+
+    return nameMapping != null ?
+        builder.withNameMapping(NameMappingParser.fromJson(nameMapping)).build() : builder.build();


Same here, let's separate the name mapping from build.

rdblue · 2020-06-16T17:38:16Z

parquet/src/main/java/org/apache/iceberg/parquet/MessageTypeToType.java

  }

-  protected int nextId() {
+  private int nextId() {


I'd also prefer to revert this change. There may not be a use of it as a protected method any more, but we don't need to change this file and risk git conflicts.

rdblue · 2020-06-16T17:38:31Z

parquet/src/main/java/org/apache/iceberg/parquet/ApplyNameMapping.java

+
+  @Override
+  public void beforeRepeatedElement(Type element) {
+  }


Can you add comments to these methods to explain why they are here?

OK, let me add the comments back.

rdblue · 2020-06-16T17:40:49Z

@chenjunjiedada, looks like there are blockers with the new logic in PruneColumns and the test in TestParquetSchemaUtil. Otherwise, I think we can get this in. Please also fix the minor issues and style. Thanks!

rdblue · 2020-06-17T16:56:13Z

Looks good! I would still like to fix the tests that don't remove IDs, but I'm going to merge this since it is a big commit to keep updating and to review. Thanks for all your work on this, @chenjunjiedada!

…pache#830)

chenjunjiedada force-pushed the namemapping branch 2 times, most recently from dd7fb25 to b3d2f4e Compare March 8, 2020 12:50

rdsr reviewed Mar 8, 2020

View reviewed changes

chenjunjiedada commented Mar 11, 2020

View reviewed changes

api/src/main/java/org/apache/iceberg/types/TypeUtil.java Outdated Show resolved Hide resolved

chenjunjiedada force-pushed the namemapping branch from 71189cd to e7a0ecb Compare March 19, 2020 07:54

rdblue reviewed Apr 6, 2020

View reviewed changes

parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java Show resolved Hide resolved

rdblue reviewed Apr 7, 2020

View reviewed changes

chenjunjiedada force-pushed the namemapping branch 3 times, most recently from 7af5eda to 6190877 Compare April 8, 2020 16:53

rdblue reviewed Apr 9, 2020

View reviewed changes

allow types without IDs

9529f0d

remove unrelated changes

526e79b

chenjunjiedada added 2 commits June 16, 2020 10:24

Merge branch 'master' into namemapping

a128fa7

resolve merge conflicts

18a803f

rdblue reviewed Jun 16, 2020

View reviewed changes

parquet/src/main/java/org/apache/iceberg/parquet/ParquetMetricsRowGroupFilter.java Outdated Show resolved Hide resolved

rdblue reviewed Jun 16, 2020

View reviewed changes

spark/src/test/java/org/apache/iceberg/spark/source/TestSparkTableUtil.java Show resolved Hide resolved

rdblue reviewed Jun 16, 2020

View reviewed changes

chenjunjiedada added 2 commits June 17, 2020 11:18

address comments

2f47631

fix style

3ca14c7

chenjunjiedada mentioned this pull request Jun 17, 2020

Apply avro name mapping unit tests to parquet #1121

Closed

rdblue merged commit 5a3cd22 into apache:master Jun 17, 2020

chenjunjiedada deleted the namemapping branch June 17, 2020 23:22

chenjunjiedada mentioned this pull request Jun 19, 2020

Fix parquet name mapping unit test #1128

Merged

rdblue added this to the Java 0.9.0 Release milestone Jul 10, 2020

cmathiesen pushed a commit to ExpediaGroup/iceberg that referenced this pull request Aug 19, 2020

Parquet: Support name mappings to recover field IDs (apache#830)

1309c98

sunchao pushed a commit to sunchao/iceberg that referenced this pull request May 10, 2023

Internal: Close the original parquet file reader when boson is true (a…

9696739

…pache#830)

Support name mapping resolution for parquet #830

Support name mapping resolution for parquet #830

Uh oh!

Conversation

chenjunjiedada commented Mar 8, 2020

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!

rdsr Mar 8, 2020 • 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!

chenjunjiedada commented Mar 10, 2020

Uh oh!

chenjunjiedada commented Mar 10, 2020

Uh oh!

Uh oh!

chenjunjiedada commented Mar 13, 2020

Uh oh!

rdsr commented Mar 13, 2020

Uh oh!

chenjunjiedada commented Mar 14, 2020

Uh oh!

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!

rdblue commented Apr 7, 2020

Uh oh!

chenjunjiedada commented Apr 7, 2020

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!

chenjunjiedada Apr 9, 2020 • 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!

chenjunjiedada Apr 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

rdsr Mar 8, 2020 •

edited

Loading

chenjunjiedada Apr 9, 2020 •

edited

Loading

chenjunjiedada Apr 8, 2020 •

edited

Loading