Fix comments

facebookincubator · Nov 24, 2023 · 00c9c0a · 00c9c0a
1 parent cba7f21
commit 00c9c0a
Show file tree

Hide file tree

Showing 3 changed files with 21 additions and 29 deletions.
diff --git a/velox/docs/functions/spark/json.rst b/velox/docs/functions/spark/json.rst
@@ -22,6 +22,8 @@ JSON Functions
 
 .. spark:function:: get_json_object(json, path) -> varchar
 
-    Extracts a json object from path::
+    Extracts a json object from ``path``. Returns NULL if it finds json string
+    is malformed. ::
 
-        SELECT get_json_object('{"a":"b"}', '$.a'); -- b
+        SELECT get_json_object('{"a":"b"}', '$.a'); -- 'b'
+        SELECT get_json_object('{"a":{"b":"c"}}', '$.a'); -- '{"b":"c"}'
diff --git a/velox/functions/sparksql/SIMDJsonFunctions.h b/velox/functions/sparksql/SIMDJsonFunctions.h
@@ -28,10 +28,10 @@ struct SIMDGetJsonObjectFunction {
   // ASCII input always produces ASCII result.
   static constexpr bool is_default_ascii_behavior = true;
 
+  // Makes a conversion from spark's json path, e.g., converts
+  // "$.a.b" to "/a/b".
   FOLLY_ALWAYS_INLINE std::string getFormattedJsonPath(
       const arg_type<Varchar>& jsonPath) {
-    // Makes a conversion from spark's json path, e.g.,
-    // converts "$.a.b" to "/a/b".
     char formattedJsonPath[jsonPath.size() + 1];
     int j = 0;
     for (int i = 0; i < jsonPath.size(); i++) {
@@ -103,14 +103,12 @@ struct SIMDGetJsonObjectFunction {
         }
       }
       case ondemand::json_type::boolean: {
-        bool boolResult = false;
-        rawResult.get_bool().get(boolResult);
-        if (boolResult) {
-          result.append("true");
-        } else {
-          result.append("false");
+        bool boolResult;
+        error = rawResult.get_bool().get(boolResult);
+        if (!error) {
+          result.append(boolResult ? "true" : "false")
         }
-        return SUCCESS;
+        return error;
       }
       case ondemand::json_type::string: {
         std::string_view stringResult;
@@ -141,20 +139,15 @@ struct SIMDGetJsonObjectFunction {
   // format validation for characters following the current parsing position.
   bool isValidEndingCharacter(const char* currentPos) {
     char endingChar = *currentPos;
-    if (endingChar == ',') {
-      return true;
-    } else if (endingChar == '}') {
-      return true;
-    } else if (endingChar == ']') {
+    if (endingChar == ',' || endingChar == '}' || endingChar == ']') {
       return true;
-    } else if (
-        endingChar == ' ' || endingChar == '\r' || endingChar == '\n' ||
+    }
+    if (endingChar == ' ' || endingChar == '\r' || endingChar == '\n' ||
         endingChar == '\t') {
       // These chars can be prior to a valid ending char.
       return isValidEndingCharacter(currentPos++);
-    } else {
-      return false;
     }
+    return false;
   }
 
   FOLLY_ALWAYS_INLINE bool call(
@@ -164,13 +157,10 @@ struct SIMDGetJsonObjectFunction {
     ParserContext ctx(json.data(), json.size());
     try {
       ctx.parseDocument();
-      simdjson_result<ondemand::value> rawResult;
-      if (formattedJsonPath_.has_value()) {
-        rawResult = ctx.jsonDoc.at_pointer(formattedJsonPath_.value().data());
-      } else {
-        rawResult =
-            ctx.jsonDoc.at_pointer(getFormattedJsonPath(jsonPath).data());
-      }
+      simdjson_result<ondemand::value> rawResult =
+          formattedJsonPath_.has_value()
+          ? ctx.jsonDoc.at_pointer(formattedJsonPath_.value().data())
+          : ctx.jsonDoc.at_pointer(getFormattedJsonPath(jsonPath).data());
       // Field not found.
       if (rawResult.error() == NO_SUCH_FIELD) {
         return false;

diff --git a/velox/functions/sparksql/tests/JsonFunctionsTest.cpp b/velox/functions/sparksql/tests/JsonFunctionsTest.cpp
@@ -25,8 +25,8 @@ namespace {
 class JsonFunctionTest : public SparkFunctionBaseTest {
  protected:
   std::optional<std::string> getJsonObject(
-      std::optional<std::string> json,
-      std::optional<std::string> jsonPath) {
+      const std::optional<std::string>& json,
+      const std::optional<std::string>& jsonPath) {
     return evaluateOnce<std::string>("get_json_object(c0, c1)", json, jsonPath);
   }
 };