Enhance simplifier by adding Canonicalize #8780

yyy1000 · 2024-01-07T22:22:26Z

Which issue does this PR close?

Closes #8724

Rationale for this change

introduce new optimizer rule and optimizer can be better.

What changes are included in this PR?

Reorder all BinaryExpr to or (the name of col1 is larger than col2)
two unit test cases

Are these changes tested?

Yes

Are there any user-facing changes?

No

Jefffrey

You're thinking in the right direction, but there'll need to be more work done to achieve the intended functionality. I've left some pointers to hopefully help you adjust the code as needed, but feel free to ask if you need further clarification or help

Jefffrey · 2024-01-08T08:05:38Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+                right: right.clone(),
+            };
+            let mut switch_op: Operator = op.clone();
+            if left.try_into_col().is_ok() && right.try_into_col().is_ok() {


I think this check is better off done via a match, e.g.

match (left.as_ref(), right.as_ref()) { (Expr::Column(a), Expr::Column(b)) => todo!(), _ => todo!(), }

Got it!
Thank you very much

Jefffrey · 2024-01-08T08:09:06Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+                    if let Some(swap_op) = op.swap() {
+                        switch_op = swap_op;
+                    }


You're on the right track here with swapping the Operator, but one very important note is that for this functionality, we should only swap certain operators that it makes sense to.

For example, we cannot swap A - B to B - A, as that changes the expression's meaning

Got it.
Here swap would return None if operator makes no sense, e.g.: -, see: https://github.com/apache/arrow-datafusion/blob/cc4289484c33e478242bf5d2b59f695fdb427ab9/datafusion/expr/src/operator.rs#L172C5-L202C6

I think in this case we will not change the order of the Expr, and and would remain the same order as before.

Jefffrey · 2024-01-08T08:12:01Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+            // Case 2, <literal> <op> <col>
+            else if left.try_into_col().is_err() && right.try_into_col().is_ok() {


One advantage of using match to destructure the left/right Exprs is that you would avoid cases like this, because a pitfall here is that just because an Expr is not a Expr::Column variant, doesn't mean it is a Expr::Literal variant, so failing to cast left to a Column could mean its another variant than Expr::Literal

yyy1000 · 2024-01-08T15:26:44Z

Changed the code according to the review. :)

Jefffrey

Looking better! 👍

I think the logic is good, but will see if CI passes to ensure no regression in behaviour.

I've left some more comments, mainly around the structure of the code and how to make it more ergonomic in a Rust way (in my opinion, at least)

Jefffrey · 2024-01-08T19:57:53Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+                (Expr::Column(_a), Expr::Column(_b)) => {
+                    let left_name = left.canonical_name();
+                    let right_name = right.canonical_name();
+                    if left_name < right_name {


I think this should be the other way around, if you want to be consistent with the docs

/// <col1> <op> <col2> is rewritten so that the name of col1 sorts higher than col2 (b > a would be canonicalized to a < b)

Jefffrey · 2024-01-08T20:03:08Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+/// Canonicalize any BinaryExprs that are not in canonical form
+/// <literal> <op> <col> is rewritten to <col> <op> <literal> (remember to switch the operator)
+/// <col> <op> <literal> is canonical
+/// <col1> <op> <col2> is rewritten so that the name of col1 sorts higher than col2 (b > a would be canonicalized to a < b)


Might need to upgrade the docs here, for example remove the remember to switch the operator line (or change it to clarify that operator would be switched)

Jefffrey · 2024-01-08T20:05:22Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+                op: op.clone(),
+                right: right.clone(),
+            };
+            match (left.as_ref(), right.as_ref()) {


You could even push the op.swap() into this match, to remove the nested if blocks, e.g.

match (left.as_ref(), right.as_ref(), op.swap()) { (Expr::Column(_), Expr::Column(_), Some(swapped_op)) => { todo!(); // swap based on name, directly using the swapped_op } (Expr::Literal(_), Expr::Column(_), Some(swapped_op)) => { todo!(); // swap to have column on left } _ => todo!(), }

You could then even add the name check of left vs right as an if condition to the branch

Jefffrey · 2024-01-08T20:08:52Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+            let mut new_expr = BinaryExpr {
+                left: left.clone(),
+                op: op.clone(),
+                right: right.clone(),
+            };


I wonder if there is a cleaner way to do this, without having to clone here (since this will always clone for Expr;:BinaryExpr even if we don't end up swapping)

Could try an approach without relying on mutability of new_expr

I use return in if statement to avoid new_expr in the new changes :)

yyy1000 · 2024-01-08T22:17:53Z

Thank you for your detailed review @Jefffrey !
I changed the code, and now it looks better :)
I'm new to Rust so the code may not looks good at first, thanks again for your guidance.

alamb

Thank you @yyy1000 and @Jefffrey -- this is looking like it is coming along nicely! There are definitely some CI failures to look into and I left some additional comments, but I think we are getting close

alamb · 2024-01-09T20:55:38Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+            let expected = col("c2").gt(col("c1"));
+            assert_eq!(simplify(expr), expected);
+        }
+    }


Could you also please add some additional slightly more complex tests that also use equality and constants (an important use case) such as the ones listed on #8724 (comment) ?

A=1 AND 1 = A AND A = 3 --> A = 1 AND A = 3 A=B AND A > 5 AND B=A --> A=B AND A > 5 (A=1 AND (B> 3 OR 3 < B)) --> (A = 1 AND B > 3)

Also, it would be great to add some negative tests as well like

A < 5 AND A >= 5 --> same (I realize this expression can never be true, but I don't think the simplifier knows how to handle that yet A > B AND A > B --> same

Correct, in the test case, for A < 5 AND A >= 5 --> same, it will make no change :)

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

alamb · 2024-01-09T21:01:47Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+        if let Expr::BinaryExpr(BinaryExpr { left, op, right }) = expr {
+            match (left.as_ref(), right.as_ref(), op.swap()) {


You can do something like this to reduce the indent of this function if you want (not required, I just figured I would point it out given you say you are looking to improve your rust skills

Suggested change

if let Expr::BinaryExpr(BinaryExpr { left, op, right }) = expr {

match (left.as_ref(), right.as_ref(), op.swap()) {

let Expr::BinaryExpr(BinaryExpr { left, op, right }) = expr else {

return Ok(expr)

};

match (left.as_ref(), right.as_ref(), op.swap()) {

alamb · 2024-01-09T21:06:35Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+                (Expr::Column(_), Expr::Column(_), Some(swapped_op)) => {
+                    if right.canonical_name() > left.canonical_name() {


Since Column implements PartialOrd you don't have to use canonical_name here (which allocates a string)

Suggested change

(Expr::Column(_), Expr::Column(_), Some(swapped_op)) => {

if right.canonical_name() > left.canonical_name() {

(Expr::Column(left_col), Expr::Column(right_col), Some(swapped_op)) => {

if left_col > right_col {

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

yyy1000 · 2024-01-09T21:53:58Z

Thank you for the review @alamb !
I pushed the change to the PR and I will look into the CI failure if it's there :)

alamb · 2024-01-10T18:54:37Z

I kicked off the CI checks!

yyy1000 · 2024-01-10T19:28:10Z

There's a FieldNotFound error in tests introduced by the PR. 🧐
I will dive into it and find what's wrong with it. :)

yyy1000 · 2024-01-11T03:02:21Z

I figured it as a SchemaError :: FieldNotFound error when using Join and it seems a little complicated for me :(
Any guidance would be helpful.

Jefffrey

I'll try take a look at the failing test and see if I can help explain/debug 👍

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

Jefffrey · 2024-01-11T09:24:02Z

So there's quite a few failing tests, I chose the first I saw: dataframe::tests::join:

https://github.com/apache/arrow-datafusion/blob/a154884545cfdeb1a6c20872b3882a5624cd1119/datafusion/core/src/dataframe/mod.rs#L1892-L1906

Pretty simple join test.

Running it fails:

Error: SchemaError(FieldNotFound { field: Column { relation: Some(Bare { table: "c2" }), name: "c1" }, valid_fields: [Column { relation: Some(Bare { table: "aggregate_test_100" }), name: "c1" }, Column { relation: Some(Bare { table: "aggregate_test_100" }), name: "c2" }] }, Some(""))

So lets change the test to explain the output and see what the plan looks like:

    #[tokio::test]
    async fn join() -> Result<()> {
        let left = test_table().await?.select_columns(&["c1", "c2"])?;
        let right = test_table_with_name("c2")
            .await?
            .select_columns(&["c1", "c3"])?;
        let join = left.join(right, JoinType::Inner, &["c1"], &["c1"], None)?;
        join.explain(true, false)?.show().await?; // HERE
        Ok(())
    }

Gives this output (truncated only to the interesting parts):

+------------------------------------------------------------+----------------------------------------------------------------------------------------------------+
| plan_type                                                  | plan                                                                                               |
+------------------------------------------------------------+----------------------------------------------------------------------------------------------------+
| initial_logical_plan                                       | Inner Join: aggregate_test_100.c1 = c2.c1                                                          |
|                                                            |   Projection: aggregate_test_100.c1, aggregate_test_100.c2                                         |
|                                                            |     TableScan: aggregate_test_100                                                                  |
|                                                            |   Projection: c2.c1, c2.c3                                                                         |
|                                                            |     TableScan: c2                                                                                  |
~~~
| logical_plan after simplify_expressions                    | Inner Join: c2.c1 = aggregate_test_100.c1                                                          |
|                                                            |   Projection: aggregate_test_100.c1, aggregate_test_100.c2                                         |
|                                                            |     TableScan: aggregate_test_100                                                                  |
|                                                            |   Projection: c2.c1, c2.c3                                                                         |
|                                                            |     TableScan: c2                                                                                  |
~~~
| logical_plan after eliminate_cross_join                    | Inner Join: aggregate_test_100.c1 = c2.c1                                                          |
|                                                            |   Projection: aggregate_test_100.c1, aggregate_test_100.c2                                         |
|                                                            |     TableScan: aggregate_test_100                                                                  |
|                                                            |   Projection: c2.c1, c2.c3                                                                         |
|                                                            |     TableScan: c2                                                                                  |
~~~
| logical_plan after simplify_expressions                    | Inner Join: c2.c1 = aggregate_test_100.c1                                                          |
|                                                            |   Projection: aggregate_test_100.c1, aggregate_test_100.c2                                         |
|                                                            |     TableScan: aggregate_test_100                                                                  |
|                                                            |   Projection: c2.c1, c2.c3                                                                         |
|                                                            |     TableScan: c2                                                                                  |
~~~
| logical_plan after optimize_projections                    | Inner Join: c2.c1 = aggregate_test_100.c1                                                          |
|                                                            |   TableScan: aggregate_test_100 projection=[c1, c2]                                                |
|                                                            |   TableScan: c2 projection=[c1, c3]                                                                |
~~~
| logical_plan after eliminate_cross_join                    | Inner Join: aggregate_test_100.c1 = c2.c1                                                          |
|                                                            |   TableScan: aggregate_test_100 projection=[c1, c2]                                                |
|                                                            |   TableScan: c2 projection=[c1, c3]                                                                |
~~~
| logical_plan after simplify_expressions                    | Inner Join: c2.c1 = aggregate_test_100.c1                                                          |
|                                                            |   TableScan: aggregate_test_100 projection=[c1, c2]                                                |
|                                                            |   TableScan: c2 projection=[c1, c3]                                                                |
~~~
| logical_plan                                               | Inner Join: c2.c1 = aggregate_test_100.c1                                                          |
|                                                            |   TableScan: aggregate_test_100 projection=[c1, c2]                                                |
|                                                            |   TableScan: c2 projection=[c1, c3]                                                                |
| initial_physical_plan                                      | Schema error: No field named c2.c1. Valid fields are aggregate_test_100.c1, aggregate_test_100.c2. |
+------------------------------------------------------------+----------------------------------------------------------------------------------------------------+

Removed rules which didn't change output

What is interesting here, is how simplify_expressions and eliminate_cross_join 'fight' over the order of the join, and in the end, simplify_expressions 'wins' with its order as there is no more eliminate_cross_join run after. This seems to suggest the order of the columns in the join expression is important.

Indeed, after some more sleuthing to find the cause (enabling backtrace feature and setting RUST_BACKTRACE=1 then running original test to see where the error originated), it is here:

https://github.com/apache/arrow-datafusion/blob/a154884545cfdeb1a6c20872b3882a5624cd1119/datafusion/core/src/physical_planner.rs#L1055-L1065

Specifically line 1061.

keys is grabbed directly from the on condition of the LogicalPlan Join.

We can see here that it relies on the assumption that given a join on condition of a = b, column a must be from the left child and column b must be from the right child.

I hope this helps in explaining the error @yyy1000 (and the process to debug it)

As for how to fix it, I'm not sure. Either we respect the current behaviour of relying on the order of columns in a join on expression to be correct with respect to its children, or we try to fix that to not rely on that.

Going with the former might suggest having to ensure that this new canonicalize step won't reorder expressions in a Join on clause.

Going with the latter, I'm not so sure, would need some input from those more familiar with the Join logic in Datafusion.

Just throwing in my 2 cents

yyy1000 · 2024-01-11T17:26:54Z

Woo, much appreciated that @Jefffrey ❤️
I learned that using explain is a good way to debug, will see the methods to fix it now. :)

Co-authored-by: Jeffrey Vo <jeffrey.vo.australia@gmail.com>

yyy1000 · 2024-01-12T18:42:16Z

As I looked into the code today,
if we respect the current behaviour, maybe we can change the info field in ExprSimplifier, and see whether info contains 'Join infomation' https://github.com/apache/arrow-datafusion/blob/d9a1d4261f78dfd3ead235d6b850c64e0ae9bec6/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs#L132-L148
if trying to fix that to not rely on join order, would it be implementable?

yyy1000 · 2024-01-12T19:42:33Z

Woops, I found implement the second way would be more easy.
I pushed my changes to the PR, it would try make column a the right child and column b the right child if it can't find them in left and right child.
Also, two test cases are modified because the name of t2.c1 is larger than t1.c1, and it meets expectation.

alamb · 2024-01-12T20:10:17Z

Retriggered CI

yyy1000 · 2024-01-12T21:02:37Z

Ah, I believe the CI fails just because the test cases need to be changed due to bigger names would be in the front, I could do that then.

yyy1000 · 2024-01-13T01:03:20Z

Well, I tried to fix the CI-error, but it's harder than I thought. :(
Now the problem is on, the two columns in Join, would change but some functions still pass left and right infos, didn't knowing that now the two columns in on has been swapped.
The failed test case ,

index out of bounds: the len is 2 but the index is 2

indicated that here is the reason, https://github.com/apache/arrow-datafusion/blob/eb81ea299aa7e121bbe244e7e1ab56513d4ef800/datafusion/physical-plan/src/joins/utils.rs#L688-L697

alamb · 2024-01-18T20:27:26Z

🤔 so this PR has turned out to be far more complicated than we imagined

What is the current state of this PR? Do we think the code is ok, and we just need to update the tests? Or do we think something more fundamental is wrong with it?

Thank you for this work @yyy1000 -- I am sorry it was not an easy first issue as I had thought

Jefffrey · 2024-01-18T20:42:46Z

From what I understand, it seems this effort will be blocked as either have to:

Fix left/right join assumptions throughout the code base
Or enhance the expression simplifier to be able to tell when an Expr is from a join on clause, and if so don't do the reordering

yyy1000 · 2024-01-18T21:36:17Z

Yeah, what @Jefffrey said is right.
I tried the first method but failed, now I'm trying the second method. It's kind of difficult for me. 🥲
Sorry for the delay. :(

alamb · 2024-01-19T20:10:07Z

Yeah, what @Jefffrey said is right. I tried the first method but failed, now I'm trying the second method. It's kind of difficult for me. 🥲 Sorry for the delay. :(

No worries -- thank you for your perseverence

yyy1000

Hi, here is what I tried the second method, now the Join would not call canonicalize.

yyy1000 · 2024-01-21T02:51:59Z

datafusion/optimizer/src/simplify_expressions/simplify_exprs.rs

+                }).collect::<Result<Vec<_>>>()?
+            },
+            _ => {
+                plan


This is what I tried for the second method, I define a new function called canonicalize in simplifier.
And the rule SimplifyExpressions will call this if the plan is not Join.
I didn't change the current simplify function because I think add a param to judge whether the Expr is from a Join plan would break a lot.

Makes sense. Could you also add a comment to the code here explaining this? Otherwise the reason is not immediately obvious for the near duplication of code 👍

yyy1000 · 2024-01-22T22:50:48Z

Ah, I see the CI failed still due to some test cases need to be changed.
Would following the CI flow https://github.com/apache/arrow-datafusion/blob/903ef94a4742a4ff6933dcf696a287972df3b5e2/.github/workflows/rust.yml#L210-L239 to see the failure in my local environment a good method?

yyy1000 · 2024-01-22T23:08:45Z

Update: following that helps me fix the wrong test cases. :)

yyy1000 · 2024-01-22T23:26:22Z

@alamb I think this PR is close to be merged. :)
I'd like to see your review and feedback. 😃

Jefffrey

Looking good now, just left a minor doc suggestion, as well as a question on the join utils changes 👍

datafusion/optimizer/src/simplify_expressions/simplify_exprs.rs

Jefffrey · 2024-01-23T11:02:09Z

datafusion/physical-plan/src/joins/utils.rs

+        let on_left_reverse = &on.iter().map(|on| on.1.clone()).collect::<HashSet<_>>();
+        let left_missing_reverse =
+            on_left_reverse.difference(left).collect::<HashSet<_>>();
+        let on_right_reverse = &on.iter().map(|on| on.0.clone()).collect::<HashSet<_>>();
+        let right_missing_reverse =
+            on_right_reverse.difference(right).collect::<HashSet<_>>();
+        if !left_missing_reverse.is_empty() | !right_missing_reverse.is_empty() {
+            return plan_err!(
+                "The left or right side of the join does not have all columns on \"on\": \nMissing on the left: {left_missing:?}\nMissing on the right: {right_missing:?}"
+            );
+        }


Is this change still required if the order of the on clause in Joins should not be reordered, now?

Ah, got it. I should remove this.

alamb · 2024-01-23T11:13:08Z

I retriggered CI and will try and check it out later today

Co-authored-by: Jeffrey Vo <jeffrey.vo.australia@gmail.com>

Jefffrey

Tests passing, nice work 👍

alamb

Thank you for sticking with this PR @yyy1000 and to @Jefffrey for the assist and review 🙏

alamb · 2024-01-24T12:14:59Z

I merged this branch up from main to make sure all the CI passes, and if it does I plan to merge it in!

alamb · 2024-01-24T12:40:55Z

While reviewing this PR, I had a thought for a slightly more consistent API which I have proposed in #8983

Enhance simplifier by adding Canonicalize

af1771f

github-actions bot added the optimizer Optimizer rules label Jan 7, 2024

yyy1000 mentioned this pull request Jan 7, 2024

Expression simplifier does not simplify A = B AND B = A #8724

Closed

fix swap operation

0bf7bdd

Jefffrey requested changes Jan 8, 2024

View reviewed changes

Using Match to match cases

0ca9e6f

alamb mentioned this pull request Jan 8, 2024

DataFusion weekly project plan (Andrew Lamb) - Jan 8, 2024 #8786

Closed

7 tasks

Jefffrey reviewed Jan 8, 2024

View reviewed changes

feat: refactor code using better match and revise doc

de23470

alamb reviewed Jan 9, 2024

View reviewed changes

yyy1000 and others added 2 commits January 9, 2024 15:34

Update datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

e879c0b

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

add more robust test case and code format more Rustic

a715cfa

Jefffrey reviewed Jan 11, 2024

View reviewed changes

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs Outdated Show resolved Hide resolved

Update datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

c330250

Co-authored-by: Jeffrey Vo <jeffrey.vo.australia@gmail.com>

Make Join unrelated to order

e3ed1ef

github-actions bot added the core Core DataFusion crate label Jan 12, 2024

fmt doc

f9692f5

Check Join for both sides

fbb6527

alamb mentioned this pull request Jan 14, 2024

DataFusion weekly project plan (Andrew Lamb) - Jan 15, 2024 #8864

Closed

9 tasks

No canonicalize for Join

b06251a

github-actions bot added sqllogictest SQL Logic Tests (.slt) and removed core Core DataFusion crate labels Jan 21, 2024

yyy1000 commented Jan 21, 2024

View reviewed changes

yyy1000 added 2 commits January 20, 2024 20:55

cargo fmt

0e16762

Add comment for dup codes

2f34c07

alamb mentioned this pull request Jan 21, 2024

DataFusion weekly project plan (Andrew Lamb) - Jan 22, 2024 #8933

Closed

9 tasks

Fix test cases

d80511c

remove wrong git commit

28927c7

Jefffrey reviewed Jan 23, 2024

View reviewed changes

yyy1000 and others added 2 commits January 23, 2024 08:38

Update datafusion/optimizer/src/simplify_expressions/simplify_exprs.rs

98cd6ea

Co-authored-by: Jeffrey Vo <jeffrey.vo.australia@gmail.com>

remove outdated change

1b76a1d

Jefffrey approved these changes Jan 23, 2024

View reviewed changes

alamb approved these changes Jan 24, 2024

View reviewed changes

Merge remote-tracking branch 'apache/main' into simplifier

dcf3172

alamb merged commit bc0ba6a into apache:main Jan 24, 2024
22 checks passed

alamb mentioned this pull request Jan 24, 2024

Improve Canonicalize API #8983

Merged

yyy1000 deleted the simplifier branch January 24, 2024 14:54

		// Case 2, <literal> <op> <col>
		else if left.try_into_col().is_err() && right.try_into_col().is_ok() {

		if let Expr::BinaryExpr(BinaryExpr { left, op, right }) = expr {
		match (left.as_ref(), right.as_ref(), op.swap()) {

		(Expr::Column(_), Expr::Column(_), Some(swapped_op)) => {
		if right.canonical_name() > left.canonical_name() {

Enhance simplifier by adding Canonicalize #8780

Enhance simplifier by adding Canonicalize #8780

Conversation

yyy1000 commented Jan 7, 2024 • edited by alamb Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Jefffrey left a 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

yyy1000 commented Jan 8, 2024

Jefffrey left a 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

yyy1000 commented Jan 8, 2024

alamb left a 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

yyy1000 commented Jan 9, 2024

alamb commented Jan 10, 2024

yyy1000 commented Jan 10, 2024 • edited Loading

yyy1000 commented Jan 11, 2024 • edited Loading

Jefffrey left a comment

Choose a reason for hiding this comment

Jefffrey commented Jan 11, 2024

yyy1000 commented Jan 11, 2024

yyy1000 commented Jan 12, 2024

yyy1000 commented Jan 12, 2024

alamb commented Jan 12, 2024

yyy1000 commented Jan 12, 2024

yyy1000 commented Jan 13, 2024

alamb commented Jan 18, 2024

Jefffrey commented Jan 18, 2024

yyy1000 commented Jan 18, 2024

alamb commented Jan 19, 2024

yyy1000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jefffrey Jan 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyy1000 commented Jan 22, 2024

yyy1000 commented Jan 22, 2024

yyy1000 commented Jan 22, 2024

Jefffrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Jan 23, 2024

Jefffrey left a comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb commented Jan 24, 2024

alamb commented Jan 24, 2024

yyy1000 commented Jan 7, 2024 •

edited by alamb

Loading

yyy1000 commented Jan 10, 2024 •

edited

Loading

yyy1000 commented Jan 11, 2024 •

edited

Loading

Jefffrey Jan 21, 2024 •

edited

Loading