[CALCITE-3789] Support validation of UNNEST multiple array columns like Presto by my7ym · Pull Request #1811 · apache/calcite

my7ym · 2020-02-16T05:17:27Z

No description provided.

danny0405 · 2020-02-17T03:00:32Z

core/src/main/java/org/apache/calcite/sql/SqlUnnestOperator.java

  }

+  private boolean allowFlattenStruct(SqlOperatorBinding operatorBinding) {
+    if (!(operatorBinding instanceof SqlCallBinding)) {


allowFlattenStruct -> allowAliasUnnestArrayColumns ?

Another class type for operator binding is RexCallBinding, but why we return true for that ?

Great name. Much more concise. Will do.

Another class type for operator binding is RexCallBinding, but why we return true for that ?

This is the original behavior. Actually there are tons of SqlOperatorBinding implementations. As the original behavior, UNNEST will ALWAYS flatten the struct. The logic here is that we do NOT allow flatten struct ONLY for SqlCallBinding && conformance.allowAliasUnnestArrayColumns()

@danny0405 Took a second thought on the function name, the result of the function is actually whether disallow flatten struct in UNNEST. So maybe disallowFlattenStructInUnnest? Let me know. Thanks.

I know you only to support SqlCallBinding for unnest alias columns, but the logic seems weird, can you change
allowFlattenStruct => allowAliasUnnestColumns ?

I think it looks even more confusing. If you read the code carefully enough, it is NOT allowAliasUnnestColumns. If you prefer the name change, I think disallowAliasUnnestColumn is more appropriate.

Couldn’t you use !allowAliasUnnestColumns ?

That's a good idea.

Good idea. Will do

danny0405 · 2020-02-17T03:01:17Z

core/src/main/java/org/apache/calcite/sql/SqlUnnestOperator.java

  }

+  private boolean allowFlattenStruct(SqlOperatorBinding operatorBinding) {
+    if (!(operatorBinding instanceof SqlCallBinding)) {


Another class type for operator binding is RexCallBinding, but why we return true for that ?

danny0405 · 2020-02-17T03:05:24Z

core/src/main/java/org/apache/calcite/sql/validate/DelegatingScope.java

+            prefix.names,
+            nameMatcher,
+            validator.getConformance().allowAliasUnnestArrayColumns(),
+            resolved);


The code may works for your case, but you have changed a common sql identifier resolving logic with a special conformance, which seems not right way to go ~

The conformance switch logic should always happen in SqlValidator, we can pass along some switch flags into the scope if necessary.

Sorry a little confused here. Here I am directly using conformance as the flag. I feel like it's more concise than an explicit flag like

SqlValidatorImpl.java::
boolean allowAliasUnnestArrays = validator.getConformance().allowAliasUnnestArrayColumns;
DelegatingScope.setAllowAliasUnnestArrays(allowAliasUnnestArrays);

DeletatingScope.java::
Just replace validator.getConformance().allowAliasUnnestArrayColumns with the explicit flag.

And it seems using conformance in scope happens before. e.g. OrderByScope.

The current fix seems not right, the nested field can be seen if we set up the record type to StructKind.PEEK_FIELDS, for the test cases you gave:

sql("select e.ENAME\n" + "from dept_nested as d CROSS JOIN\n" + " UNNEST(d.employees) as t(e)") .withConformance(SqlConformanceEnum.PRESTO).columnType("VARCHAR(10) NOT NULL");

There t(e) table has record type with e as field, the e can be seen and if you change the nested employee type with code snippet:

final RelDataType empRecordType = typeFactory.builder() .add("EMPNO", intType) .add("ENAME", varchar10Type) .add("DETAIL", typeFactory.builder() .add("SKILLS", array(skillRecordType)).build()) .kind(StructKind.PEEK_FIELDS) // here is the line you can tweak .build();

So, in total, this line change is not necessary, the field access should not be controlled by the SqlConformance.

make sense. Will do

danny0405 · 2020-02-17T03:06:40Z

core/src/main/java/org/apache/calcite/sql/validate/SqlConformanceEnum.java

+  SQL_SERVER_2008,
+
+  PRESTO;



PRESTO is not really a db, it behaves more like a computation engine, i'm not sure if we should add a new conformance for it.

That's right. I am also debating on this. I could delete it or rename it. But I think the conformance is only for SQL string validation, so whether or not DB is kinda fine. But it's your call.

Presto has its own SQL parser & validator & type system, so it counts as a 'dialect' (conformance) for these purposes.

I'll add further comments to https://issues.apache.org/jira/browse/CALCITE-3789.

my7ym

Thanks @danny0405 for the timely and detailed review!

my7ym · 2020-02-17T16:48:59Z

core/src/main/java/org/apache/calcite/sql/SqlUnnestOperator.java

  }

+  private boolean allowFlattenStruct(SqlOperatorBinding operatorBinding) {
+    if (!(operatorBinding instanceof SqlCallBinding)) {


Great name. Much more concise. Will do.

my7ym · 2020-02-17T16:54:47Z

core/src/main/java/org/apache/calcite/sql/SqlUnnestOperator.java

  }

+  private boolean allowFlattenStruct(SqlOperatorBinding operatorBinding) {
+    if (!(operatorBinding instanceof SqlCallBinding)) {


Another class type for operator binding is RexCallBinding, but why we return true for that ?

This is the original behavior. Actually there are tons of SqlOperatorBinding implementations. As the original behavior, UNNEST will ALWAYS flatten the struct. The logic here is that we do NOT allow flatten struct ONLY for SqlCallBinding && conformance.allowAliasUnnestArrayColumns()

my7ym · 2020-02-17T17:02:00Z

core/src/main/java/org/apache/calcite/sql/validate/DelegatingScope.java

+            prefix.names,
+            nameMatcher,
+            validator.getConformance().allowAliasUnnestArrayColumns(),
+            resolved);


Sorry a little confused here. Here I am directly using conformance as the flag. I feel like it's more concise than an explicit flag like

SqlValidatorImpl.java::
boolean allowAliasUnnestArrays = validator.getConformance().allowAliasUnnestArrayColumns;
DelegatingScope.setAllowAliasUnnestArrays(allowAliasUnnestArrays);

DeletatingScope.java::
Just replace validator.getConformance().allowAliasUnnestArrayColumns with the explicit flag.

And it seems using conformance in scope happens before. e.g. OrderByScope.

my7ym · 2020-02-17T17:04:26Z

core/src/main/java/org/apache/calcite/sql/validate/SqlConformanceEnum.java

+  SQL_SERVER_2008,
+
+  PRESTO;



That's right. I am also debating on this. I could delete it or rename it. But I think the conformance is only for SQL string validation, so whether or not DB is kinda fine. But it's your call.

my7ym · 2020-02-19T05:22:30Z

@danny0405 Just a kindly reminder, could you please take another look when available? Thanks a lot!

my7ym · 2020-02-25T06:25:26Z

@danny0405 Could you help take another look at this PR? Thanks!

danny0405 · 2020-02-25T09:36:28Z

I'm planning to review again this weekend, very busy with the release work of Calcite 1.22 and also my own company work, sorry for that, Just like Julian said, add a new SqlConformance is really a big change, for "big" i mean, we should keep all the changes correct.

my7ym · 2020-02-26T01:43:07Z

I'm planning to review again this weekend, very busy with the release work of Calcite 1.22 and also my own company work, sorry for that, Just like Julian said, add a new SqlConformance is really a big change, for "big" i mean, we should keep all the changes correct.

Make sense. Just send a reminder and get a rough timeline. Thanks @danny0405

my7ym · 2020-03-04T04:21:21Z

@danny0405 @julianhyde Just a soft reminder on this PR. No rush. Thanks!

danny0405 · 2020-03-10T08:47:56Z

Reviewing now, may take some time ~

core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java

core/src/main/java/org/apache/calcite/sql/SqlDialectFactoryImpl.java

danny0405 · 2020-03-10T08:55:13Z

Thanks @my7ym , could you rebase you code to remove the unnecessary change ?

core/src/main/java/org/apache/calcite/sql/validate/SqlAbstractConformance.java

core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java

danny0405 · 2020-03-11T04:38:04Z

We should also add a test case for at least one SqlToRelConverterTest.

my7ym · 2020-03-11T05:11:52Z

We should also add a test case for at least one SqlToRelConverterTest.

Will do it in the future if needed. I am still unclear that how this should be represented as RelNode. (discussed in https://issues.apache.org/jira/browse/CALCITE-3787)

Is it OK for you?

EDIT: Some other reasons:

We still plan to communicate with Presto using SQL.
RelNode -> SqlNode is not quite mature I believe, but I may be wrong there.
The optimization enabled by RelNode is helpful. But not a high-pri since Presto will also optimize the internal execution plan.

core/src/test/java/org/apache/calcite/test/catalog/MockCatalogReaderSimple.java

danny0405 · 2020-03-14T00:34:12Z

We should also add a test case for at least one SqlToRelConverterTest.

Will do it in the future if needed. I am still unclear that how this should be represented as RelNode. (discussed in https://issues.apache.org/jira/browse/CALCITE-3787)

Is it OK for you?

EDIT: Some other reasons:

We still plan to communicate with Presto using SQL.

RelNode -> SqlNode is not quite mature I believe, but I may be wrong there.

The optimization enabled by RelNode is helpful. But not a high-pri since Presto will also optimize the internal execution plan.

Usually we have 2 kinds of use cases for Calcite:

Generates RelNode/RexNode to adapter other downstream planners
Generates RelNode/RexNode and unparse it as special SQL dialect to interpretation through JDBC.

Either way we should generates a working plan, that is why I suggest to add SqlToRel test. For more general, we should add operator test and SQL ITCase if we change an operator semantics. But for you case, I think a SqlToRelConverter test is okey.

my7ym · 2020-03-14T01:20:04Z

We should also add a test case for at least one SqlToRelConverterTest.

Will do it in the future if needed. I am still unclear that how this should be represented as RelNode. (discussed in https://issues.apache.org/jira/browse/CALCITE-3787)
Is it OK for you?
EDIT: Some other reasons:

We still plan to communicate with Presto using SQL.

RelNode -> SqlNode is not quite mature I believe, but I may be wrong there.

The optimization enabled by RelNode is helpful. But not a high-pri since Presto will also optimize the internal execution plan.

Usually we have 2 kinds of use cases for Calcite:

Generates RelNode/RexNode to adapter other downstream planners

Generates RelNode/RexNode and unparse it as special SQL dialect to interpretation through JDBC.

Either way we should generates a working plan, that is why I suggest to add SqlToRel test. For more general, we should add operator test and SQL ITCase if we change an operator semantics. But for you case, I think a SqlToRelConverter test is okey.

OK. I could do that for something like unnest(employees) as t(e), but I am not sure what's a valid plan for unnest(employees, admins) as t(e, a). Can I just add one test case for something like unnest(employees) as t(e)? Thanks.

my7ym · 2020-03-29T18:52:11Z

We should also add a test case for at least one SqlToRelConverterTest.

Will do it in the future if needed. I am still unclear that how this should be represented as RelNode. (discussed in https://issues.apache.org/jira/browse/CALCITE-3787)
Is it OK for you?
EDIT: Some other reasons:

We still plan to communicate with Presto using SQL.

RelNode -> SqlNode is not quite mature I believe, but I may be wrong there.

The optimization enabled by RelNode is helpful. But not a high-pri since Presto will also optimize the internal execution plan.

Usually we have 2 kinds of use cases for Calcite:

Generates RelNode/RexNode to adapter other downstream planners

Generates RelNode/RexNode and unparse it as special SQL dialect to interpretation through JDBC.

Either way we should generates a working plan, that is why I suggest to add SqlToRel test. For more general, we should add operator test and SQL ITCase if we change an operator semantics. But for you case, I think a SqlToRelConverter test is okey.

I have added tests for it. But it needs changes on SqlToRelConverter.

The change is because usually for STRUCT, Uncollect will flatten the type but for this feature, it should not. This is also aligned with the registered type during validation.

I am not sure whether this should be the best way to implement it. Let me know. Thanks.

EDIT: If you think this change is large unnecessarily because we don't need SqlToRel conversion right now), I am happy to revert the SqlToRel conversion changes.

my7ym · 2020-03-29T18:54:50Z

We should also add a test case for at least one SqlToRelConverterTest.

Done

core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java

my7ym · 2020-03-29T19:21:03Z

core/src/main/java/org/apache/calcite/rel/core/Uncollect.java

      if (field.getType() instanceof MapSqlType) {
        builder.add(SqlUnnestOperator.MAP_KEY_COLUMN_NAME, field.getType().getKeyType());
        builder.add(SqlUnnestOperator.MAP_VALUE_COLUMN_NAME, field.getType().getValueType());
      } else {


I am also not sure what will be the best way to handle MAP here.

core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java

core/src/main/java/org/apache/calcite/sql/validate/SqlConformance.java

julianhyde · 2020-04-06T15:59:28Z

core/src/main/java/org/apache/calcite/rel/core/Uncollect.java

+      boolean withOrdinality, List<String> fieldAliases) {
    super(cluster, traitSet, input);
    this.withOrdinality = withOrdinality;
+    this.fieldAliases = fieldAliases;


you should copy fieldAliases into an immutable list.

Do you think the logic would be more uniform if fieldAliases were empty, not null, in the usual case? I'm a big fan of empty lists.

Done. Usually I think null is a separate state to flag something other than empty. But here they seem to be the same and we have extra bonus to avoid NPE. Good call.

julianhyde · 2020-04-06T16:04:55Z

core/src/main/java/org/apache/calcite/sql/validate/SqlConformance.java

+   *
+   * <p>E.g. in UNNEST(a_array, b_array) AS T(a, b),
+   * a and b will be aliases of a_array and b_array
+   * respectively


What will be the behavior of that expression if allowAliasUnnestColumns is false?

What is the most logical place for this function inside SqlConformance? I bet it's not at the very end.

julianhyde · 2020-04-06T16:05:08Z

core/src/main/java/org/apache/calcite/sql/validate/SqlConformanceEnum.java

+
+  /** Conformance value that instructs Calcite to use SQL semantics
+   * consistent with Presto. */
+  PRESTO;


alphabetical

julianhyde · 2020-04-06T16:07:07Z

core/src/main/java/org/apache/calcite/sql/validate/SqlConformanceEnum.java

      return true;
    }
  }
+  public boolean allowAliasUnnestColumns() {


need space before this function, no space after.

core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java

julianhyde · 2020-04-06T16:13:40Z

core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java

+
+  private void convertUnnest(Blackboard bb, SqlCall from, List<String> fieldAliases) {
+    SqlCall call;
+    call = from;


assign on one line and make final? or eliminate a variable?

This is my bad. Done.

julianhyde · 2020-04-06T16:15:32Z

core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java

+      replaceSubQueries(bb, node, RelOptUtil.Logic.TRUE_FALSE_UNKNOWN);
+    }
+    final List<RexNode> exprs = new ArrayList<>();
+    final List<String> fieldNames = new ArrayList<>();


you don't need two separate arrays. RelBuilder.project will apply aliases if you have used RelBuilder.alias to wrap expressions in 'as'. Use project rather than projectNamed.

Done. btw this logic is there already before I extract it as a separate method. I usually avoid to touch unnecessary part as much as possible.

julianhyde · 2020-04-06T16:16:48Z

core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java

+        (null != bb.root) ? bb.root : LogicalValues.createOneRow(cluster);
+    relBuilder.push(child).projectNamed(exprs, fieldNames, false);
+
+    Uncollect uncollect =


it's about time we added RelBuilder.uncollect

core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java

my7ym

@danny0405 @julianhyde Danny's patch seems to resolve all the pending issues in this PR. Let me know if you folks have other feedbacks. If not, please accept it. Thanks!

core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java

core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java

core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java

danny0405 · 2020-04-22T01:35:20Z

If you think my fix is okey, can you check it in this patch, thanks ~

my7ym · 2020-04-22T04:13:47Z

If you think my fix is okey, can you check it in this patch, thanks ~

Left another comment. I thought you preferred to keep them separate so I did not merge them. Will do after that comment is resolved! Thanks!

danny0405 · 2020-04-26T06:55:06Z

I have fired a new fix at https://github.com/danny0405/calcite/tree/fix-3789, please check if you have time.

my7ym · 2020-04-26T21:18:33Z

I have fired a new fix at https://github.com/danny0405/calcite/tree/fix-3789, please check if you have time.

👍 Will do and let you know.

EIDT: Just two nit comments. Solid fix. Let me know if you want to update it or I could directly resolve them when I merge your fix. Thanks!

danny0405 · 2020-04-27T01:31:05Z

I have fired a new fix at https://github.com/danny0405/calcite/tree/fix-3789, please check if you have time.

👍 Will do and let you know.

EIDT: Just two nit comments. Solid fix. Let me know if you want to update it or I could directly resolve them when I merge your fix. Thanks!

Directly resolve them when you merge the fix, thanks for the contribution ~

…nny Chan)

my7ym · 2020-04-27T06:11:07Z

I have fired a new fix at https://github.com/danny0405/calcite/tree/fix-3789, please check if you have time.

👍 Will do and let you know.
EIDT: Just two nit comments. Solid fix. Let me know if you want to update it or I could directly resolve them when I merge your fix. Thanks!

Directly resolve them when you merge the fix, thanks for the contribution ~

Done. Not sure whether I should make the commit message & JIRA title the same. Feel free to modify either of them. Thanks!

This patch also add a new PRESTO conformance. Fix-up (by Danny): - Fix SqlTypeUtil#flattenRecordType to not append field index if there are no duplicates - Rename SqlConformance#allowAliasUnnestColumns to SqlConformance#allowAliasUnnestItems - Fix RelStructuredTypeFlattener to not generate flattenned field based on struct field - Promote SqlToRelConverter#convertFrom to allow specify field aliases - Add comment to RelBuilder#uncollect close apache#1811 Change-Id: Ibeaeda6f0007b5409afcb43e5aab3b878e0de89b

apache#343) This patch also add a new PRESTO conformance. Fix-up (by Danny): - Fix SqlTypeUtil#flattenRecordType to not append field index if there are no duplicates - Rename SqlConformance#allowAliasUnnestColumns to SqlConformance#allowAliasUnnestItems - Fix RelStructuredTypeFlattener to not generate flattenned field based on struct field - Promote SqlToRelConverter#convertFrom to allow specify field aliases - Add comment to RelBuilder#uncollect close apache#1811 Co-authored-by: Will Yu <wmy7ymw@gmail.com>

This patch also add a new PRESTO conformance. Fix-up (by Danny): - Fix SqlTypeUtil#flattenRecordType to not append field index if there are no duplicates - Rename SqlConformance#allowAliasUnnestColumns to SqlConformance#allowAliasUnnestItems - Fix RelStructuredTypeFlattener to not generate flattenned field based on struct field - Promote SqlToRelConverter#convertFrom to allow specify field aliases - Add comment to RelBuilder#uncollect close apache#1811 Change-Id: Ibeaeda6f0007b5409afcb43e5aab3b878e0de89b

apache#343) This patch also add a new PRESTO conformance. Fix-up (by Danny): - Fix SqlTypeUtil#flattenRecordType to not append field index if there are no duplicates - Rename SqlConformance#allowAliasUnnestColumns to SqlConformance#allowAliasUnnestItems - Fix RelStructuredTypeFlattener to not generate flattenned field based on struct field - Promote SqlToRelConverter#convertFrom to allow specify field aliases - Add comment to RelBuilder#uncollect close apache#1811 Co-authored-by: Will Yu <wmy7ymw@gmail.com>

danny0405 reviewed Feb 17, 2020

View reviewed changes

my7ym commented Feb 17, 2020

View reviewed changes

my7ym force-pushed the miao/alias-unnest-multiple-array-columns branch 2 times, most recently from f25f91d to 5aba5c4 Compare February 26, 2020 05:19

danny0405 force-pushed the master branch from a549342 to 9b7b631 Compare March 5, 2020 04:59

danny0405 reviewed Mar 10, 2020

View reviewed changes

core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java Outdated Show resolved Hide resolved

danny0405 reviewed Mar 10, 2020

View reviewed changes

core/src/main/java/org/apache/calcite/sql/SqlDialectFactoryImpl.java Show resolved Hide resolved

my7ym force-pushed the miao/alias-unnest-multiple-array-columns branch 2 times, most recently from 999fe1b to d48b2f1 Compare March 10, 2020 21:26

danny0405 reviewed Mar 11, 2020

View reviewed changes

core/src/main/java/org/apache/calcite/sql/validate/SqlAbstractConformance.java Show resolved Hide resolved

danny0405 reviewed Mar 11, 2020

View reviewed changes

core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java Outdated Show resolved Hide resolved

my7ym force-pushed the miao/alias-unnest-multiple-array-columns branch 2 times, most recently from 9e8d652 to 12c5af4 Compare March 12, 2020 00:38

danny0405 reviewed Mar 13, 2020

View reviewed changes

core/src/test/java/org/apache/calcite/test/catalog/MockCatalogReaderSimple.java Outdated Show resolved Hide resolved

hsyuan force-pushed the master branch from e42a79b to a0ef3c9 Compare March 29, 2020 16:52

my7ym force-pushed the miao/alias-unnest-multiple-array-columns branch from 12d30c5 to 667c43d Compare March 29, 2020 18:48

my7ym commented Mar 29, 2020

View reviewed changes

core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java Outdated Show resolved Hide resolved

my7ym commented Mar 29, 2020

View reviewed changes

my7ym force-pushed the miao/alias-unnest-multiple-array-columns branch from 667c43d to d1c6964 Compare March 29, 2020 19:24

danny0405 reviewed Apr 4, 2020

View reviewed changes

core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java Outdated Show resolved Hide resolved

julianhyde reviewed Apr 6, 2020

View reviewed changes

my7ym force-pushed the miao/alias-unnest-multiple-array-columns branch from d1c6964 to 1e212e1 Compare April 9, 2020 05:26

my7ym commented Apr 21, 2020

View reviewed changes

[CALCITE-3789] Introduce Presto-like alias unnest items (Will Yu & Da…

5ab8dee

…nny Chan)

my7ym force-pushed the miao/alias-unnest-multiple-array-columns branch from 1e212e1 to 5ab8dee Compare April 27, 2020 06:08

danny0405 approved these changes Apr 27, 2020

View reviewed changes

danny0405 added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Apr 27, 2020

danny0405 closed this in e44beba Apr 27, 2020

Conversation

my7ym commented Feb 16, 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!

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!

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

Choose a reason for hiding this comment

Uh oh!

my7ym commented Feb 19, 2020

Uh oh!

my7ym commented Feb 25, 2020

Uh oh!

danny0405 commented Feb 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

my7ym commented Feb 26, 2020

Uh oh!

my7ym commented Mar 4, 2020

Uh oh!

danny0405 commented Mar 10, 2020

Uh oh!

Uh oh!

Uh oh!

danny0405 commented Mar 10, 2020

Uh oh!

Uh oh!

Uh oh!

danny0405 commented Mar 11, 2020

Uh oh!

my7ym commented Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

danny0405 commented Mar 14, 2020

Uh oh!

my7ym commented Mar 14, 2020

danny0405 commented Feb 25, 2020 •

edited

Loading

my7ym commented Mar 11, 2020 •

edited

Loading

my7ym commented Mar 29, 2020 •

edited

Loading