[Velox To Substrait] Remove hard-coded function references by chaojun-zhang · Pull Request #2496 · facebookincubator/velox

chaojun-zhang · 2022-09-09T11:00:38Z

Replace hard-coded function reference mapping with the one built on the fly.
Assign unique sequential reference numbers to scalar and aggregate functions
used in the plan.

netlify · 2022-09-09T11:00:43Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`137f3cf`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/63291261f3d88700095125a0

mbasmanova

@chaojun-zhang This PR has lots of changes and it difficult to review. Can you break it up into smaller pieces? Also, looks like there are no additional tests. How is the new functionality verified?

velox/substrait/TypeUtils.h

mbasmanova · 2022-09-09T12:38:13Z

velox/substrait/TypeUtils.h

 TypePtr toVeloxType(const std::string& typeName);

+/// Return the Substrait type signature according to the type
+std::string substraitSignature(const TypePtr& type);


Is this the opposite of the toVeloxType function above? If so, let's rename toSubstraitType.

To make this PR smaller and easier to remove, please, extract this new function and its test into a separate PR.

No, it only convert to substrait type signature which can't be convert backed to velox type, for more detail information please refer to substrait signature here

velox/substrait/TypeUtils.cpp

mbasmanova · 2022-09-09T12:39:56Z

velox/substrait/tests/TypeUtilsTest.cpp

+      const TypePtr& type,
+      const std::string& expectedType) {
+    SCOPED_TRACE(type->toString());
+    auto substraitType = substraitSignature(type);


Do we have tests for toVeloxType function? If not, let's add these. Perhaps, write a round trip test.

Already have test for toVeloxType function here

toSubstraitSignature is not an opposite of toVeloxType

velox/substrait/VeloxToSubstraitType.cpp

mbasmanova · 2022-09-09T12:50:38Z

velox/substrait/SubstraitToVeloxExpr.cpp


+std::shared_ptr<const core::ITypedExpr>
+SubstraitVeloxExprConverter::toVeloxExpr(
+    const ::substrait::Expression_IfThen& substraitIfThen,


Is this an IF expression or a SWITCH expression?

Can you extract this change into a separate PR and add a test?

velox/substrait/SubstraitExtensionCollector.h

velox/substrait/SubstraitExtensionCollector.cpp

mbasmanova · 2022-09-09T12:56:29Z

velox/substrait/SubstraitExtensionCollector.cpp

+      typeSignature.emplace_back(substraitSignature(arg));
+    }
+    std::string signature = funcName + ":" + folly::join("_", typeSignature);
+    return {"", signature};


Why is uri empty?

Currently we don't introduce any substrait extension YAML files instead we treat all velox function signature as custom substrait function variant, so leave it empty.

velox/substrait/SubstraitExtensionCollector.cpp

chaojun-zhang · 2022-09-11T14:50:32Z

This PR has lots of changes and it difficult to review. Can you break it up into smaller pieces

Done

Also, looks like there are no additional tests. How is the new functionality verified

This Test already covered this new functionality

mbasmanova

@chaojun-zhang Thank you for iterating on this PR. Some follow-up comments.

mbasmanova · 2022-09-13T14:50:34Z

velox/substrait/SubstraitExtensionCollector.h

+  }
+};
+
+/// A ExtensionIdResolver is intend to used to resolve functionId or typeId


There are multiple typos and the description could be streamlined.

Given a function name and argument types returns a matching FunctionId.

However, I'm confused about the term "resolver". I don't see the implementation resolving anything. Instead, the implementation simply converts argument types into an underscore-delimited string using base type names and prepend it with function name and a colon.

Maybe we could have a free function toFunctionId(functionName, arguments) instead? Or add a static method to FunctionId struct.

static FunctionId FunctionId::create(functionName, arguments).

mbasmanova · 2022-09-13T14:51:06Z

velox/substrait/SubstraitExtensionCollector.h

+/// A ExtensionIdResolver is intend to used to resolve functionId or typeId
+class ExtensionIdResolver {
+ public:
+  /// resolve functionId by given velox function name and function


If you update the class comment as suggested above, this comment can be removed as it will just repeat the class comment.

mbasmanova · 2022-09-13T14:56:41Z

velox/substrait/SubstraitExtensionCollector.h

+
+using ExtensionIdResolverPtr = std::shared_ptr<const ExtensionIdResolver>;
+
+/// Maintains a mapping for function and function reference


Comments must end with a period. Please, fix throughout the PR.

Perhaps, clarify this comment a bit. "Assigns unique IDs to function signatures using FunctionId."

mbasmanova · 2022-09-13T14:56:52Z

velox/substrait/SubstraitExtensionCollector.h

+class SubstraitExtensionCollector {
+ public:
+  SubstraitExtensionCollector();
+  /// get function reference by function name and arguments.


Add empty line between methods. Fix throughout.

mbasmanova · 2022-09-13T14:57:34Z

velox/substrait/SubstraitExtensionCollector.h

+/// Maintains a mapping for function and function reference
+class SubstraitExtensionCollector {
+ public:
+  SubstraitExtensionCollector();


This constructor is not needed. Member variables can be initialized inline.

I try to remove it, but it report the bi-direction map not initialized properly

mbasmanova · 2022-09-13T15:17:13Z

velox/substrait/tests/SubstraitExtensionCollectorTest.cpp

+};
+
+TEST_F(SubstraitExtensionCollectorTest, getFunctionReferenceTest) {
+  assertGetFunctionReference("plus", {INTEGER(), INTEGER()}, 0);


This test would be more readable if function name is shortened and assertion is moved to the caller.

ASSERT_EQ(getFunctionReference("plus", "{INTEGER(), INTEGER()}", 0);

mbasmanova · 2022-09-13T15:17:55Z

velox/substrait/tests/SubstraitExtensionCollectorTest.cpp

+
+TEST_F(SubstraitExtensionCollectorTest, getFunctionReferenceTest) {
+  assertGetFunctionReference("plus", {INTEGER(), INTEGER()}, 0);
+  assertGetFunctionReference("plus", {INTEGER(), INTEGER()}, 0);


Instead of repeating the call and verifying the result is the same, consider, modifying the helper function to always repeat the call and assert the same result. This way we'll get more test coverage with fewer lines of test code.

mbasmanova · 2022-09-13T15:18:32Z

velox/substrait/tests/SubstraitExtensionCollectorTest.cpp

+  assertGetFunctionReference("divide", {INTEGER(), INTEGER()}, 1);
+}
+
+TEST_F(SubstraitExtensionCollectorTest, addExtensionsToPlanTest) {


naming: same as above; drop Test suffix from the test name.

mbasmanova · 2022-09-13T15:20:02Z

velox/substrait/tests/SubstraitExtensionCollectorTest.cpp

+TEST_F(SubstraitExtensionCollectorTest, getFunctionReferenceTest) {
+  assertGetFunctionReference("plus", {INTEGER(), INTEGER()}, 0);
+  assertGetFunctionReference("plus", {INTEGER(), INTEGER()}, 0);
+  assertGetFunctionReference("divide", {INTEGER(), INTEGER()}, 1);


Let's add tests for functions taking complex types, for example, cardinality, array_sum, transform_keys. Let's also add tests for aggregate functions: sum, count. Let's make sure to tests both partial and final aggregate function calls.

mbasmanova · 2022-09-13T15:20:34Z

velox/substrait/tests/SubstraitExtensionCollectorTest.cpp

+}
+
+TEST_F(SubstraitExtensionCollectorTest, addExtensionsToPlanTest) {
+  extensionCollector_->getFunctionReference("plus", {INTEGER(), INTEGER()});


Let's add a few more functions. Let's add functions with complex type inputs as well as aggregate functions. Let's also add "duplicate" functions to make sure these are properly de-duplicated.

mbasmanova

@chaojun-zhang Thank you for making further improvements. VeloxSubstraitSignature has a good amount of logic and can be reviewed standalone. Consider extracting it and the corresponding test into a separate PR.

mbasmanova · 2022-09-15T14:51:25Z

velox/substrait/VeloxSubstraitSignature.h

+  /// signature.
+  static std::string toVeloxSignature(
+      const std::string& functionName,
+      const std::vector<facebook::velox::TypePtr>& inputs);


drop facebook::velox:: prefix

OK, will have a further PR.

mbasmanova · 2022-09-15T14:51:43Z

velox/substrait/VeloxSubstraitSignature.h

+  /// Given a collection of function signature, return the velox function
+  /// signatures join by the ',' delimiter.
+  static std::string toVeloxSignature(
+      const std::vector<const facebook::velox::exec::FunctionSignature*>&


drop facebook::velox:: prefix

mbasmanova · 2022-09-15T15:02:40Z

velox/substrait/tests/VeloxSubstraitSignatureTest.cpp

+
+using namespace facebook::velox;
+using namespace facebook::velox::substrait;
+#include "velox/functions/prestosql/registration/RegistrationFunctions.h"


Put all #includes together before "using namespace" directives.

mbasmanova · 2022-09-15T15:02:53Z

velox/substrait/tests/VeloxSubstraitSignatureTest.cpp

+    Test::SetUp();
+    functions::prestosql::registerAllScalarFunctions();
+  }
+  static std::string toSubstraitSignature(const TypePtr& type) {


add empty line

mbasmanova · 2022-09-15T15:03:28Z

velox/substrait/tests/VeloxSubstraitSignatureTest.cpp

+  }
+};
+
+TEST_F(VeloxSubstraitSignatureTest, toSubstraitSignature_with_type) {


naming: toSubstraitSignature_with_type -> toSubstraitSignatureWithType

Please fix throughout.

mbasmanova · 2022-09-15T15:04:21Z

velox/substrait/tests/VeloxSubstraitSignatureTest.cpp

+
+namespace facebook::velox::substrait::test {
+
+class VeloxSubstraitSignatureTest : public ::testing::Test {


Can we add tests for resolveFunction API?

mbasmanova · 2022-09-15T15:05:09Z

velox/substrait/VeloxSubstraitSignature.h

+
+  /// Given a velox function name and argument types, return a matching function
+  /// signature, throw if no match found.
+  static const exec::FunctionSignature& resolveFunction(


This function handles both scalar and aggregate functions and includes a hack for aggregate functions that assumes that single argument means aggregate function runs in final aggregation stage. It would be cleaner to handle scalar and aggregate functions separately. When resolving aggregate function, the caller needs to specify the aggregation step in addition to function name and input types.

Ok, move to further PR.

mbasmanova · 2022-09-15T15:05:49Z

velox/substrait/VeloxSubstraitSignature.cpp

+
+std::string VeloxSubstraitSignature::toSubstraitSignature(
+    const exec::TypeSignature& typeSignature) {
+  if ("T" == typeSignature.baseType() ||


Would it be possible to look at the type variables specified in the function signature to avoid hard-coding some of these?

I assume this won't work for functions like map_keys which use K and V type variables.

Yes, will do in further PR.

mbasmanova · 2022-09-15T15:06:06Z

velox/substrait/VeloxSubstraitSignature.cpp

+  if (functionName == "and" || functionName == "or" || functionName == "xor") {
+    return functionName + ":bool_bool";
+  }
+  if (functionName == "not") {


Why do we need to hard-code "not" function. Is should be present in the registry. Is it not?

'not' function not present in the registry currently, do you think it should be present in registry?

Yes, I expect it to be registered.

velox/functions/prestosql/registration/ArithmeticFunctionsRegistration.cpp

void registerArithmeticFunctions() { registerSimpleFunctions(); VELOX_REGISTER_VECTOR_FUNCTION(udf_not, "not"); VELOX_REGISTER_VECTOR_FUNCTION(udf_decimal_add, "plus"); VELOX_REGISTER_VECTOR_FUNCTION(udf_decimal_sub, "minus"); VELOX_REGISTER_VECTOR_FUNCTION(udf_decimal_mul, "multiply"); }

add test case to cover the logical functions, compare functions and aggregate functions. etc.

…cRef # Conflicts: # velox/substrait/TypeUtils.cpp # velox/substrait/VeloxToSubstraitExpr.cpp

chaojun-zhang · 2022-09-16T06:33:55Z

@chaojun-zhang Thank you for making further improvements. VeloxSubstraitSignature has a good amount of logic and can be reviewed standalone. Consider extracting it and the corresponding test into a separate PR.

Ok, I will raise a new PR for this.

…cRef

… more tests

…cRef # Conflicts: # velox/substrait/VeloxToSubstraitPlan.cpp

mbasmanova

@chaojun-zhang Thank you for iterating on the PR. I still think it would be easier to review if the PR was split into smaller pieces. For example VeloxSubstraitSignature class and its test could be extracted into a separate PR.

I noticed that there are some limitations re: lambda functions (these are not supported) and aggregate functions (the step is not used, but the implications are not clear to me). Would you update PR description to call out these explicitly?

mbasmanova · 2022-09-19T10:06:42Z

velox/substrait/VeloxToSubstraitPlan.cpp

-    } else {
-      VELOX_NYI("Couldn't find the aggregate function '{}' ", funName);
-    }
+    auto referenceId = extensionCollector_->getReferenceNumber(


nit: referenceId -> referenceNumber

mbasmanova · 2022-09-19T10:09:55Z

velox/substrait/VeloxSubstraitSignature.h

+class VeloxSubstraitSignature {
+ public:
+  /// Given a velox type kind, return the Substrait type signature, throw if no
+  /// match found, Substrait signature used in the function extension


Most of this comment seems to belong to the other method. Please, update.

mbasmanova · 2022-09-19T10:11:35Z

velox/substrait/SubstraitExtensionCollector.h

+ private:
+  /// A bi-direction hash map to keep the relation between reference number and
+  /// either function or type signature.
+  /// @T ExtensionFunctionId


Use @tparam

mbasmanova · 2022-09-19T10:19:18Z

velox/substrait/SubstraitExtensionCollector.h

+  template <class T>
+  class BiDirectionHashMap {
+   public:
+    /// If the forwardMap_ does not contain the key, then the key and value will


This behavior seems difficult to work with. For the bi-map, users would expect that the map enforces uniqueness of both keys and values, no?

Consider, what happens when we add the following:

1, 10 => {1 -> 10}, {10 ->1}

1, 20 => {1 -> 20}, {10 ->1, 20 -> 1}

2, 20 => {1 -> 20, 2 -> 20}, {10 -> 1, 20 -> 2}

mbasmanova · 2022-09-19T10:21:08Z

velox/substrait/SubstraitExtensionCollector.h

+    /// otherwise the key will be overwritten.
+    void put(const int& key, const T& value);
+
+    std::map<int, T>& forwardMap() {


These methods should be const and should return const &. Also, consider replacing these with lookups by key and value, i.e.

std::optional<int> findKey(value) const; std::optional<T> findValue(key) const;

change to const is ok, but if replace with lookup methods that won't work for addExtensionsToPlan that need to iterate over the forwardMap

I see. I guess we still can make this method const and have it return const &. No need to provide write access to forwardMap_, right?

Yes, updated

mbasmanova · 2022-09-19T10:23:08Z

velox/substrait/SubstraitExtensionCollector.cpp

+    const std::vector<TypePtr>& arguments) {
+  const auto& substraitFunctionSignature =
+      VeloxSubstraitSignature::toSubstraitSignature(functionName, arguments);
+  /// TODO: Currently we treat all velox registry based function signatures as


Use // for comments inside functions.

mbasmanova · 2022-09-19T10:24:00Z

velox/substrait/SubstraitExtensionCollector.cpp

+    const std::string& functionName,
+    const std::vector<TypePtr>& arguments,
+    const core::AggregationNode::Step aggregationStep) {
+  // TODO: Ignore aggregationStep for now, will refactor when introduce velox


Why is it OK to ignore the step? There is already a registry of aggregate functions. What's missing?

Because we make an agreement that separate the velox registry to a new PR.

mbasmanova · 2022-09-19T10:26:14Z

velox/substrait/tests/SubstraitExtensionCollectorTest.cpp

+  int getReferenceNumber(
+      const std::string& functionName,
+      std::vector<TypePtr>&& arguments) {
+    int functionReferenceId1 =


nit: functionReferenceId1 -> referenceNumber1

mbasmanova · 2022-09-19T10:26:44Z

velox/substrait/tests/SubstraitExtensionCollectorTest.cpp

+  int getReferenceNumber(
+      const std::string& functionName,
+      std::vector<TypePtr>&& arguments,
+      const core::AggregationNode::Step step) {


mbasmanova · 2022-09-19T10:28:05Z

velox/substrait/tests/SubstraitExtensionCollectorTest.cpp

+}
+
+TEST_F(SubstraitExtensionCollectorTest, addExtensionsToPlan) {
+  // Arrange


What does this comment mean? Please, make sure all comments end with a period.

It's a unit test pattern (Arrange->Act->Assert), have a look at this link

Thank you for the pointer. I didn't know about this acronym. I do agree though with the author of that blog post that // Arrange and similar comments are redundant. The rest of the code base doesn't use these, hence, let's not add them here.

Test code, in the same way as production code, is supposed to be clean and self-explanatory. Preempting what you are going to do just adds unnecessary clutter. You can still have a clear distinction of the three steps by using spaces and indentation. Which are ultimately the key features the compiler provides you to structure your code. It does look silly having a comment preceding each paragraph in this text, it tires the reader, as it has to constantly go through the obvious (or skip it). So why would you do that with your code?

On a separate note, this test would be easier to read if assertions were refactored to use a helper function.

auto getFunctionName = [&] (auto id) { return substraitPlan->extensions(0).extension_function().name(); }; ASSERT_EQ(getFunctionName(0), "plus:i32_i32"); ASSERT_EQ(getFunctionName(1), "divide:i32_i32"); ...

chaojunzhang · 2022-09-19T11:39:34Z

@chaojun-zhang Thank you for iterating on the PR. I still think it would be easier to review if the PR was split into smaller pieces. For example VeloxSubstraitSignature class and its test could be extracted into a separate PR.

I noticed that there are some limitations re: lambda functions (these are not supported) and aggregate functions (the step is not used, but the implications are not clear to me). Would you update PR description to call out these explicitly?

If the VeloxSubstraitSignature extract to a separate PR then this PR won't be complete functional, we always need the logic toSubstraitSignature , if you think it won't belong to class of 'VeloxSubstraitSignature' , then it must be put into other place which can't be remove.

mbasmanova · 2022-09-19T11:52:03Z

If the VeloxSubstraitSignature extract to a separate PR then this PR won't be complete functional, we always need the logic toSubstraitSignature

My thinking is that this PR will depend on the PR introducing VeloxSubstraitSignature. I.e. we introduce VeloxSubstraitSignature in PR 1, then start using it in PR 2.

Modify forwardMap_ type to use unordered_map.

mbasmanova · 2022-09-20T14:00:38Z

velox/substrait/SubstraitExtensionCollector.cpp

+void SubstraitExtensionCollector::BiDirectionHashMap<T>::putIfAbsent(
+    const int& key,
+    const T& value) {
+  if (forwardMap_.find(key) == forwardMap_.end()) {


Shouldn't we check both maps first, then insert a (key, value) only if forwardMap_ doesn't have key and reverseMap_ doesn't have value?

facebook-github-bot · 2022-09-20T14:01:39Z

@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

mbasmanova

@chaojun-zhang Thank you for the contribution.

I'll fix putIfAbsent and update PR description before landing.

template <typename T>
bool SubstraitExtensionCollector::BiDirectionHashMap<T>::putIfAbsent(
    const int& key,
    const T& value) {
  if (forwardMap_.find(key) == forwardMap_.end() &&
      reverseMap_.find(value) == reverseMap_.end()) {
    forwardMap_[key] = value;
    reverseMap_[value] = key;
    return true;
  }
  return false;
}

Lambda functions are not supported. Support for aggregated functions is limited.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 9, 2022

mbasmanova reviewed Sep 9, 2022

View reviewed changes

remove hard code function reference mapping

a21ba57

chaojun-zhang added 2 commits September 11, 2022 21:48

use extension collector instead of hardcode function reference

2f11218

revert change for JsonToProtoConverter.cpp

3b6a5d0

mbasmanova reviewed Sep 13, 2022

View reviewed changes

mbasmanova changed the title ~~[Velox To Substrait] remove hard code function reference mapping~~ [Velox To Substrait] Remove hard-coded function references Sep 13, 2022

mbasmanova mentioned this pull request Sep 13, 2022

fix: nullability semantics of predicates in JoinRel and FilterRel substrait-io/substrait-validator#52

Merged

4 tasks

mbasmanova reviewed Sep 15, 2022

View reviewed changes

chaojun-zhang added 8 commits September 15, 2022 18:20

use velox registry to resolve function signature

9db857b

add test case to cover the logical functions, compare functions and aggregate functions. etc.

Merge remote-tracking branch 'upstream/main' into remove_hardcode_fun…

5eb24fb

…cRef # Conflicts: # velox/substrait/TypeUtils.cpp # velox/substrait/VeloxToSubstraitExpr.cpp

Comments tidy

569e001

Add more comments for signature

adde313

Add more comments

07a46ed

fix code format issue

10e8dc6

fix code format issue

88552e5

Fix all comments to start with a capital letter and end with a period

ad2ae36

chaojun-zhang and others added 6 commits September 16, 2022 12:04

Merge remote-tracking branch 'upstream/main' into remove_hardcode_fun…

dd51cbe

…cRef

seperate getReferenceNumber for scalar and aggregate function and add…

d6dac50

… more tests

code format fix

c6583f7

Simplify to only handle one URI

c0570f6

Add comment for substrait type signature

2a542f9

Merge remote-tracking branch 'upstream/main' into remove_hardcode_fun…

1a8ce2b

…cRef # Conflicts: # velox/substrait/VeloxToSubstraitPlan.cpp

mbasmanova reviewed Sep 19, 2022

View reviewed changes

fix for variable names and comments

4d456e6

Modify put method to ensure keys and values are unique.

137f3cf

Modify forwardMap_ type to use unordered_map.

mbasmanova reviewed Sep 20, 2022

View reviewed changes

mbasmanova approved these changes Sep 20, 2022

View reviewed changes

facebook-github-bot closed this in 6a97c9e Sep 20, 2022

marin-ma pushed a commit to marin-ma/velox-oap that referenced this pull request Dec 15, 2023

[VL] Adjust scopes of dependencies of gluten-it (facebookincubator#2496)

496cda0


		using ExtensionIdResolverPtr = std::shared_ptr<const ExtensionIdResolver>;

		/// Maintains a mapping for function and function reference


		namespace facebook::velox::substrait::test {

		class VeloxSubstraitSignatureTest : public ::testing::Test {

Conversation

chaojun-zhang commented Sep 9, 2022 • edited by mbasmanova Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Sep 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for meta-velox canceled.

Uh oh!

mbasmanova left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chaojun-zhang commented Sep 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

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!

mbasmanova left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chaojun-zhang commented Sep 9, 2022 •

edited by mbasmanova

Loading

netlify bot commented Sep 9, 2022 •

edited

Loading

chaojun-zhang commented Sep 11, 2022 •

edited

Loading