GH-43187 [C++] Support is_in predicates for SimplifyWithGuarantee #43256

larry98 · 2024-07-15T15:43:23Z

Rationale for this change

We would like to enable parquet predicate pushdown on is_in predicates. The first step is to add support for is_in predicates in SimplifyWithGuarantee.

What changes are included in this PR?

We add a option to SimplifyWithGuarantee that users can set if they can ensure that all is_in value sets in the expression are sorted and contain no duplicates. When this option is set, SimplifyWithGuarantee will attempt to simplify any is_in predicates that it finds in the expression.

Are these changes tested?

A new unit test was added to arrow-compute-expression-test testing this change.

Are there any user-facing changes?

No.

GitHub Issue: [C++] Support SimplifyWithGuarantee for is_in expressions #43187

github-actions · 2024-07-15T15:43:45Z

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

github-actions · 2024-07-15T15:45:02Z

⚠️ GitHub issue #43187 has been automatically assigned in GitHub to PR creator.

zanmato1984

I saw that in #43187 you mentioned:

Our current approach adds a new field to SetLookupOptions which allows the user to declare whether the value set is pre-sorted and deduplicated.

I think this makes sense as it makes the is_in expression more self-explaining. Why is it not in this PR? (Instead, an extra argument is_in_value_set_sorted is added.)

cpp/src/arrow/compute/expression.h

larry98 · 2024-07-16T20:47:03Z

I saw that in #43187 you mentioned:

Our current approach adds a new field to SetLookupOptions which allows the user to declare whether the value set is pre-sorted and deduplicated.

I think this makes sense as it makes the is_in expression more self-explaining. Why is it not in this PR? (Instead, an extra argument is_in_value_set_sorted is added.)

I was unsure if people would have liked that approach so I went with something simpler. But it sounds like we're OK with it, so I'll update this PR with the SetLookupOptions change.

zanmato1984 · 2024-07-17T03:05:45Z

Hi guys, @mapleFU @felipecrv what's your opinions on the current interface design? I.e., adding flag sorted_and_deduped in SetLookupOptions. Thanks.

I can proceed with reviewing the implementation when people agree on the interface.

mapleFU

I like the idea that using SimplifyIsIn to remove the elements in "is_in" until it's whole pruned. But I think sorted_and_deduped can put into separate patch since it's a bit weird to define a "ordering" here, and can easily break the invariant.

What about removing sorted_and_deduped firstly and sort during simplify?

mapleFU · 2024-07-17T06:33:22Z

cpp/src/arrow/compute/expression.cc

+    if (!value_set) return expr;
+    if (value_set->length() == 0) return literal(false);
+
+    if (!options->sorted_and_deduped) return expr;


Why without sorted_and_deduped the underlying data isn't get checked?

If the value set isn't sorted, then we'd have to do a linear scan. For large value sets (the use case that I care about), this cost is prohibitive. Our goal after all is support fast parquet filtering on is_in.

However, I could see this being useful for small value sets. Do you think we should add another option, either to SetLookupOption or SimplifyWithGuarantee, to enable simplifying is_in via linear scan? Or can we punt until someone has a use case and actually wants this feature?

cpp/src/arrow/compute/expression.cc

zanmato1984 · 2024-07-17T07:22:14Z

What about removing sorted_and_deduped firstly and sort during simplify?

IIUC, an in_list is often composed of constants. This list could be sorted-and-deduped only once in the planning phase (the "user" code on top of arrow expression). If we sort the in_list for the evaluation on every batch, that is probably redundant? IMHO, using a flag as a hint of the coupled in_list (whether it is already sorted-and-deduped), could be more flexible and compact.

mapleFU · 2024-07-17T07:26:38Z

IIUC, an in_list is often composed of constants. This list could be sorted-and-deduped only once in the planning phase (the "user" code on top of arrow expression). If we sort the in_list for the evaluation on every batch, that is probably redundant?

I agree, but a flag in planning phase is so weird, and considering something like nan in float, I think we'd at least separating it?

"In" list might be large, sort is important for performance, however still I think adding a "planning flag" is so hacking, at least in the first time, the "is_in" doesn't need any flag?

zanmato1984 · 2024-07-17T07:42:32Z

but a flag in planning phase is so weird

I'd rather not consider it a "flag in planning phase". It's an execution flag, being set in the planning phase, like any XXXOptions in arrow.

However I can kind of get your point about the weirdness. Maybe it tastes weird in the sense that it is a "hint" (about another field value_set in the option) rather than an imperative and definitive option?

mapleFU · 2024-07-17T07:46:47Z

Yes, this solving is pretty good but I think it would be better when we encapsulate all "is_in" and sorted checking within SimplifyWithGuarantee 🤔

mapleFU · 2024-07-17T11:16:43Z

I'd rather not consider it a "flag in planning phase". It's an execution flag, being set in the planning phase, like any XXXOptions in arrow.

I just catch that this flag would introduce a new flag, and avoid previous user from filtering "is_in" and might get worse performance...

larry98 · 2024-07-17T18:00:05Z

Thanks for the review.

To give some context, we are trying to optimize parquet filtering on high cardinality columns. You can think of this column as an ID on the row, so a typical use case might be to read 100K rows out of a file with 10MM rows. We express the 100K rows that we want to read as an is_in predicate.

I think it would be better when we encapsulate all "is_in" and sorted checking within SimplifyWithGuarantee 🤔

SimplifyWithGuarantee is called twice per row group, so sorting or checking for sortedness on each call to SimplifyWithGuarantee would eliminate any performance improvements - the predicate pushdown statistics computations would outweigh any gains from decoding less data.

This list could be sorted-and-deduped only once in the planning phase (the "user" code on top of arrow expression).

I agree that having some sort of planning or preprocessing step is what we ultimately want, I'm just unsure of what the right interface is. If we put the option inside SetLookupOptions, then it is up to the end user to to do this. If we add an option to SimplifyWithGuarantee then ParquetFileFragment could preprocess the user's filter expression to sort is_in value sets and invoke SimplifyWithGuarantee with the option to turn on the optimization.

cpp/src/arrow/compute/api_scalar.cc

cpp/src/arrow/compute/expression.cc

felipecrv · 2024-07-21T15:23:28Z

cpp/src/arrow/compute/expression.cc

@@ -1242,33 +1242,104 @@ struct Inequality {
                            /*insert_implicit_casts=*/false, &exec_context);
  }

+  /// Simplify an is_in predicate against this inequality as a guarantee.
+  Result<Expression> SimplifyIsIn(Expression expr) {


expr should probably be named is_in_call_expr to make it clear that this function assumes pre-conditions that should be ensured by the caller and it can't be called with any generic Expression.

cpp/src/arrow/compute/expression.cc

felipecrv · 2024-07-21T16:45:22Z

SimplifyWithGuarantee is called twice per row group, so sorting or checking for sortedness on each call to SimplifyWithGuarantee would eliminate any performance improvements - the predicate pushdown statistics computations would outweigh any gains from decoding less data.

This list could be sorted-and-deduped only once in the planning phase (the "user" code on top of arrow expression).

In a sense, sorting the value_set is part of the work that the is_in kernel should do. But during simplification you perform a bind that augments that Expression::Call with these extra fields

arrow/cpp/src/arrow/compute/expression.h

Lines 53 to 59 in c3ebdf5

    
           // post-Bind properties: 
        
           std::shared_ptr<Function> function; 
        
           const Kernel* kernel = NULLPTR; 
        
           std::shared_ptr<KernelState> kernel_state; 
        
           TypeHolder type;

You can encode the "pre-sorted" fact in the kernel_state here. Then you can skip checking again when simplifying if the Expression::Call value is already bound and with its kernel state initialized.

SetLookupBase is the base class for the kernel state of is_in and index_is_in.
https://github.com/apache/arrow/blob/main/cpp/src/arrow/compute/kernels/scalar_set_lookup.cc

larry98 · 2024-07-22T18:19:12Z

In a sense, sorting the value_set is part of the work that the is_in kernel should do. But during simplification you perform a bind that augments that Expression::Call with these extra fields

arrow/cpp/src/arrow/compute/expression.h

Lines 53 to 59 in c3ebdf5

// post-Bind properties:

std::shared_ptr<Function> function;

const Kernel* kernel = NULLPTR;

std::shared_ptr<KernelState> kernel_state;

TypeHolder type;

You can encode the "pre-sorted" fact in the kernel_state here. Then you can skip checking again when simplifying if the Expression::Call value is already bound and with its kernel state initialized.

SetLookupBase is the base class for the kernel state of is_in and index_is_in. https://github.com/apache/arrow/blob/main/cpp/src/arrow/compute/kernels/scalar_set_lookup.cc

Ah I didn't realize that binding initializes the kernel state and constructs the lookup table. Turns out there was a "bug" in my proof of concept where I copy over the post-bind fields with a simplified value set instead of re-binding. But this happens to DTRT for parquet predicate pushdown since the simplified expression is simply thrown away after we check that it is satisfiable, and the original, unsimplified expression is actually used to filter the data.

To fix this, I think we'd also need to delay binding until the end of SimplifyWithGuarantee once all inequalities have been processed. That way, the total cost of re-binding the simplified expressions across all row groups is still linear in the value set size.

Otherwise, thanks for the suggestion! I like this approach, but just to check my understanding of how this would work:

We change SetLookupState::Init so that it takes the SetLookupOptions& argument without const. When initializing, we sort and remove duplicates from the value set and update the options in place. This effectively makes it so that binding an expression can mutate the input in a way that doesn't change semantics.
Sorting the value set inside SetLookupState would change the behavior of index_in. Hence, we'd also need to modify InitSetLookup to plumb through whether we are initializing is_in or index_in.
Inside Simplify, if we see an is_in call that is unbound, then we immediately fail simplification, since an unbound is_in is not guaranteed to have a sorted and unique value set. We cannot bind the expression on the spot since we don't have access to a schema.
Once we compute the simplified value set, we must avoid re-binding so that we don't re-sorted the value set (and re-construct the memo table). We only re-bind once after all inequalities in the guarantee have been processed.

@felipecrv Does this sound right?

felipecrv · 2024-07-23T02:36:45Z

We change SetLookupState::Init so that it takes the SetLookupOptions& argument without const. When initializing, we sort and remove duplicates from the value set and update the options in place. This effectively makes it so that binding an expression can mutate the input in a way that doesn't change semantics.

It's probably a bad idea to mutate SetLookupOptions. I would simply stored the simplified/equivalent value_set your simplification builds in the KernelState itself.

You don't have to do it in SetLookupState::Init. You can mutate it after the Init as an optional pre-processing step that you do to help the kernel "execute better".

Sorting the value set inside SetLookupState would change the behavior of index_in. Hence, we'd also need to modify InitSetLookup to plumb through whether we are initializing is_in or index_in.

Yeah, don't mutate SetLookupState.

Inside Simplify, if we see an is_in call that is unbound, then we immediately fail simplification, since an unbound is_in is not guaranteed to have a sorted and unique value set. We cannot bind the expression on the spot since we don't have access to a schema.

I don't understand this.

Once we compute the simplified value set, we must avoid re-binding so that we don't re-sorted the value set (and re-construct the memo table). We only re-bind once after all inequalities in the guarantee have been processed.

Yeah. You should be able to look at the KernelState and decide how to proceed.

cpp/src/arrow/compute/kernels/set_lookup_internal.h

cpp/src/arrow/compute/expression.cc

felipecrv · 2024-07-30T19:29:52Z

cpp/src/arrow/compute/expression.cc

+      if (cmp & Comparison::LESS_EQUAL) {
+        lo = mid;
+      } else {
+        hi = mid;
+      }


I suspect this binary-search doesn't necessarily work for all possible comparison operators. I don't have time to think carefully about this yet though.

felipecrv · 2024-08-08T14:28:10Z

@bkietz since you're familiar with this code, can you review it? I don't think I can make progress with the review without checking out the changes and trying the suggestions myself. And that would take a lot of my time at the moment.

mapleFU · 2024-08-13T17:10:01Z

cpp/src/arrow/compute/expression.cc

+  }
+
+    switch (cmp_type.id()) {
+      CASE(UInt8)


Just a naive question, would decimal could be checked here? ( or just not support it yet?)

It seems like supporting decimal types would be easy - the basic decimal C types support comparison operators and the is_in function has kernels for decimal types.

But here you mention that we should leave out floating point types, so should we support decimals but not floats? Or just support both?

bkietz

I think that this needs more tests and should at least initially be simplified to avoid sorting/uniquing sets altogether. Initially winnowing of value sets should be accomplished by filtering with the guarantee.

After this PR merges with a complete set of correctness tests, a follow up PR can add the performance enhancement of slicing sorted value sets (ideally with a benchmark to demonstrate the improvement for large value sets/many guarantees/...)

bkietz · 2024-08-19T15:40:34Z

cpp/src/arrow/compute/expression.cc

+    }
+    if (!state->sorted_and_unique_value_set->type()->Equals(*type)) {
+      ARROW_ASSIGN_OR_RAISE(
+          state->sorted_and_unique_value_set,


This is a race condition; a kernel state may be shared across multiple threads its Expression was bound in the main thread and then used in a filter on multiple worker threads. In general it is not safe to mutate kernel state in any way unless you are constructing it as part of a new Expression::Call or otherwise know that only a single thread accesses this kernel state.

Since this is a property which we expect to be a pure function of value_set, it would also be possible to do this with a memoizing accessor like Result<Datum> SetLookupState::SortedAndUniqueValueSet() const; (would need atomics).

However, I don't see how this adds value over just sorting and unique-ing all value sets as part of SetLookupState::Init(). To avoid paying for sorting and unique-ing when the arrays are already sorted and uniqued, bool SetLookupOptions::value_set_is_sorted_and_unique could be added (but it'd be better if we could add optional<bool> is_sorted to ArrayStatistics)

If we are ok with requiring the user to set SetLookupOptions::value_set_is_sorted_and_unique correctly, what do you think about simplifying is_in only if this option is set? (This was the original approach, but we later switched to attempting to have SimplifyWithGuarantee automatically sort/unique the value set without requiring the user to do anything).

I think that this should at least initially be simplified to avoid sorting/uniquing sets altogether. Initially winnowing of value sets should be accomplished by filtering with the guarantee.

It's true that SetLookupOptions::value_set_is_sorted_and_unique is a bit odd since it's not used directly by the kernel, which is why I'd prefer that if we add the optimization that we wait and do so through ArrayStatistics instead since that doesn't need weird modification of the options for just one function

bkietz · 2024-08-19T16:47:06Z

cpp/src/arrow/compute/expression_test.cc

+  auto is_in = [](Expression field, std::shared_ptr<DataType> value_set_type,
+                  std::string json_array) {
+    SetLookupOptions options{ArrayFromJSON(value_set_type, json_array),
+                             SetLookupOptions::MATCH};


The PR currently simplifies every is_in call but you only test for null_matching_behavior=MATCH. Please either restrict the simplification to MATCH or test for all null matching behaviors

bkietz · 2024-08-19T16:52:07Z

cpp/src/arrow/compute/expression_test.cc

+  Simplify{is_in(field_ref("i32"), int32(), "[null]")}
+      .WithGuarantee(greater(field_ref("i32"), literal(2)))
+      .Expect(false);


I think this is incorrect; the is_in call is simplified to false but if null_matching_behavior=MATCH then i32 IS IN [null] should produce a true in every slot where i32 was null.

I think this is subtle enough to benefit from testing evaluation against real data: for some input data (maybe random, just filter by the guarantee to produce an array which satisfies the guarantee), evaluate the original expression and also the simplified expression then assert that the two results are identical.

Not sure if I follow, the guarantee i32 > 2 itself guarantees that i32 is non-null.

bkietz · 2024-08-19T17:03:13Z

cpp/src/arrow/compute/expression.cc

+        value_set = value_set->Slice(0, pivot);
+        break;
+      case Comparison::LESS_EQUAL:
+        value_set = value_set->Slice(0, pivot + (found ? 1 : 0));


These will always slice null out of the value set, which is not correct for all null handling behaviors

Nulls are already removed in PrepareIsInValueSet. But as you pointed out above this is incorrect if the null matching behavior is INCONCLUSIVE. I'll handle this by skipping simplification in this case.

larry98 · 2024-08-19T20:40:34Z

I think that this needs more tests and should at least initially be simplified to avoid sorting/uniquing sets altogether. Initially winnowing of value sets should be accomplished by filtering with the guarantee.

After this PR merges with a complete set of correctness tests, a follow up PR can add the performance enhancement of slicing sorted value sets (ideally with a benchmark to demonstrate the improvement for large value sets/many guarantees/...)

I opened #43761 which contains the basic implementation.

larry98 · 2024-09-04T18:21:26Z

@bkietz before I rebase these changes on top of #43761, do you have any thoughts on the high level approach? Specifically

Any concerns with using the SimplificationContext unordered map to defer binding the simplified is_in exprs until all inequalities have been processed?
Any thoughts on how to use ArrayStatistics for this optimization? It appears that the statistics class was added, but they aren't computed anywhere. Also, transparently updating ArrayStatistics::is_sorted across calls to SortIndices and Take doesn't seem straightforward. Were you thinking that ArrayStatistics::is_sorted would be manually set by the caller?
It sounds like we're ok with storing the memoized sorted and unique value set in the kernel state, even though it's not used by the kernel?

### Rationale for this change Prior to #43256, this PR adds a basic implementation that does a linear scan filter over the value set on each guarantee. This isolates the correctness/semantics of `is_in` predicate simplification from the binary search performance optimization. ### What changes are included in this PR? `SimplifyWithGuarantee` now handles `is_in` expressions. ### Are these changes tested? A new unit test was added to arrow-compute-expression-test testing this change. ### Are there any user-facing changes? No. * GitHub Issue: #43187 Lead-authored-by: Larry Wang <[email protected]> Co-authored-by: larry98 <[email protected]> Co-authored-by: Benjamin Kietzman <[email protected]> Signed-off-by: Benjamin Kietzman <[email protected]>

bkietz · 2024-09-10T20:01:41Z

My main note when this PR is rebased is I'd like to see benchmarks comparing the sort-and-slice simplification to the basic filtering one. My intuition is that we'll only prefer sort-and-slice for larger value sets and it'd be best to measure the threshold.

It sounds like we're ok with storing the memoized sorted and unique value set in the kernel state

The issue is less about where state should be stored and more about the potential for race conditions when updating KernelState. I'm not opposed to adding memoizations to KernelState, but I don't think it will be necessary here.

Any thoughts on how to use ArrayStatistics for this optimization?

A user can set the flag explicitly, but mostly: after the first time a value set is simplified by this optimization, the optimization itself can set that flag (and then subsequent simplifications of the same value set will be able to skip sorting).

Any concerns with using the SimplificationContext unordered map to defer binding

This does add complexity and I'd prefer to avoid it if we can. One approach to reducing the cost of binding I'd prefer is pattern matching against the guarantee to extract as much of it as is usable by SimplifyIsIn. For example parquet statistics frequently produces guarantee expressions like (a > 3 and a < 9) or a is null which either filtering or sort-and-slice could use in one go.

…pache#43761) ### Rationale for this change Prior to apache#43256, this PR adds a basic implementation that does a linear scan filter over the value set on each guarantee. This isolates the correctness/semantics of `is_in` predicate simplification from the binary search performance optimization. ### What changes are included in this PR? `SimplifyWithGuarantee` now handles `is_in` expressions. ### Are these changes tested? A new unit test was added to arrow-compute-expression-test testing this change. ### Are there any user-facing changes? No. * GitHub Issue: apache#43187 Lead-authored-by: Larry Wang <[email protected]> Co-authored-by: larry98 <[email protected]> Co-authored-by: Benjamin Kietzman <[email protected]> Signed-off-by: Benjamin Kietzman <[email protected]>

larry98 · 2024-10-04T15:28:48Z

I haven't gotten around to writing the benchmark yet, but here is what the rest of the code looks like. lmk if you have any preliminary thoughts.

github-actions bot added Component: C++ awaiting review Awaiting review labels Jul 15, 2024

larry98 changed the title ~~[C++] Support is_in predicates for SimplifyWithGuarantee~~ GH-43187 [C++] Support is_in predicates for SimplifyWithGuarantee Jul 15, 2024

larry98 mentioned this pull request Jul 15, 2024

[C++] Support SimplifyWithGuarantee for is_in expressions #43187

Closed

mapleFU self-requested a review July 15, 2024 16:00

larry98 force-pushed the isin-simplify branch from 67c9d4c to afd5288 Compare July 15, 2024 16:30

zanmato1984 reviewed Jul 16, 2024

View reviewed changes

cpp/src/arrow/compute/expression.h Outdated Show resolved Hide resolved

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jul 16, 2024

github-actions bot added the Component: Python label Jul 16, 2024

mapleFU reviewed Jul 17, 2024

View reviewed changes

felipecrv requested changes Jul 21, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Jul 21, 2024

github-actions bot added awaiting change review Awaiting change review and removed Component: Python awaiting changes Awaiting changes labels Jul 24, 2024

mapleFU reviewed Jul 27, 2024

View reviewed changes

cpp/src/arrow/compute/kernels/set_lookup_internal.h Show resolved Hide resolved

larry98 commented Jul 29, 2024

View reviewed changes

cpp/src/arrow/compute/expression.cc Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jul 30, 2024

felipecrv requested changes Jul 30, 2024

View reviewed changes

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Jul 31, 2024

felipecrv requested a review from bkietz August 8, 2024 14:26

mapleFU reviewed Aug 13, 2024

View reviewed changes

bkietz requested changes Aug 19, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Aug 19, 2024

larry98 mentioned this pull request Aug 19, 2024

GH-43187: [C++] Support basic is_in predicate simplification #43761

Merged

larry98 added 2 commits October 1, 2024 07:22

avoid repeated binds when simplifying is_in

2bef6fd

implement fast, binary-search simplification

0469a8f

larry98 force-pushed the isin-simplify branch from 862aafa to 0469a8f Compare October 4, 2024 15:26

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 4, 2024

GH-43187 [C++] Support is_in predicates for SimplifyWithGuarantee #43256

Are you sure you want to change the base?

GH-43187 [C++] Support is_in predicates for SimplifyWithGuarantee #43256

Conversation

larry98 commented Jul 15, 2024 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Jul 15, 2024

github-actions bot commented Jul 15, 2024

zanmato1984 left a comment

Choose a reason for hiding this comment

larry98 commented Jul 16, 2024

zanmato1984 commented Jul 17, 2024

mapleFU left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanmato1984 commented Jul 17, 2024

mapleFU commented Jul 17, 2024 • edited Loading

zanmato1984 commented Jul 17, 2024

mapleFU commented Jul 17, 2024

mapleFU commented Jul 17, 2024

larry98 commented Jul 17, 2024

Choose a reason for hiding this comment

felipecrv commented Jul 21, 2024

larry98 commented Jul 22, 2024 • edited Loading

felipecrv commented Jul 23, 2024

Choose a reason for hiding this comment

felipecrv commented Aug 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkietz 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

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

larry98 commented Aug 19, 2024

larry98 commented Sep 4, 2024

bkietz commented Sep 10, 2024

larry98 commented Oct 4, 2024

larry98 commented Jul 15, 2024 •

edited by github-actions bot

Loading

mapleFU commented Jul 17, 2024 •

edited

Loading

larry98 commented Jul 22, 2024 •

edited

Loading