Skip to content

Commit

Permalink
Back out "Add json_format Presto function" (#3510)
Browse files Browse the repository at this point in the history
Summary:
The implementation of json_format() uses the setNoCopy() API to avoid copying strings from the input to the output vector. But it forgets to copy the string buffer to the output vector as well. This causes some memory bugs that fails Velox fuzzer test in CircleCI. We back out this change now and will add it back after we fix it.

Pull Request resolved: #3510

Original commit changeset: f2cc011a4b03

Original Phabricator Diff: D41643635 (e2802d7)

Reviewed By: mbasmanova

Differential Revision: D42020592

fbshipit-source-id: a159ac80f661609abd052463ff504967322353b4
  • Loading branch information
kagamiori authored and facebook-github-bot committed Dec 14, 2022
1 parent 38505ff commit f020d3e
Show file tree
Hide file tree
Showing 4 changed files with 0 additions and 51 deletions.
7 changes: 0 additions & 7 deletions velox/docs/functions/json.rst
Original file line number Diff line number Diff line change
Expand Up @@ -133,13 +133,6 @@ JSON Functions
SELECT json_size('{"x": [1, 2, 3]}', '$.x'); -- 3
SELECT json_size('{"x": {"a": 1, "b": 2}}', '$.x.a'); -- 0

.. function:: json_format(json) -> varchar

Serializes the input JSON value to JSON text conforming to RFC 7159.
The JSON value can be a JSON object, a JSON array, a JSON string, a JSON number, true, false or null

SELECT json_format(JSON '{"a": 1, "b": 2}')

============
JSON Vectors
============
Expand Down
12 changes: 0 additions & 12 deletions velox/functions/prestosql/JsonFunctions.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,6 @@

namespace facebook::velox::functions {

template <typename T>
struct JsonFormatFunction {
VELOX_DEFINE_FUNCTION_TYPES(T);

FOLLY_ALWAYS_INLINE void call(
out_type<Varchar>& jsonString,
const arg_type<Json>& json) {
folly::parseJson(json.getString());
jsonString.setNoCopy(json);
}
};

template <typename T>
struct IsJsonScalarFunction {
VELOX_DEFINE_FUNCTION_TYPES(T);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ void registerJsonFunctions() {
registerFunction<JsonArrayContainsFunction, bool, Json, Varchar>(
{"json_array_contains"});
registerFunction<JsonSizeFunction, int64_t, Json, Varchar>({"json_size"});
registerFunction<JsonFormatFunction, Varchar, Json>({"json_format"});
}

} // namespace facebook::velox::functions
31 changes: 0 additions & 31 deletions velox/functions/prestosql/tests/JsonFunctionsTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,37 +58,6 @@ class JsonFunctionsTest : public functions::test::FunctionBaseTest {
}
};

TEST_F(JsonFunctionsTest, JsonFormat) {
const auto json_format = [&](std::optional<std::string> value) {
return evaluateOnce<std::string, std::string>(
"json_format(c0)", {value}, {JSON()});
};

EXPECT_EQ(json_format(R"(true)"), "true");
EXPECT_EQ(json_format(R"(null)"), "null");
EXPECT_EQ(json_format(R"(42)"), "42");
EXPECT_EQ(json_format(R"("abc")"), "\"abc\"");
EXPECT_EQ(json_format(R"([1, 2, 3])"), "[1, 2, 3]");
EXPECT_EQ(json_format(R"({"k1":"v1"})"), "{\"k1\":\"v1\"}");

// check keys and values are there
const std::string jsonStr = json_format(R"({"k1":"v1","k2":"v2"})").value();
folly::dynamic object = folly::parseJson(jsonStr);

std::set<std::string> keys{"k1", "k2"};

for (const auto& key : object.keys()) {
EXPECT_TRUE(keys.find(key.getString()) != keys.end());
}

std::unordered_map<std::string, std::string> jsonMap{
{"k1", "v1"}, {"k2", "v2"}};

for (const auto& key : jsonMap) {
EXPECT_EQ(object.at(key.first), jsonMap.at(key.first));
}
}

TEST_F(JsonFunctionsTest, isJsonScalarSignatures) {
auto signatures = getSignatureStrings("is_json_scalar");
ASSERT_EQ(1, signatures.size());
Expand Down

0 comments on commit f020d3e

Please sign in to comment.