ARROW-694: [C++] Initial parser interface for reading JSON into RecordBatches #3592

bkietz · 2019-02-08T15:06:42Z

( abandoning #3206 )

Adds json sub project with:

BlockParser which parses Buffers of json formatted data into a StructArray with minimal conversion
- true/false, and null fields are stored in BooleanArray and NullArray respectively
- strings are stored as indices into a single StringArray
- numbers are not converted; their string representations are stored alongside string values
- nested fields are stored as ListArray or StructArray of their parsed (unconverted) children
Three approaches to handling unexpected fields:
1. Error on an unexpected field
2. Ignore unexpected fields
3. Infer the type of unexpected fields and add them to the schema
Convenience interface for parsing a single chunk of json data into a RecordBatch with fully converted columns
Chunker to process a stream of unchunked data for use by BlockParser (not currently used)

wesm · 2019-02-08T15:15:31Z

Can you update the title and write a PR description of what is in the patch?

wesm · 2019-02-08T17:25:18Z

I removed the WIP from the title. I will review, then let's get the tests passing and merge this in the near future

xhochy

Left some initial comments.

The code here makes a lot use of std::move in places where we have used const & instead. We should keep using const references. I don't see whether moving really has performance impacts, I rather feel that is the other way around.

cpp/src/arrow/json/chunker.cc

xhochy · 2019-02-12T10:23:08Z

cpp/src/arrow/json/chunker.cc

+
+class ReadOnlyStream {
+ public:
+  using Ch = char;


I don't really like this 4->2 letter abbreviation. It worsens readability enormously for me.

This is a convention (a requirement maybe?) of rapidjson streams https://github.com/Tencent/rapidjson/blob/master/include/rapidjson/stream.h#L27

xhochy · 2019-02-12T10:23:32Z

cpp/src/arrow/json/chunker.cc

+
+class StringStream : public ReadOnlyStream {
+ public:
+  using Ch = char;


Please remove this abbreviation

That typedef is part of RapidJson's stream interface, but I can refactor the rest of the class to use char instead

cpp/src/arrow/type.h

cpp/src/arrow/json/parser.h

cpp/src/arrow/json/parser.cc

xhochy · 2019-02-15T14:55:30Z

Looking good so far. Please ping again before you merge.

wesm · 2019-02-15T15:54:06Z

I will give this a look through today and then merge with no build issues. We can address further things in follow up patches

wesm

+1. I'm going to go ahead and merge this with a passing build. I left comments -- a lot of the polishing and improvements can happen up in follow up patches. I think we should soon implement Python bindings and begin to drive feature and performance work based on feature requirements.

At the face of it, the parsing perf of ~77 MB/s seems slower than I would have expected but I didn't do any perf tests of other libraries to get a baseline. I made a quick flamegraph and it seems about 20% of runtime is spent in SetFieldBuilder. We may need to make some optimizations for the common case where fields appear in the same order in each record.

cpp/src/arrow/array/builder_binary.h

wesm · 2019-02-15T21:15:59Z

cpp/src/arrow/json/chunker.cc

+
+class ReadOnlyStream {
+ public:
+  using Ch = char;


This is a convention (a requirement maybe?) of rapidjson streams https://github.com/Tencent/rapidjson/blob/master/include/rapidjson/stream.h#L27

cpp/src/arrow/json/chunker.cc

wesm · 2019-02-15T21:28:06Z

cpp/src/arrow/json/chunker.h

+/// \brief A reusable block-based chunker for JSON data
+///
+/// The chunker takes a block of JSON data and finds a suitable place
+/// to cut it up without splitting an object.


Since there are no benchmarks yet for the Chunker code, whoever works on this next needs to write some before doing any more work on the API or implementation

cpp/src/arrow/json/options.h

cpp/src/arrow/json/parser.cc

Instead of a subclass of ArrayBuilder, type inferrence now uses the new interface AdaptiveArrayBuilder. This class is designed to accomodate changing element type while building.

Also minor refactorings in parser, renamed to new un-prefixed convention

BlockParser now stores the both strings and unconverted numbers as a `dictionary<indices::Int32, dictionary::String>` for later parsing.

…uctibility

(it is a stand in for the eventual TableReader impl) Also: do not rely on pointer identity of tags (`std::shared_ptr<const KeyValueMetadata>`)

Useful: ``` clang-query-7 -p=compile_commands.json \ cpp/src/arrow/json/* \ -c='match ifStmt(hasThen(stmt(unless(compoundStmt()))))' ```

Change-Id: I8e632a786ba6cc26afa3fb14e8769013ffe9a28c

Change-Id: Ibe9148f4210c9a1f794d7c928d7e70c5ddcaae56

Change-Id: I96d6a741ecef9dbfced5ef7ee4c4fcdc697ad64e

wesm · 2019-02-16T03:11:55Z

The chunker test was failing valgrind, so I disabled it

cpp/src/arrow/json/chunker.cc

wesm · 2019-02-20T14:48:17Z

This looks like a race condition with the metadata_fbs target, not related to this patch

[33/566] Building CXX object src/arrow/flight/CMakeFiles/arrow_flight_objlib.dir/client.cc.o
FAILED: src/arrow/flight/CMakeFiles/arrow_flight_objlib.dir/client.cc.o 
/Users/travis/build/apache/arrow/cpp-toolchain/bin/ccache /Users/travis/build/apache/arrow/cpp-toolchain/bin/clang++  -DARROW_EXTRA_ERROR_CONTEXT -DARROW_JEMALLOC -DARROW_JEMALLOC_INCLUDE_DIR=/Users/travis/build/apache/arrow/cpp-build/jemalloc_ep-prefix/src/jemalloc_ep/dist//include -DARROW_NO_DEPRECATED_API -DARROW_USE_GLOG -DARROW_USE_SIMD -DARROW_WITH_BROTLI -DARROW_WITH_BZ2 -DARROW_WITH_LZ4 -DARROW_WITH_SNAPPY -DARROW_WITH_ZLIB -DARROW_WITH_ZSTD -Isrc -I/Users/travis/build/apache/arrow/cpp/src -isystem boost_ep-prefix/src/boost_ep -isystem /Users/travis/build/apache/arrow/cpp-toolchain/include -isystem gbenchmark_ep/src/gbenchmark_ep-install/include -isystem jemalloc_ep-prefix/src -isystem /Users/travis/build/apache/arrow/cpp/thirdparty/hadoop/include -isystem orc_ep-install/include -isystem /Users/travis/build/apache/arrow/cpp-toolchain/include/thrift -Qunused-arguments -ggdb -O0  -Weverything -Wno-c++98-compat -Wno-c++98-compat-pedantic -Wno-deprecated -Wno-weak-vtables -Wno-padded -Wno-comma -Wno-unused-macros -Wno-unused-parameter -Wno-unused-template -Wno-undef -Wno-shadow -Wno-switch-enum -Wno-exit-time-destructors -Wno-global-constructors -Wno-weak-template-vtables -Wno-undefined-reinterpret-cast -Wno-implicit-fallthrough -Wno-unreachable-code-return -Wno-float-equal -Wno-missing-prototypes -Wno-documentation-unknown-command -Wno-old-style-cast -Wno-covered-switch-default -Wno-cast-align -Wno-vla-extension -Wno-shift-sign-overflow -Wno-used-but-marked-unused -Wno-missing-variable-declarations -Wno-gnu-zero-variadic-macro-arguments -Wconversion -Wno-sign-conversion -Wno-disabled-macro-expansion -Wno-format-nonliteral -Wno-missing-noreturn -Wno-gnu-folding-constant -Wno-reserved-id-macro -Wno-range-loop-analysis -Wno-double-promotion -Wno-undefined-func-template -Wno-zero-as-null-pointer-constant -Wno-unknown-warning-option -Werror -Wno-unknown-warning-option -msse4.2 -maltivec -stdlib=libc++  -g -fPIC   -std=gnu++11 -MD -MT src/arrow/flight/CMakeFiles/arrow_flight_objlib.dir/client.cc.o -MF src/arrow/flight/CMakeFiles/arrow_flight_objlib.dir/client.cc.o.d -o src/arrow/flight/CMakeFiles/arrow_flight_objlib.dir/client.cc.o -c /Users/travis/build/apache/arrow/cpp/src/arrow/flight/client.cc
In file included from /Users/travis/build/apache/arrow/cpp/src/arrow/flight/client.cc:29:
/Users/travis/build/apache/arrow/cpp/src/arrow/ipc/metadata-internal.h:32:10: fatal error: 'arrow/ipc/Schema_generated.h' file not found
#include "arrow/ipc/Schema_generated.h"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
[36/566] Performing build step for 'boost_ep'
-- boost_ep build command succeeded.  See also /Users/travis/build/apache/arrow/cpp-build/boost_ep-prefix/src/boost_ep-stamp/boost_ep-build-*.log
ninja: build stopped: subcommand failed.

…ndition with Flatbuffers Change-Id: Ief2c1de54e39731fb17ffe6b5c306152b0c76169

xhochy · 2019-02-20T14:52:03Z

This looks like a race condition with the metadata_fbs target, not related to this patch

[33/566] Building CXX object src/arrow/flight/CMakeFiles/arrow_flight_objlib.dir/client.cc.o
FAILED: src/arrow/flight/CMakeFiles/arrow_flight_objlib.dir/client.cc.o 
/Users/travis/build/apache/arrow/cpp-toolchain/bin/ccache /Users/travis/build/apache/arrow/cpp-toolchain/bin/clang++  -DARROW_EXTRA_ERROR_CONTEXT -DARROW_JEMALLOC -DARROW_JEMALLOC_INCLUDE_DIR=/Users/travis/build/apache/arrow/cpp-build/jemalloc_ep-prefix/src/jemalloc_ep/dist//include -DARROW_NO_DEPRECATED_API -DARROW_USE_GLOG -DARROW_USE_SIMD -DARROW_WITH_BROTLI -DARROW_WITH_BZ2 -DARROW_WITH_LZ4 -DARROW_WITH_SNAPPY -DARROW_WITH_ZLIB -DARROW_WITH_ZSTD -Isrc -I/Users/travis/build/apache/arrow/cpp/src -isystem boost_ep-prefix/src/boost_ep -isystem /Users/travis/build/apache/arrow/cpp-toolchain/include -isystem gbenchmark_ep/src/gbenchmark_ep-install/include -isystem jemalloc_ep-prefix/src -isystem /Users/travis/build/apache/arrow/cpp/thirdparty/hadoop/include -isystem orc_ep-install/include -isystem /Users/travis/build/apache/arrow/cpp-toolchain/include/thrift -Qunused-arguments -ggdb -O0  -Weverything -Wno-c++98-compat -Wno-c++98-compat-pedantic -Wno-deprecated -Wno-weak-vtables -Wno-padded -Wno-comma -Wno-unused-macros -Wno-unused-parameter -Wno-unused-template -Wno-undef -Wno-shadow -Wno-switch-enum -Wno-exit-time-destructors -Wno-global-constructors -Wno-weak-template-vtables -Wno-undefined-reinterpret-cast -Wno-implicit-fallthrough -Wno-unreachable-code-return -Wno-float-equal -Wno-missing-prototypes -Wno-documentation-unknown-command -Wno-old-style-cast -Wno-covered-switch-default -Wno-cast-align -Wno-vla-extension -Wno-shift-sign-overflow -Wno-used-but-marked-unused -Wno-missing-variable-declarations -Wno-gnu-zero-variadic-macro-arguments -Wconversion -Wno-sign-conversion -Wno-disabled-macro-expansion -Wno-format-nonliteral -Wno-missing-noreturn -Wno-gnu-folding-constant -Wno-reserved-id-macro -Wno-range-loop-analysis -Wno-double-promotion -Wno-undefined-func-template -Wno-zero-as-null-pointer-constant -Wno-unknown-warning-option -Werror -Wno-unknown-warning-option -msse4.2 -maltivec -stdlib=libc++  -g -fPIC   -std=gnu++11 -MD -MT src/arrow/flight/CMakeFiles/arrow_flight_objlib.dir/client.cc.o -MF src/arrow/flight/CMakeFiles/arrow_flight_objlib.dir/client.cc.o.d -o src/arrow/flight/CMakeFiles/arrow_flight_objlib.dir/client.cc.o -c /Users/travis/build/apache/arrow/cpp/src/arrow/flight/client.cc
In file included from /Users/travis/build/apache/arrow/cpp/src/arrow/flight/client.cc:29:
/Users/travis/build/apache/arrow/cpp/src/arrow/ipc/metadata-internal.h:32:10: fatal error: 'arrow/ipc/Schema_generated.h' file not found
#include "arrow/ipc/Schema_generated.h"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
[36/566] Performing build step for 'boost_ep'
-- boost_ep build command succeeded.  See also /Users/travis/build/apache/arrow/cpp-build/boost_ep-prefix/src/boost_ep-stamp/boost_ep-build-*.log
ninja: build stopped: subcommand failed.

Rebasing should help, I've fixed this race recently.

wesm

+1, thanks @bkietz! This puts us on a good path to getting some Arrow-powered JSON into the hands of users

wesm mentioned this pull request Feb 8, 2019

ARROW-694: [C++] json reader, WIP #3206

Closed

bkietz changed the title ~~ARROW-694: [C++] json reader [WIP]~~ ARROW-694: [C++] initial parser interface for reading JSON into RecordBatches [WIP] Feb 8, 2019

wesm changed the title ~~ARROW-694: [C++] initial parser interface for reading JSON into RecordBatches [WIP]~~ ARROW-694: [C++] Initial parser interface for reading JSON into RecordBatches [WIP] Feb 8, 2019

wesm changed the title ~~ARROW-694: [C++] Initial parser interface for reading JSON into RecordBatches [WIP]~~ ARROW-694: [C++] Initial parser interface for reading JSON into RecordBatches Feb 8, 2019

bkietz force-pushed the ARROW-694-json-reader-WIP branch from e7804f6 to 6126a92 Compare February 8, 2019 20:34

xhochy force-pushed the master branch from 29f76df to 2b9155a Compare February 8, 2019 21:00

bkietz force-pushed the ARROW-694-json-reader-WIP branch from 6126a92 to 9f4065a Compare February 8, 2019 21:27

xhochy reviewed Feb 12, 2019

View reviewed changes

bkietz force-pushed the ARROW-694-json-reader-WIP branch from 9e158df to ca59903 Compare February 13, 2019 17:51

wesm approved these changes Feb 15, 2019

View reviewed changes

bkietz added 16 commits February 15, 2019 17:34

first sketch of JSON parser

17176f9

refactoring Schema::GetFieldIndex to return int

cc3698a

adding first draft of type inferrence to BlockParser

cb6a313

Refactored type inferrence

6472738

Instead of a subclass of ArrayBuilder, type inferrence now uses the new interface AdaptiveArrayBuilder. This class is designed to accomodate changing element type while building.

Added trivial parser benchmark and data generator

0f3a3bc

Also minor refactorings in parser, renamed to new un-prefixed convention

add (failing) test for '-0' consistency

e39a4a9

use recommended loop style

2677a57

adding Chunker implementation and tests

b9d5c3d

Rewrite parser to defer conversion of strings and numbers

69d7c5c

BlockParser now stores the both strings and unconverted numbers as a `dictionary<indices::Int32, dictionary::String>` for later parsing.

adding first draft of parsing with type inference

330615b

adding ParseOne interface for dead simple parsing

92ffc64

add missing virtual destructor

10e4f0d

remove bitfields

aaaf9e7

fix format issue, use SFINAE to detect StringConverter default constr…

9679b6f

…uctibility

remove misplaced const

e334648

add Status return to Generate

f92f850

bkietz and others added 11 commits February 15, 2019 17:34

add inline tag

292672a

check status for generating list elements

c1a7f4b

disabling chunker test

0d6d920

clean up Chunker's stream usage

1bebda8

Move ParseOne to reader.cc

6e86078

(it is a stand in for the eventual TableReader impl) Also: do not rely on pointer identity of tags (`std::shared_ptr<const KeyValueMetadata>`)

use const shared_ptr<T>& instead of move

baac467

Use compound statements in if()

be720c8

Useful: ``` clang-query-7 -p=compile_commands.json \ cpp/src/arrow/json/* \ -c='match ifStmt(hasThen(stmt(unless(compoundStmt()))))' ```

correct test util includes

69541ea

add json project back (merge error)

05fd9d1

Restore BinaryBuilder::UnsafeAppend for const std::string&

83150ec

Change-Id: I8e632a786ba6cc26afa3fb14e8769013ffe9a28c

cmake-format

76d0431

Change-Id: Ibe9148f4210c9a1f794d7c928d7e70c5ddcaae56

wesm force-pushed the ARROW-694-json-reader-WIP branch from d2042d2 to 76d0431 Compare February 15, 2019 23:35

Disable arrow-json-chunker-test

29c2312

Change-Id: I96d6a741ecef9dbfced5ef7ee4c4fcdc697ad64e

wesm force-pushed the master branch from 3088183 to 0c6b2d2 Compare February 18, 2019 19:34

fix doc error in chunker.h

d7d0a2e

pitrou reviewed Feb 19, 2019

View reviewed changes

cpp/src/arrow/json/chunker.cc Show resolved Hide resolved

bkietz added 3 commits February 19, 2019 11:21

Add ARROW_PREDICT_* to conditions in parser.cc

0d0caa9

adding explanatory comments to json/parser.cc

554b595

adding more comments to parser.cc

d67aff6

Add arrow_dependencies to arrow_flight.so dependencies to fix race co…

e42e5d7

…ndition with Flatbuffers Change-Id: Ief2c1de54e39731fb17ffe6b5c306152b0c76169

wesm approved these changes Feb 20, 2019

View reviewed changes

wesm closed this in 9c19bb6 Feb 20, 2019

bkietz deleted the ARROW-694-json-reader-WIP branch February 22, 2019 16:15

pcmoritz mentioned this pull request Mar 27, 2019

ARROW-5027: [Python] Python bindings for JSON reader #4044

Closed

asfimport mentioned this pull request Feb 20, 2019

[C++] Build JSON "scanner" for reading record batches from line-delimited JSON files #16314

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-694: [C++] Initial parser interface for reading JSON into RecordBatches #3592

ARROW-694: [C++] Initial parser interface for reading JSON into RecordBatches #3592

bkietz commented Feb 8, 2019 •

edited

Loading

wesm commented Feb 8, 2019

wesm commented Feb 8, 2019

xhochy left a comment

xhochy Feb 12, 2019

wesm Feb 15, 2019

xhochy Feb 12, 2019

bkietz Feb 12, 2019

xhochy commented Feb 15, 2019

wesm commented Feb 15, 2019

wesm left a comment

wesm Feb 15, 2019

wesm Feb 15, 2019

wesm commented Feb 16, 2019

wesm commented Feb 20, 2019

xhochy commented Feb 20, 2019

wesm left a comment

ARROW-694: [C++] Initial parser interface for reading JSON into RecordBatches #3592

ARROW-694: [C++] Initial parser interface for reading JSON into RecordBatches #3592

Conversation

bkietz commented Feb 8, 2019 • edited Loading

wesm commented Feb 8, 2019

wesm commented Feb 8, 2019

xhochy left a comment

Choose a reason for hiding this comment

xhochy Feb 12, 2019

Choose a reason for hiding this comment

wesm Feb 15, 2019

Choose a reason for hiding this comment

xhochy Feb 12, 2019

Choose a reason for hiding this comment

bkietz Feb 12, 2019

Choose a reason for hiding this comment

xhochy commented Feb 15, 2019

wesm commented Feb 15, 2019

wesm left a comment

Choose a reason for hiding this comment

wesm Feb 15, 2019

Choose a reason for hiding this comment

wesm Feb 15, 2019

Choose a reason for hiding this comment

wesm commented Feb 16, 2019

wesm commented Feb 20, 2019

xhochy commented Feb 20, 2019

wesm left a comment

Choose a reason for hiding this comment

bkietz commented Feb 8, 2019 •

edited

Loading