Skip to content

ARROW-3982: [C++] Allow "binary" input in simple JSON format #3222

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
pitrou wants to merge 1 commit into from
Closed

ARROW-3982: [C++] Allow "binary" input in simple JSON format #3222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions cpp/src/arrow/ipc/json-simple-test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,7 @@ TEST(TestDouble, Errors) {
}

TEST(TestString, Basics) {
// String type
std::shared_ptr<DataType> type = utf8();
std::shared_ptr<Array> expected, actual;

Expand All @@ -300,6 +301,20 @@ TEST(TestString, Basics) {
s += '\x00';
s += "char";
AssertJSONArray<StringType, std::string>(type, "[\"\", \"some\\u0000char\"]", {"", s});
// UTF8 sequence in string
AssertJSONArray<StringType, std::string>(type, "[\"\xc3\xa9\"]", {"\xc3\xa9"});

// Binary type
type = binary();
AssertJSONArray<BinaryType, std::string>(type, "[\"\", \"foo\", null]",
{true, true, false}, {"", "foo", ""});
// Arbitrary binary (non-UTF8) sequence in string
s = "\xff\x9f";
AssertJSONArray<BinaryType, std::string>(type, "[\"" + s + "\"]", {s});
// Bytes < 0x20 can be represented as JSON unicode escapes
s = '\x00';
s += "\x1f";
AssertJSONArray<BinaryType, std::string>(type, "[\"\\u0000\\u001f\"]", {s});
}

TEST(TestString, Errors) {
Expand All @@ -310,6 +325,31 @@ TEST(TestString, Errors) {
ASSERT_RAISES(Invalid, ArrayFromJSON(type, "[[]]", &array));
}

TEST(TestFixedSizeBinary, Basics) {
std::shared_ptr<DataType> type = fixed_size_binary(3);
std::shared_ptr<Array> expected, actual;

AssertJSONArray<FixedSizeBinaryType, std::string>(type, "[]", {});
AssertJSONArray<FixedSizeBinaryType, std::string>(type, "[\"foo\", \"bar\"]",
{"foo", "bar"});
AssertJSONArray<FixedSizeBinaryType, std::string>(type, "[null, \"foo\"]",
{false, true}, {"", "foo"});
// Arbitrary binary (non-UTF8) sequence in string
std::string s = "\xff\x9f\xcc";
AssertJSONArray<FixedSizeBinaryType, std::string>(type, "[\"" + s + "\"]", {s});
}

TEST(TestFixedSizeBinary, Errors) {
std::shared_ptr<DataType> type = fixed_size_binary(3);
std::shared_ptr<Array> array;

ASSERT_RAISES(Invalid, ArrayFromJSON(type, "[0]", &array));
ASSERT_RAISES(Invalid, ArrayFromJSON(type, "[[]]", &array));
// Invalid length
ASSERT_RAISES(Invalid, ArrayFromJSON(type, "[\"\"]", &array));
ASSERT_RAISES(Invalid, ArrayFromJSON(type, "[\"abcd\"]", &array));
}

TEST(TestDecimal, Basics) {
std::shared_ptr<DataType> type = decimal(10, 4);
std::shared_ptr<Array> expected, actual;
Expand Down
45 changes: 42 additions & 3 deletions cpp/src/arrow/ipc/json-simple.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ using ::arrow::internal::checked_cast;
static constexpr auto kParseFlags = rj::kParseFullPrecisionFlag | rj::kParseNanAndInfFlag;

static Status JSONTypeError(const char* expected_type, rj::Type json_type) {
return Status::Invalid("Expected ", expected_type, " or null, got type ", json_type);
return Status::Invalid("Expected ", expected_type, " or null, got JSON type ",
json_type);
}

class Converter {
Expand Down Expand Up @@ -91,7 +92,6 @@ class ConcreteConverter : public Converter {
};

// TODO : dates and times?
// TODO : binary / fixed size binary?

// ------------------------------------------------------------------------
// Converter for null arrays
Expand Down Expand Up @@ -284,7 +284,7 @@ class DecimalConverter final : public ConcreteConverter<DecimalConverter> {
};

// ------------------------------------------------------------------------
// Converter for string arrays
// Converter for binary and string arrays

class StringConverter final : public ConcreteConverter<StringConverter> {
public:
Expand Down Expand Up @@ -313,6 +313,43 @@ class StringConverter final : public ConcreteConverter<StringConverter> {
std::shared_ptr<BinaryBuilder> builder_;
};

// ------------------------------------------------------------------------
// Converter for fixed-size binary arrays

class FixedSizeBinaryConverter final
: public ConcreteConverter<FixedSizeBinaryConverter> {
public:
explicit FixedSizeBinaryConverter(const std::shared_ptr<DataType>& type) {
this->type_ = type;
builder_ = std::make_shared<FixedSizeBinaryBuilder>(type, default_memory_pool());
}

Status AppendNull() override { return builder_->AppendNull(); }

Status AppendValue(const rj::Value& json_obj) override {

@fsaintjacques fsaintjacques Dec 20, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd refactor this function with a simple switch (if possible?) on Json type and hide the string_view parsing in a static inline function.

switch(json_type) {
  case Json::NULL: return AppendNull();
  case Json::STRING: {
    util::string_view view;
    ARROW_RETURN_NOT_OK(JsonStringToViewOfFixedSize(json_obj, fixed_size, &view));
    return builder_->Append(view);
  }
  default: return Status::Invalid(...);
}

This is a style preference, but I think it improves reading since there's less control flow involved.

@pitrou pitrou Dec 20, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current idiom (a chain of ifs) is used elsewhere in the module, though. So we'd have to change other occurrences as well. IMHO there's not much benefit.

@wesm wesm Dec 20, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's OK to leave the code as is since it's similar to the rest of the file. If future refactoring is needed then whoever is working on it is free to do this code scrubbing

if (json_obj.IsNull()) {
return AppendNull();
}
if (json_obj.IsString()) {
auto view = util::string_view(json_obj.GetString(), json_obj.GetStringLength());
if (view.length() != static_cast<size_t>(builder_->byte_width())) {
std::stringstream ss;

@pitrou pitrou Dec 20, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be simplified now :-)

ss << "Invalid string length " << view.length() << " in JSON input for "
<< this->type_->ToString();
return Status::Invalid(ss.str());
}
return builder_->Append(view);
} else {
return JSONTypeError("string", json_obj.GetType());
}
}

std::shared_ptr<ArrayBuilder> builder() override { return builder_; }

protected:
std::shared_ptr<FixedSizeBinaryBuilder> builder_;
};

// ------------------------------------------------------------------------
// Converter for list arrays

Expand Down Expand Up @@ -449,6 +486,8 @@ Status GetConverter(const std::shared_ptr<DataType>& type,
SIMPLE_CONVERTER_CASE(Type::LIST, ListConverter)
SIMPLE_CONVERTER_CASE(Type::STRUCT, StructConverter)
SIMPLE_CONVERTER_CASE(Type::STRING, StringConverter)
SIMPLE_CONVERTER_CASE(Type::BINARY, StringConverter)
SIMPLE_CONVERTER_CASE(Type::FIXED_SIZE_BINARY, FixedSizeBinaryConverter)
SIMPLE_CONVERTER_CASE(Type::DECIMAL, DecimalConverter)
default: {
return Status::NotImplemented("JSON conversion to ", type->ToString(),
Expand Down
11 changes: 2 additions & 9 deletions cpp/src/arrow/pretty_print-test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,18 +277,11 @@ TEST_F(TestPrettyPrint, ListType) {

TEST_F(TestPrettyPrint, FixedSizeBinaryType) {
std::vector<bool> is_valid = {true, true, false, true, false};
std::vector<std::string> values = {"foo", "bar", "baz"};

std::shared_ptr<Array> array;
auto type = fixed_size_binary(3);
FixedSizeBinaryBuilder builder(type);

ASSERT_OK(builder.Append(values[0]));
ASSERT_OK(builder.Append(values[1]));
ASSERT_OK(builder.Append(values[2]));
ASSERT_OK(builder.Finish(&array));
auto array = ArrayFromJSON(type, "[\"foo\", \"bar\", null, \"baz\"]");

static const char* ex = "[\n 666F6F,\n 626172,\n 62617A\n]";
static const char* ex = "[\n 666F6F,\n 626172,\n null,\n 62617A\n]";
CheckArray(*array, {0, 10}, ex);
static const char* ex_2 = " [\n 666F6F,\n ...\n 62617A\n ]";
CheckArray(*array, {2, 1}, ex_2);
Expand Down