Migrate Arrow-related code to incorporate IPC code improvements since 0.4.x, simplifications and perf improvements #65

wesm · 2017-09-03T00:29:08Z

Use up-to-date Arrow IPC serialization / read APIs
Use vector appends with Arrow builders (cf ARROW-1468)
Avoid extra full result set memory copy in multithreaded fetches
Use library calls for pretty printing (cf ARROW-1396)

CLAassistant · 2017-09-03T00:29:14Z

All committers have signed the CLA.

asuhan · 2017-09-03T00:34:06Z

Thanks, the API simplifications look very nice. Regarding exceptions, it all depends on their nature. If it's running out of memory for example, you can throw OutOfMemory defined in DataMgr/BufferMgr/BufferMgr.h.

wesm · 2017-09-03T13:45:40Z

OK, I'll add a function to create different exception types based on the type of error status

wesm

About all for tonight -- I need to do some do some debugging to get the test suite passing again. The next step beyond this is to make a bridge between MapD's CudaMgr and Arrow's GPU memory management so that the IPC code can write into device memory owned by MapD in a clean way: https://issues.apache.org/jira/browse/ARROW-1423.

Once I finish all this some additional testing from folks would be helpful to ensure I haven't broken anything, or instructions how I can test

As a last matter (probably in a follow up patch), I'm going to add some simple APIs for dealing with SysV and POSIX shared memory segments: https://issues.apache.org/jira/browse/ARROW-1385

wesm · 2017-09-06T02:23:43Z

QueryEngine/ArrowResultSet.cpp

@@ -68,36 +69,36 @@ std::vector<TargetValue> ArrowResultSet::getNextRow(const bool translate_strings
    const auto& column_typeinfo = getColType(i);
    switch (column_typeinfo.get_type()) {
      case kSMALLINT: {
-        CHECK(dynamic_cast<const arrow::NumericArray<arrow::Int16Type>*>(&column));
-        const auto& i16_column = static_cast<const arrow::NumericArray<arrow::Int16Type>&>(column);


I do not recommend using arrow::NumericArray -- use the typedefs

Nice, I didn't notice them when I wrote this.

wesm · 2017-09-06T02:27:50Z

QueryEngine/ResultSetConversion.cpp

+      CHECK(is_valid.get());
+      ARROW_THROW_NOT_OK(typed_builder->Append(vals.data(), length, *is_valid));
+    } else {
+      ARROW_THROW_NOT_OK(typed_builder->Append(vals.data(), length));
    }


Vector appends

asuhan · 2017-09-06T02:38:26Z

@wesm Thanks, I'll have a look tomorrow. For testing, have a look at https://github.com/mapd/mapd-core/blob/master/run_sanity_tests. You can run it as is, but you only need to run ExecuteTest (keep the first 9 lines unchanged).

wesm · 2017-09-06T02:40:22Z

Yep, I found that. I'll step around gdb and fix the bugs I introduced, probably tomorrow later in the day

asuhan · 2017-09-06T02:43:35Z

QueryEngine/ArrowUtil.h

+#define ARROW_THROW_NOT_OK(s)                   \
+  do {                                          \
+    ::arrow::Status _s = (s);                   \
+    if (ARROW_PREDICT_FALSE(!_s.ok())) {        \


Alternatively, you can use UNLIKELY from Shared/likely.h. I guess everyone has a version of this macro somewhere in the codebase.

asuhan · 2017-09-07T00:00:13Z

Do the tests pass now?

wesm · 2017-09-07T00:10:43Z

Not yet, I am going to look at them hopefully before I go to bed tonight

wesm · 2017-09-07T00:12:19Z

I think it would be best to defer improvements re: direct writes to GPU device memory (i.e. https://github.com/apache/arrow/blob/master/cpp/src/arrow/gpu/cuda_arrow_ipc.h#L50) to a subsequent patch.

I also will need to update the toolchain files with an updated Arrow version and build recipe. 0.7.0 will probably be released next week roughly so you shouldn't have to be on an unreleased version for very long

wesm · 2017-09-07T03:13:20Z

I'm out of gas for tonight. I have a few free hours tomorrow morning (Eastern Time) so I'll look at this first thing and and get the issues worked out

wesm · 2017-09-07T15:02:05Z

@asuhan I'm finding when I build with gcc 4.9 that I get errors like

3: Test command: /home/wesm/code/mapd-core/build/Tests/ExecuteTest "--gtest_output=xml:../"
3: Test timeout computed to be: 9.99988e+06
3: F0907 11:00:11.645380 18703 QueryRunner.cpp:36] Check failed: boost::filesystem::exists(base_path) 
3: *** Check failure stack trace: ***
3:     @     0x7f45a03e2d5e  google::LogMessage::Fail()
3:     @     0x7f45a03e2caa  google::LogMessage::SendToLog()
3:     @     0x7f45a03e2690  google::LogMessage::Flush()
3:     @     0x7f45a03e5597  google::LogMessageFatal::~LogMessageFatal()
3:     @          0x120f8b6  get_session()
3:     @          0x11dadb3  main
3:     @     0x7f459c89ff45  __libc_start_main
3:     @          0x1174d9c  (unknown)
3:     @              (nil)  (unknown)

When I build with clang-3.8 instead, the tests work, but gdb doesn't work very well -- I'm able to get break points to work but inspecting variables seems not to work. I don't have any issues with gdb elsewhere, so it seems this is something MapD-specific. I can debug with print statements but gdb would really be better

wesm · 2017-09-07T15:06:38Z

I created a tmp symlink to the data directory in Tests and seem to be in business. I must have followed the directions wrong someplace but let me know for the future

wesm · 2017-09-07T22:59:47Z

Looking at this again later this evening

asuhan · 2017-09-07T23:01:30Z

OK, I'll have a look myself tomorrow if you don't get to the bottom of it. FYI, I've seen a similar type of crash during my initial development of the test and it was due to buffer ownership problems. Holding the PoolBuffers fixed it.

wesm · 2017-09-07T23:04:12Z

Hm, interesting. Perhaps a pointer is being leaked somewhere. I will let you know if I can't sort it out

As an aside, it would also be better to use std::shared_ptr<RecordBatch>. That type is only copyable by accident. Just opened https://issues.apache.org/jira/browse/ARROW-1486

wesm · 2017-09-08T03:53:25Z

OK, tests are passing again. I summarized the work in the commit message.

Some follow up JIRAs

When you use BufferReader with the IPC read functions, so do not need to manually retain a reference to the original buffer, because internally the buffers in the record batch have a reference to the parent. See https://github.com/apache/arrow/blob/master/cpp/src/arrow/io/memory.cc#L227. If you had to keep track of which arrow::Buffer owns the memory all the time it would be complete madness. If you ever run into any problems with chained memory references, please bug me as I've tried to be really careful about simple zero-copy and RAII-based memory management but no one is perfect =) Better to fix the underlying issues than work around them.

…to 0.7.x * Avoid extra full result set memory copy in multithreaded fetches * Use vector appends with Arrow builders (cf ARROW-1468) * Use up-to-date Arrow IPC serialization / read APIs * Fix memory ownership issues with RecordBatch::column (returns a new object now) * Use library calls for pretty printing (cf ARROW-1396)

wesm · 2017-09-08T03:55:57Z

I have not made use of any of the direct-to-CUDA serialization code in Arrow yet. I think it would be better to scrutinize that patch more carefully in isolation

asuhan · 2017-09-08T18:32:08Z

I agree we should leave the CUDA parts for later. We should also remove the dependency on the Arrow GPU, scripts/common-functions.sh doesn't build with GPU support, which breaks the CUDA build.

The patch looks good otherwise, I'll pick it with the dependency on Arrow GPU removed for now. Thanks!

wesm · 2017-09-08T19:24:33Z

Oops, sorry about that (didn't test the toolchain from scratch). Thanks!

…-dbe-install remove editable dbe install by default

wesm force-pushed the arrow-0.7-arrow_gpu branch from 3f8b5cd to a850a24 Compare September 5, 2017 23:49

wesm commented Sep 6, 2017

View reviewed changes

asuhan reviewed Sep 6, 2017

View reviewed changes

wesm force-pushed the arrow-0.7-arrow_gpu branch from 5881226 to 3f615c0 Compare September 8, 2017 03:50

wesm force-pushed the arrow-0.7-arrow_gpu branch from 3f615c0 to b6c7fce Compare September 8, 2017 03:54

wesm changed the title ~~WIP: Migrate Arrow-related code to incorporate IPC code improvements since 0.4.x~~ Migrate Arrow-related code to incorporate IPC code improvements since 0.4.x, simplifications and perf improvements Sep 8, 2017

wesm mentioned this pull request Sep 8, 2017

ARROW-1479: [JS] Expand JavaScript implementation apache/arrow#1062

Closed

asuhan merged commit 046d876 into heavyai:master Sep 8, 2017

wesm deleted the arrow-0.7-arrow_gpu branch September 8, 2017 19:24

anton-malakhov pushed a commit to anton-malakhov/omniscidb that referenced this pull request Aug 10, 2020

Merge pull request heavyai#65 from intel-go/ienkovich/remove-editable…

575128d

…-dbe-install remove editable dbe install by default

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Arrow-related code to incorporate IPC code improvements since 0.4.x, simplifications and perf improvements #65

Migrate Arrow-related code to incorporate IPC code improvements since 0.4.x, simplifications and perf improvements #65

wesm commented Sep 3, 2017 •

edited

CLAassistant commented Sep 3, 2017 •

edited

asuhan commented Sep 3, 2017

wesm commented Sep 3, 2017

wesm left a comment

wesm Sep 6, 2017

asuhan Sep 6, 2017

wesm Sep 6, 2017

asuhan commented Sep 6, 2017

wesm commented Sep 6, 2017

asuhan Sep 6, 2017

asuhan commented Sep 7, 2017

wesm commented Sep 7, 2017

wesm commented Sep 7, 2017

wesm commented Sep 7, 2017

wesm commented Sep 7, 2017

wesm commented Sep 7, 2017

wesm commented Sep 7, 2017

asuhan commented Sep 7, 2017

wesm commented Sep 7, 2017

wesm commented Sep 8, 2017 •

edited

wesm commented Sep 8, 2017

asuhan commented Sep 8, 2017

wesm commented Sep 8, 2017

Migrate Arrow-related code to incorporate IPC code improvements since 0.4.x, simplifications and perf improvements #65

Migrate Arrow-related code to incorporate IPC code improvements since 0.4.x, simplifications and perf improvements #65

Conversation

wesm commented Sep 3, 2017 • edited

CLAassistant commented Sep 3, 2017 • edited

asuhan commented Sep 3, 2017

wesm commented Sep 3, 2017

wesm left a comment

Choose a reason for hiding this comment

wesm Sep 6, 2017

Choose a reason for hiding this comment

asuhan Sep 6, 2017

Choose a reason for hiding this comment

wesm Sep 6, 2017

Choose a reason for hiding this comment

asuhan commented Sep 6, 2017

wesm commented Sep 6, 2017

asuhan Sep 6, 2017

Choose a reason for hiding this comment

asuhan commented Sep 7, 2017

wesm commented Sep 7, 2017

wesm commented Sep 7, 2017

wesm commented Sep 7, 2017

wesm commented Sep 7, 2017

wesm commented Sep 7, 2017

wesm commented Sep 7, 2017

asuhan commented Sep 7, 2017

wesm commented Sep 7, 2017

wesm commented Sep 8, 2017 • edited

wesm commented Sep 8, 2017

asuhan commented Sep 8, 2017

wesm commented Sep 8, 2017

wesm commented Sep 3, 2017 •

edited

CLAassistant commented Sep 3, 2017 •

edited

wesm commented Sep 8, 2017 •

edited