Allow Phoenix to replace partial topn with limit. #8171

lhofhansl · 2021-06-01T23:03:19Z

Adds Phoenix support for #6634 to Phoenix

This requires Phoenix 4.16.1 and Phoenix 5.1.2 on the server side to avoid memory overestimation.
4.16.1 is out, Phoenix 5.1.2 will be released soon, this PR should wait until that release.

No Phoenix client updates are needed.

lhofhansl · 2021-06-02T00:18:43Z

Shame on me for not running the Phoenix tests.

lhofhansl · 2021-06-02T00:24:21Z

Turns out for the tests I do need update the client. (I had only tested with real server instances).

That means that for the phoenix 4.x code I can't make this change, since I cannot change the client to 4.16.1 as it is not guaranteed to be compatible with older 4.x version. It's currently using the 4.14.1 which forward compatible to later 4.x version.

So new proposal. The Phoenix 4.x connector does not get this change. The Phoenix 5 connector get this change once 5.1.2 and I will change the Phoenix client version in the connector to 5.1.2 in a separate PR first. (5.1.2 is backward compatible with 5.1.1 and 5.1.0).

I'll wait until 5.1.2 is out, update the client to that in separate, then rebase this one, and remove the change from the 4.x connector.

lhofhansl · 2021-06-02T18:45:08Z

@findepi Is there a way to leave the Phoenix dependency at 4.14.1, but for tests use 4.16.1 for the server part? (likely not worth the effort, but thought I'd check)

findepi · 2021-06-07T14:04:00Z

Can we run Phoenix in a docker container?

lhofhansl · 2021-06-08T18:35:12Z

Rebased after the change to Phoenix 5.1.2.

@findepi Re: Docker. Probably. Lemme see if can make some for that. Until then we do this optimization (only that, not a correctness issue) for Phoenix 5 only.

raunaqmorarka · 2021-06-08T18:48:25Z

plugin/trino-phoenix5/src/main/java/io/trino/plugin/phoenix5/PhoenixClient.java

@@ -293,16 +292,7 @@ public boolean supportsTopN(ConnectorSession session, JdbcTableHandle handle, Li
    @Override
    protected Optional<TopNFunction> topNFunction()
    {
-        return Optional.of((query, sortItems, limit) -> {


This looks like it belongs in a different commit

This is needed so that topN is always handled by the connector. If the connector sometimes does it, and it is not apparent from the plan, I cannot do the push down.
That said, happy to break this into a separate commit, if that is better.

if ttps://issues.apache.org/jira/browse/PHOENIX-6436 mentioned below is resolved (and shipped), we can address the TODO. It's fine to have this as a prep commit.

That said, happy to break this into a separate commit, if that is better.

please do

plugin/trino-phoenix5/src/main/java/io/trino/plugin/phoenix5/PhoenixMetadata.java

lhofhansl · 2021-06-09T00:32:17Z

Enabled the SUPPORTS_TOPN_PUSHDOWN behavior in the test. Let's wait for a test run.

lhofhansl · 2021-06-09T00:47:45Z

Forgoet that SUPPORTS_TOPN_PUSHDOWN is different.

plugin/trino-phoenix5/src/main/java/io/trino/plugin/phoenix5/PhoenixMetadata.java

lhofhansl · 2021-06-09T04:48:50Z

What I don't understand is why this test is now failing:

[ERROR] Failures: 
2021-06-09T01:13:42.8011054Z [ERROR]   TestPhoenixConnectorTest>BaseJdbcConnectorTest.testTopNPushdown:629 Plan does not match, expected [
2021-06-09T01:13:42.8012257Z 
2021-06-09T01:13:42.8013551Z - anyTree
2021-06-09T01:13:42.8014227Z     - node(TopNNode)
2021-06-09T01:13:42.8014991Z         - node(TableScanNode)
2021-06-09T01:13:42.8015439Z 
2021-06-09T01:13:42.8015869Z ] but found [
2021-06-09T01:13:42.8016215Z 
2021-06-09T01:13:42.8016694Z Output[orderkey]
2021-06-09T01:13:42.8017533Z │   Layout: [orderkey:bigint]
2021-06-09T01:13:42.8018416Z └─ TopN[10 by (orderkey ASC NULLS LAST)]
2021-06-09T01:13:42.8019256Z    │   Layout: [orderkey:bigint]
2021-06-09T01:13:42.8020119Z    └─ LocalExchange[SINGLE] ()
2021-06-09T01:13:42.8020974Z       │   Layout: [orderkey:bigint]
2021-06-09T01:13:42.8021813Z       └─ RemoteExchange[GATHER]
2021-06-09T01:13:42.8023021Z          │   Layout: [orderkey:bigint]
2021-06-09T01:13:42.8024127Z          └─ LimitPartial[10, input pre-sorted by (orderkey)]
2021-06-09T01:13:42.8025114Z             │   Layout: [orderkey:bigint]
2021-06-09T01:13:42.8026559Z             └─ TableScan[phoenix:tpch.orders TPCH.ORDERS sortOrder=[ORDERKEY:bigint:BIGINT ASC NULLS LAST] limit=10 columns=[ORDERKEY:bigint:BIGINT]]
2021-06-09T01:13:42.8027747Z                    Layout: [orderkey:bigint]
2021-06-09T01:13:42.8028437Z                    orderkey := ORDERKEY:bigint:BIGINT

This test passed when I had the if(...isEmpty()) instead of the Optional.map change, which does not make any sense since it's functionally the same.

raunaqmorarka · 2021-06-09T05:17:19Z

This test passed when I had the if(...isEmpty()) instead of the Optional.map change, which does not make any sense since it's functionally the same.

The plan generated above is the right one, it looks like we'll need to override that test for phoenix as the behaviour of (topN pushdown + no topN limit guarantee + using sorting property) is a unique one.
Not sure what was wrong with previous version, if it's reproducible you could go back to it and try debugging.

raunaqmorarka · 2021-06-09T05:25:51Z

plugin/trino-phoenix5/src/main/java/io/trino/plugin/phoenix5/PhoenixMetadata.java

+    public ConnectorTableProperties getTableProperties(ConnectorSession session, ConnectorTableHandle table)
+    {
+        JdbcTableHandle tableHandle = (JdbcTableHandle) table;
+        List<LocalProperty<ColumnHandle>> sortingProperties = tableHandle.getSortOrder()


I think we could just do this in DefaultJdbcMetadata#getTableProperties
I don't see anything here unique to phoenix
Any other connector which can do TopN pushdown should automatically benefit in same way

One case where this would not be true in the phoenix (4) connector. The topN is pushed in the plan, but the connection still can decide not honor it. The same is true for some of the other connector (Postgres, MySQL), which also also may decide not to do the topN after the plan has decided to push this down.

Ok, doing it in DefaultJdbcMetadata just requires the assumption that the sortOrder in JdbcTableHandle will be guaranteed when the data is read from it, it's not required that the limit part of topN be guaranteed.
If that assumption also does not hold, then we can't move this to DefaultJdbcMetadata.
I'm not sure why we wouldn't drop the sortOrder from JdbcTableHandle if we weren't going to actually use that in forming the JDBC query.

yes, in DefaultJdbcMetadata it would need to depend on io.trino.plugin.jdbc.JdbcClient#isTopNLimitGuaranteed

@findepi is isTopNLimitGuaranteed just about whether the limit part of topN is guaranteed by connector or does it impact the sorting part as well ?
If it's just about the limit, then I think don't need to worry about that, for this optimisation the main concern is whether the sorting part of TopN is guaranteed or not, we'll apply the limit part ourselves anyway.

I think for now let's leave it in the Phoenix connector.

A special case is the Phoenix (4) connector, which overrides the topN function in some cases. That scenario would not work in general, right? As it would not even push the order by down.

Btw. order by pushdown (without limit) would be a useful feature anyway. Also see #8093, and it seems that I am missing something.

~~As for isTopNGuaranteed... Phoenix will silently not enforce a limit when the limit > 2^31. Does that mean now that topN is guaranteed or not?~~

Wait... Not true, it will throw an exception. In that case we can change both isLimitGuaranteed and isTopNGuaranteed to return true!

Arrgghh. So here's why isLimitGuaranteed is false: Trino will issue mutliple SQL statements to Phoenix, each of which has the limit. So it's guaranteed only per "partition".

Sorry for the back-and-forth.

I'm wondering what it would take to get this optimization into DefaultJdbcMetadata for cases where after TopN pushdown into TS, the result size is not strictly guaranteed (isTopNGuaranteed is false) but the ordering is.

@raunaqmorarka that could perhaps be useful in some cases (but currently JdbcMetadata lacks information to disntuigh such cases)

OTOH, it's useful for large data sets only (right?), which limits applicability of this, as JDBC connectors are not superfast anyway.

I think we should first make it work when isTopNGuaranteed is true (simpler case, less changes) and log further improvement as a separate ticket

With a special case for Phoenix (5)...? (Where isTopGuaranteed is false, but we still want this optimization)

@lhofhansl good point. If Phoenix 5 is most interesting to you & special case anyway, let's proceed with Phoenix, but let's define what we want to achieve in general, in an issue, so that's easier to follow up

lhofhansl · 2021-06-10T05:18:47Z

plugin/trino-phoenix5/src/test/java/io/trino/plugin/phoenix5/TestPhoenixConnectorTest.java

+    {
+        assertThat(query("SELECT orderkey FROM orders ORDER BY orderkey LIMIT 10"))
+                .ordered()
+                .isNotFullyPushedDown(TopNNode.class, ExchangeNode.class, ExchangeNode.class, LimitNode.class);


This passes the test. Not entirely sure whether it is still a useful test, though.

I think we can include few more tests from BCT with modification here, like order by DESC, multiple different orders etc.

How is this different from testing that TopN pushdown doesn't happen? (which is already tested in superclass if SUPPORTS_TOPN_PUSHDOWN is false). Or is the difference that the plan looks different (the retained nodes look same to me).

i.e. would it be better to add the new cases to superclass itself?

trino/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java

Lines 626 to 631 in 41ff64c

if (!hasBehavior(SUPPORTS_TOPN_PUSHDOWN)) {

assertThat(query("SELECT orderkey FROM orders ORDER BY orderkey LIMIT 10"))

.ordered()

.isNotFullyPushedDown(TopNNode.class);

return;

}

This is the original plan:

Output[orderkey] │ Layout: [orderkey:bigint] └─ TopN[10 by (orderkey ASC NULLS LAST)] │ Layout: [orderkey:bigint] └─ LocalExchange[SINGLE] () │ Layout: [orderkey:bigint] └─ RemoteExchange[GATHER] │ Layout: [orderkey:bigint] └─ TopNPartial[10 by (orderkey ASC NULLS LAST)] │ Layout: [orderkey:bigint] └─ TableScan[phoenix:tpch.orders TPCH.ORDERS sortOrder=[ORDERKEY:bigint:BIGINT ASC NULLS LAST] limit=10 columns=[ORDERKEY:bigint:BIGINT]] Layout: [orderkey:bigint] orderkey := ORDERKEY:bigint:BIGINT

So this indeed verifies that the topNPartial was replaced by a limit

Thanks @lhofhansl . Missed that the partial got removed.

That would work.

Not sure what we are trying to test. The basic plan rewrite is done in #6634, here I just want to make that Phoenix is triggering that query rewrite. Assuming changes to #6634 would add tests, I would add the most generic version to the connectors, just making sure that the replacement actually happens.

As usually, I do not feel strongly :) Happy to add a more explicit test as you suggest.

#6634 did add tests with a mock connector, hive and later on tpch connector
Here we want to test that after partial pushdown of topN into table scan takes place, we detect that through the sorting ordering in the JDBC table handle and expose the right ConnectorTableProperties for it. This part is unique to this connector. We can assume that if the right properties are set, then the planner will do the expected query rewrite. The plan assertions are an indirect but convenient way to test that the right sorting properties were set.
PlanMatchPattern would allow us to explicitly verify the preSortedInputs in LimitNode.

This is the only way I can get this to work:

assertThat(query("SELECT orderkey FROM orders ORDER BY orderkey LIMIT 10")) .ordered() .matches( output( node(TopNNode.class, anyTree( node(LimitNode.class, node(TableScanNode.class))))));

Not sure that is much better.

We can make that slightly better by building more specific plan node patterns rather than just using Node.class, e.g. see the plan patterns used in assertions in TestPartialTopNWithPresortedInput#testWithSortedTable.
However, to me this is nice to have, not a blocker to merging the PR.

Thanks! I had tried the more specific pattern, but in the time I had I couldn't get the plan to match. TestPartialTopNWithPresortedInput is a great example, though. I'll try that.

plugin/trino-phoenix5/src/test/java/io/trino/plugin/phoenix5/TestPhoenixConnectorTest.java

plugin/trino-phoenix5/pom.xml

plugin/trino-phoenix5/src/test/java/io/trino/plugin/phoenix5/TestPhoenixConnectorTest.java

raunaqmorarka · 2021-06-10T19:18:58Z

plugin/trino-phoenix5/src/test/java/io/trino/plugin/phoenix5/TestPhoenixConnectorTest.java

+    {
+        assertThat(query("SELECT orderkey FROM orders ORDER BY orderkey LIMIT 10"))
+                .ordered()
+                .isNotFullyPushedDown(TopNNode.class, ExchangeNode.class, ExchangeNode.class, LimitNode.class);


I think we can include few more tests from BCT with modification here, like order by DESC, multiple different orders etc.

lhofhansl · 2021-06-11T17:19:08Z

I think this is good to go (I added more tests and addressed the review comments).

Let's file a general issue for JDBC. I do agree with @findepi that for most JDBC connectors this will nor be a substantive improvement. Phoenix is special here as it actually generates splits and executes these again many nodes in the Phoenix/HBase cluster - so the extra, unnecessary topNPartial can have a significant impact.

(Next: Push down ORDER BY).

lhofhansl · 2021-06-15T18:38:36Z

Hi... Just making sure this is not waiting on anything from me.

lhofhansl · 2021-06-18T01:30:23Z

Ping ... :)

lhofhansl · 2021-06-28T20:41:46Z

Thinking about how I'd be using BasePlanTest.

hashhar

a few comments

plugin/trino-phoenix5/src/test/java/io/trino/plugin/phoenix5/TestPhoenixConnectorTest.java

hashhar · 2021-06-28T20:46:36Z

plugin/trino-phoenix5/src/test/java/io/trino/plugin/phoenix5/TestPhoenixConnectorTest.java

+    {
+        assertThat(query("SELECT orderkey FROM orders ORDER BY orderkey LIMIT 10"))
+                .ordered()
+                .isNotFullyPushedDown(TopNNode.class, ExchangeNode.class, ExchangeNode.class, LimitNode.class);


@lhofhansl Using isNotFullyPushedDown for asserting plan shape when it has nothing to do with pushdown seems a bit dirty.

Using something similar to what Piotr shows above is more explicit in the intent of the test.

lhofhansl · 2021-06-28T20:49:25Z

@hashhar I agree in isNofFullyPushedDown, felt the same way.
Trick is figuring out how to create a local query runner for Phoenix, which is required for subclasses of BasePlanTest.

Thinking more about it...
This is what we want to test, right? That (1) topN was not pushed down, and (2) that the plan was modified to include LimitNode instead of a partial topN.

Perhaps I can add more functions with better names to QueryAssertions?

What do you think @hashhar ?

lhofhansl · 2021-06-28T23:51:04Z

I added SkipException to the test.

As for BasePlanTests... I am still looking into that. Looks like all existing such tests are for Hive...?

lhofhansl · 2021-07-01T20:14:13Z

Can we merge with the current tests? (as I mentioned above, they do test what we want: That topN is actually not pushed down, but that instead the plan is modified)

lhofhansl · 2021-07-07T04:14:13Z

Rebased

hashhar

LGTM % comments.

Sorry to keep you waiting @lhofhansl .

plugin/trino-phoenix5/src/test/java/io/trino/plugin/phoenix5/TestPhoenixConnectorTest.java

lhofhansl · 2021-07-07T05:30:59Z

Hey @hashhar , no problem at all. I'll address your comments tomorrow.

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

hashhar

Looks good to me. Thanks @lhofhansl.

@raunaqmorarka @sopel39 PTAL if you want to.

raunaqmorarka · 2021-07-09T08:18:35Z

Minor comment about looking into https://github.com/trinodb/trino/pull/8171/files#r665088423
Otherwise lgtm

raunaqmorarka · 2021-07-09T21:02:49Z

plugin/trino-phoenix5/src/test/java/io/trino/plugin/phoenix5/TestPhoenixConnectorTest.java

+                tableName);
+        assertUpdate(createTableSql, 1500L);
+
+        String expected = "SELECT custkey FROM customer ORDER BY 1 NULLS FIRST LIMIT 100";


One doubt here, assuming that phoenix will honour whatever nulls sorting order Trino will pass down in the jdbc query to it, we can expect this to work even without the NULLS FIRST part right ?
The hive case is a bit different as there the sort order of NULLS FIRST is fixed when the sorted files are written.

Yes. It will just be slower on the Phoenix side if there happens to an index on the sort column(s), The index can only be used with NULL FIRST. So for correctness this is not needed.

lhofhansl · 2021-07-13T05:09:40Z

This one works with explicit plan matching.

raunaqmorarka

the updated tests lgtm

raunaqmorarka · 2021-07-13T06:47:14Z

plugin/trino-phoenix5/src/test/java/io/trino/plugin/phoenix5/TestPhoenixConnectorTest.java

@@ -320,21 +335,92 @@ public void testTopNPushdown()
    @Test
    public void testReplacePartialTopNWithLimit()
    {
+        List<PlanMatchPattern.Ordering> orderBy = ImmutableList.of(sort("orderkey", ASCENDING, LAST));
+
        assertThat(query("SELECT orderkey FROM orders ORDER BY orderkey LIMIT 10"))
                .ordered()


you don't need ordered now

core/trino-main/src/test/java/io/trino/sql/query/QueryAssertions.java

lhofhansl · 2021-07-15T08:54:48Z

Restructured commits to add QueryAssertions.matches as separate commit.

hashhar

LGTM.

lhofhansl · 2021-07-16T20:29:19Z

Thank you.

cla-bot bot added the cla-signed label Jun 1, 2021

lhofhansl force-pushed the phoenix-topn-limit branch from 73efe65 to a2e65f1 Compare June 2, 2021 17:18

lhofhansl force-pushed the phoenix-topn-limit branch from a2e65f1 to 3e70aef Compare June 8, 2021 18:33

raunaqmorarka reviewed Jun 8, 2021

View reviewed changes

lhofhansl force-pushed the phoenix-topn-limit branch from 3e70aef to 33ebd28 Compare June 9, 2021 00:24

lhofhansl force-pushed the phoenix-topn-limit branch from 33ebd28 to ec91da4 Compare June 9, 2021 00:56

raunaqmorarka reviewed Jun 9, 2021

View reviewed changes

plugin/trino-phoenix5/src/main/java/io/trino/plugin/phoenix5/PhoenixMetadata.java Show resolved Hide resolved

raunaqmorarka reviewed Jun 9, 2021

View reviewed changes

hashhar mentioned this pull request Jun 9, 2021

Rename isTopNLimitGuaranteed to isTopNGuaranteed #8238

Merged

lhofhansl force-pushed the phoenix-topn-limit branch from ec91da4 to 2cf3a20 Compare June 10, 2021 05:17

lhofhansl commented Jun 10, 2021

View reviewed changes

plugin/trino-phoenix5/src/test/java/io/trino/plugin/phoenix5/TestPhoenixConnectorTest.java Outdated Show resolved Hide resolved

lhofhansl force-pushed the phoenix-topn-limit branch from 2cf3a20 to 95aee98 Compare June 10, 2021 16:17

raunaqmorarka reviewed Jun 10, 2021

View reviewed changes

lhofhansl force-pushed the phoenix-topn-limit branch from 95aee98 to 3ed73a0 Compare June 10, 2021 22:33

raunaqmorarka approved these changes Jun 11, 2021

View reviewed changes

raunaqmorarka requested a review from findepi June 15, 2021 18:44

lhofhansl force-pushed the phoenix-topn-limit branch from 3ed73a0 to 102ada0 Compare June 23, 2021 17:36

hashhar reviewed Jun 28, 2021

View reviewed changes

lhofhansl force-pushed the phoenix-topn-limit branch from 2b5cd76 to 4faff2f Compare June 28, 2021 23:50

lhofhansl force-pushed the phoenix-topn-limit branch from 4faff2f to 0c7bb96 Compare July 7, 2021 04:13

hashhar approved these changes Jul 7, 2021

View reviewed changes

lhofhansl force-pushed the phoenix-topn-limit branch from 0c7bb96 to 56b05f5 Compare July 7, 2021 16:30

lhofhansl commented Jul 8, 2021

View reviewed changes

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java Show resolved Hide resolved

hashhar approved these changes Jul 9, 2021

View reviewed changes

raunaqmorarka reviewed Jul 9, 2021

View reviewed changes

lhofhansl force-pushed the phoenix-topn-limit branch from 56b05f5 to 6a8b0a6 Compare July 13, 2021 05:09

raunaqmorarka reviewed Jul 13, 2021

View reviewed changes

lhofhansl force-pushed the phoenix-topn-limit branch from 6a8b0a6 to 544ece1 Compare July 15, 2021 08:54

Add QueryAssertions.matches(PlanMatchPattern) method.

c1cf825

lhofhansl force-pushed the phoenix-topn-limit branch from 544ece1 to 6f82f2f Compare July 15, 2021 09:15

Allow Phoenix to replace partial topn with limit.

6f82f2f

raunaqmorarka approved these changes Jul 15, 2021

View reviewed changes

hashhar approved these changes Jul 16, 2021

View reviewed changes

hashhar merged commit fe3ba1b into trinodb:master Jul 16, 2021

hashhar added this to the 360 milestone Jul 16, 2021

hashhar mentioned this pull request Jul 27, 2021

Release notes for 360 #8455

Closed

11 tasks

raunaqmorarka mentioned this pull request Jul 25, 2023

Rewrite partial top-n node to LimitNode or LastNNode #18384

Open

	if (!hasBehavior(SUPPORTS_TOPN_PUSHDOWN)) {
	assertThat(query("SELECT orderkey FROM orders ORDER BY orderkey LIMIT 10"))
	.ordered()
	.isNotFullyPushedDown(TopNNode.class);
	return;
	}

Allow Phoenix to replace partial topn with limit. #8171

Allow Phoenix to replace partial topn with limit. #8171

Conversation

lhofhansl commented Jun 1, 2021

lhofhansl commented Jun 2, 2021

lhofhansl commented Jun 2, 2021 • edited Loading

lhofhansl commented Jun 2, 2021

findepi commented Jun 7, 2021

lhofhansl commented Jun 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhofhansl commented Jun 9, 2021

lhofhansl commented Jun 9, 2021

lhofhansl commented Jun 9, 2021

raunaqmorarka commented Jun 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhofhansl Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

lhofhansl Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhofhansl commented Jun 11, 2021 • edited Loading

lhofhansl commented Jun 15, 2021

lhofhansl commented Jun 18, 2021

lhofhansl commented Jun 28, 2021 • edited Loading

hashhar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhofhansl commented Jun 28, 2021 • edited Loading

lhofhansl commented Jun 28, 2021

lhofhansl commented Jul 1, 2021

lhofhansl commented Jul 7, 2021

hashhar left a comment

Choose a reason for hiding this comment

lhofhansl commented Jul 7, 2021

hashhar left a comment

Choose a reason for hiding this comment

raunaqmorarka commented Jul 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhofhansl commented Jul 13, 2021

raunaqmorarka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhofhansl commented Jul 15, 2021

hashhar left a comment

Choose a reason for hiding this comment

lhofhansl commented Jul 16, 2021

lhofhansl commented Jun 2, 2021 •

edited

Loading

lhofhansl Jun 9, 2021 •

edited

Loading

lhofhansl Jun 9, 2021 •

edited

Loading

lhofhansl commented Jun 11, 2021 •

edited

Loading

lhofhansl commented Jun 28, 2021 •

edited

Loading

lhofhansl commented Jun 28, 2021 •

edited

Loading