Use const&

Add VELOX_CHECK that !argumentType.empty()

Do you need the next 3 lines, since the expectedRawInputTypes is empty ?

I'm a bit confused about what is the significance of the event_based_revenue argument in the json if the SQLFunction handle doesn't have any arguments ?

I blindly copied the test cases from https://github.com/prestodb/presto/blob/master/presto-native-execution/presto_cpp/presto_protocol/tests/CallExpressionTest.cpp.
I just needed to have my hands on an instance of a SqlFunctionHandle for the unit test, because the parseSqlFunctionHandle function had a bug which failed in the parsing of the functionId string.
Do you think this is a good way to test it? Please let me know if you have any better ideas.

@pdabre12 : Where do you anticipate this json to be generated ? You can run a query with an aggregate function and pick up the json from the web-console.

It will only be generated for functions in the new function namespace manager : #23358.
Below is the json for the array_constructor function :

                "@type" : "call",
                  "sourceLocation" : {
                    "line" : 1,
                    "column" : 74
                  },
                  "displayName" : "array_constructor",
                  "functionHandle" : {
                    "@type" : "native",
                    "signature" : {
                      "name" : "native.default.array_constructor",
                      "kind" : "SCALAR",
                      "typeVariableConstraints" : [ ],
                      "longVariableConstraints" : [ ],
                      "returnType" : "array(double)",
                      "argumentTypes" : [ "double" ],
                      "variableArity" : false
                    },
                    "functionId" : "native.default.array_constructor;double",
                    "version" : "1"
                  },

The problem is without #23358 landing, native execution does not understand the native functionHandle type, I suppose I could pick the json but keep the functionHandle type as json_file as we really only are testing the parsing of the functionId string because that's where the bug manifests, let me know if that sounds okay.

@pdabre12 : Yes, that is fine. Thanks.

Make a small inline function to take expectedRawInputTypes and sqlFunctionHandle as inputs and do the parse and compare actualRowInputTypes and expectedRawInputTypes.

This snippet is repeated in all the functions.

Again, I'm confused since the type of this variable is real, but the SQLFunctionHandle has an integer parameter.

Please look at the above comment.

To add to the above comment,
This is the string that was present in the CallExpression unit tests.

std::string str = R"(
          {
            "@type": "call",
            "arguments": [
              {
                "@type": "variable",
                "name": "event_based_revenue",
                "type": "real"
              }
            ],
            "displayName": "sum",
            "functionHandle": {
              "@type": "json_file",
              "functionId": "json.x4.sum;INTEGER;INTEGER",
              "version": "1"
            },
            "returnType": "double"
          }
    )"

Can you add a function with all the different input types (like INT, BIGINT, REAL, DOUBLE, VARCHAR etc) to ensure they are all parsed correctly ?

How are complex types handled in this function ? Do you have an example ?

It would be great to add a test corresponding to the error that you saw previously that led you to debugging this issue.

So the error was very generic.
Whenever we tried to parse any SqlFunctionHandle with zero / multiple params, because of the wrong exit condition it lead to a never-ending loop, for eg: "functionId": "json.x4.sum;" failed because of zero params, and "functionId": "json.x4.sum;bigint;bigint;" will fail too.