Add SimpleFunctionMetadata toDebugString function #10821

jinchengchenghh · 2024-08-23T06:30:20Z

Print all the Metadata fields such a logical and physical argument type and result type.

Logical signature: (decimal(i1,i5), integer) -> decimal(i2,i5)
Physical signature: (HUGEINT, INTEGER) -> HUGEINT
Priority: 999997
DefaultNullBehavior: true

netlify · 2024-08-23T06:30:35Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`f980aba`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/6709ea8aa318cd0008889ba5

jinchengchenghh · 2024-08-26T01:58:21Z

Could you help review? Thanks! @kgpai

jinchengchenghh · 2024-08-27T06:16:05Z

Can you help review? Thanks! @Yuhta

jinchengchenghh · 2024-09-10T01:27:40Z

hi @xiaoxmeng @mbasmanova , can you help review this PR? Thanks!

mbasmanova · 2024-09-11T11:18:46Z

velox/expression/tests/SimpleFunctionTest.cpp

+  EXPECT_TRUE(function.has_value());
+  EXPECT_EQ(
+      function.value().helpMessage(functionName),
+      "FunctionName: decimal_plus_value\nSignature argument types:\nDECIMAL(I1,I5), BIGINT\n"


I find the new message hard to read. What problem are you trying to solve?

Before this PR, we don't print the Metadata physical argument and result types, and other fields like priority_ and defaultNullBehavior_ that user may wish to know.
If we use std::cout to print it, we will get

FunctionName: decimal_plus_value Signature argument types: DECIMAL(I1,I5), BIGINT Physical argument types: HUGEINT, BIGINT Physical result types: HUGEINT Priority:999997 DefaultNullBehavior:true

This message might be helpful to developers, but not users.

Can we keep existing message as is, but introduce a separate method for debugging?

Ok, can I name the new method toString?

Sure, this should work.

jinchengchenghh · 2024-09-23T02:32:33Z

Can you help review again? Thanks! @mbasmanova

mbasmanova

@jinchengchenghh Let's update PR description to provide an example of the output.

FunctionName: decimal_plus_value
Signature argument types:
DECIMAL(I1,I5), BIGINT
Physical argument types:
HUGEINT, BIGINT
Physical result types:
HUGEINT
Priority:999997
DefaultNullBehavior:true

Also, let's streamline this a bit.

Remove function name (it is not part of the signature).
Add logical result type.
Put arguments and result type on a single line.
Add space after Priority: and DefaultNullBehavior:.

Logical signature: (DECIMAL(I1,I5), BIGINT) -> <result type>
Physical signature: (HUGEINT, BIGINT) -> HUGEINT
Priority: 999997
DefaultNullBehavior: true

mbasmanova · 2024-09-23T11:39:05Z

velox/core/SimpleFunctionMetadata.h

@@ -394,6 +394,7 @@ class ISimpleFunctionMetadata {
  virtual bool physicalSignatureEquals(
      const ISimpleFunctionMetadata& other) const = 0;
  virtual std::string helpMessage(const std::string& name) const = 0;
+  virtual std::string toString(const std::string& name) const = 0;


Let's rename to toDebugString and remove 'name' parameter.

Let's document this method.

mbasmanova · 2024-09-23T11:40:14Z

velox/core/SimpleFunctionMetadata.h

@@ -536,6 +537,44 @@ class SimpleFunctionMetadata : public ISimpleFunctionMetadata {
    return s;
  }

+  std::string toString(const std::string& name) const final {
+    std::stringstream sig;


do not abbreviate: sig -> signature

It is strange that we do not include the result type here. Let's add it.

You seem to be building signatures twice: once using logical types and once again using physical types. If that's the case, then let's name variables to match: logicalSignature and physicalSignature. Let's make sure both signatures include argument types and return type.

mbasmanova · 2024-09-23T11:44:00Z

velox/core/SimpleFunctionMetadata.h

+    }
+
+    std::stringstream ss;
+    first = true;


This code repeats. Let's extract a small helper template:

auto logicalSignature = toSignatureString(signature_->argumentTypes(), resultType_); auto physicalSignature = toSignatureString(argPhysicalTypes_, resultPhysicalType_);

mbasmanova · 2024-09-23T11:44:48Z

velox/core/SimpleFunctionMetadata.h

+    }
+    return fmt::format(
+        "FunctionName: {}\nSignature argument types:\n{}\nPhysical argument types:\n{}"
+        "\nPhysical result types:\n{}\nPriority:{}\nDefaultNullBehavior:{}",


Make sure to add empty space after Priority: and nDefaultNullBehavior:

jinchengchenghh · 2024-10-12T03:20:06Z

Can you help review again? Thanks! @mbasmanova

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 Aug 23, 2024

jinchengchenghh force-pushed the metadata branch from f73086e to a011246 Compare September 3, 2024 07:56

mbasmanova requested changes Sep 11, 2024

View reviewed changes

jinchengchenghh mentioned this pull request Sep 12, 2024

Fix register simple function SignatureMap overwrite #10811

Open

jinchengchenghh force-pushed the metadata branch from a011246 to fea5906 Compare September 14, 2024 02:06

jinchengchenghh changed the title ~~Enhance SimpleFunctionMetadata helpMessage function~~ Add SimpleFunctionMetadata toString function Sep 14, 2024

mbasmanova requested changes Sep 23, 2024

View reviewed changes

jinchengchenghh force-pushed the metadata branch from b389930 to dcffc48 Compare September 24, 2024 04:56

jinchengchenghh changed the title ~~Add SimpleFunctionMetadata toString function~~ Add SimpleFunctionMetadata toDebugString function Sep 24, 2024

jinchengchenghh force-pushed the metadata branch from dcffc48 to f980aba Compare October 12, 2024 03:18

jinchengchenghh added 2 commits October 12, 2024 10:37

add toDebugString to SimpleFunctionMetadata

b19d7fc

address all the comments

f980aba

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SimpleFunctionMetadata toDebugString function #10821

Add SimpleFunctionMetadata toDebugString function #10821

jinchengchenghh commented Aug 23, 2024 •

edited

Loading

netlify bot commented Aug 23, 2024 •

edited

Loading

jinchengchenghh commented Aug 26, 2024

jinchengchenghh commented Aug 27, 2024

jinchengchenghh commented Sep 10, 2024

mbasmanova Sep 11, 2024

jinchengchenghh Sep 12, 2024

mbasmanova Sep 13, 2024 •

edited

Loading

jinchengchenghh Sep 14, 2024

mbasmanova Sep 14, 2024

jinchengchenghh commented Sep 23, 2024

mbasmanova left a comment

mbasmanova Sep 23, 2024

mbasmanova Sep 23, 2024

mbasmanova Sep 23, 2024

mbasmanova Sep 23, 2024

jinchengchenghh commented Oct 12, 2024

Add SimpleFunctionMetadata toDebugString function #10821

Are you sure you want to change the base?

Add SimpleFunctionMetadata toDebugString function #10821

Conversation

jinchengchenghh commented Aug 23, 2024 • edited Loading

netlify bot commented Aug 23, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

jinchengchenghh commented Aug 26, 2024

jinchengchenghh commented Aug 27, 2024

jinchengchenghh commented Sep 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbasmanova Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinchengchenghh commented Sep 23, 2024

mbasmanova 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

jinchengchenghh commented Oct 12, 2024

jinchengchenghh commented Aug 23, 2024 •

edited

Loading

netlify bot commented Aug 23, 2024 •

edited

Loading

mbasmanova Sep 13, 2024 •

edited

Loading