ARROW-15258: [C++] Easy options to create a source node from a table #12267
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vibhatha
changed the title
vibhatha
changed the title
westonpace
reviewed
Jan 26, 2022
westonpace
requested changes
Jan 28, 2022
Member
westonpace
left a comment
westonpace
left a comment
There was a problem hiding this comment.
This looks great. I have a lot of minor comments about style but the actual substance looks correct to me.
Member
There was a problem hiding this comment.
Can you use ASSIGN_OR_ABORT and add a comment explaining that a TableBatchReader should not be able to fail but always returns an OK status to comply with the RecordBatchReader interface.
Then you can change the return value to std::vector<ExecBatch> and get rid of the ValueOrDie
Otherwise I don't think it would be safe to be doing this kind of work in a constructor (and we should be doing it in the Make method instead so we can propagate the failure).
Collaborator
Author
There was a problem hiding this comment.
Yes, this is indeed neater and safe.
Collaborator
Author
There was a problem hiding this comment.
Isn't this is a part of gtest_util.h, should we include it? I see it's usage is only in the tests and benchmarks.
Did I interpret it right? Please correct me if I am wrong.
Collaborator
Author
There was a problem hiding this comment.
Instead what if we add
auto batch_res = reader->Next();
if (batch_res.ok()) {
batch = batch_res.ValueOrDie();
}
vibhatha
force-pushed
the
arrow-15258-rb
branch
from
1c86ddc to
d91d2ee
Compare
vibhatha
force-pushed
the
arrow-15258-rb
branch
from
b9a25d1 to
c333c7e
Compare
westonpace
requested changes
Feb 14, 2022
Member
westonpace
left a comment
westonpace
left a comment
There was a problem hiding this comment.
Thanks for fixing things up. Sorry for the delay on this review.
vibhatha
force-pushed
the
arrow-15258-rb
branch
from
8b85ed5 to
6ac1ed2
Compare
vibhatha
force-pushed
the
arrow-15258-rb
branch
from
bfedc95 to
dec8ea2
Compare
westonpace
approved these changes
Feb 24, 2022
|
Benchmark runs are scheduled for baseline = a26506c and contender = fffdca2. fffdca2 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
marin-ma
pushed a commit
to marin-ma/arrow-1
that referenced
this pull request
Mar 28, 2022
This PR includes the addition of `TableSourceNode` to create a `ExecNode` easily using a table as the data source. ### TODO - [x] Fix test case for chunk_size Closes apache#12267 from vibhatha/arrow-15258-rb Authored-by: Vibhatha Abeykoon <[email protected]> Signed-off-by: Weston Pace <[email protected]> (cherry picked from commit fffdca2)
zhouyuan
pushed a commit
to oap-project/arrow
that referenced
this pull request
Apr 18, 2022
* ARROW-15238: [C++] ARROW_ENGINE module with substrait consumer Continuation of apache#11707. I'm taking over from @bkietz for now because he's unavailable right now for personal reasons. Closes apache#12279 from jvanstraten/substrait-consumer Lead-authored-by: Benjamin Kietzman <[email protected]> Co-authored-by: Jeroen van Straten <[email protected]> Co-authored-by: Weston Pace <[email protected]> Signed-off-by: Weston Pace <[email protected]> * ARROW-15700: [C++] Compilation error on Ubuntu 18.04 Modified Substrait consumer interaction with libprotobuf to support versions down to 3.0.0, which is the minimum required due to Substrait's usage of proto3 syntax. Tested locally with: ``` export ARROW_PROTOBUF_URL=https://github.com/protocolbuffers/protobuf/releases/download/v3.0.0/protobuf-cpp-3.0.0.tar.gz cmake \ --preset ninja-debug \ -DProtobuf_SOURCE=BUNDLED \ -DARROW_PROTOBUF_BUILD_VERSION=v3.0.0 \ -DARROW_PROTOBUF_BUILD_SHA256_CHECKSUM=318e8f375fb4e5333975a40e0d1215e855b4a8c581d692eb0eb7df70db1a8d4e ``` (Is there an easier way to do this without modifying versions.txt or 751fb9d? Also, the env var is needed only because Google isn't at all consistent with their release file naming that far back.) It'd also be nice to add this to CI, but it's probably excessive to always run for a PR, unless combined with some other run. Closes apache#12448 from jvanstraten/ARROW-15700-Compilation-error-on-Ubuntu-18-04 Authored-by: Jeroen van Straten <[email protected]> Signed-off-by: Weston Pace <[email protected]> (cherry picked from commit 7d16a78) * ARROW-15258: [C++] Easy options to create a source node from a table This PR includes the addition of `TableSourceNode` to create a `ExecNode` easily using a table as the data source. ### TODO - [x] Fix test case for chunk_size Closes apache#12267 from vibhatha/arrow-15258-rb Authored-by: Vibhatha Abeykoon <[email protected]> Signed-off-by: Weston Pace <[email protected]> (cherry picked from commit fffdca2) * ARROW-15709: [C++] Compilation of ARROW_ENGINE fails if doing an "inline" build This should fix: - inline builds in general (ARROW-15709); - [weird stuff with inline builds causing non-tracked files to be deleted](https://github.com/apache/arrow;/pull/12444#issuecomment-1043143303) that the [previous fix](apache#12444) for the above [caused](apache#12454) - dependencies on git for downloading dependencies (ARROW-15760); - the build process for Substrait previously being treated as something too special to use Arrow's normal method for dealing with third-party dependencies (i.e. `ThirdpartyToolchain.cmake`) --- Initial attempt at making something functional to solve this issue properly. The use of `add_arrow_lib` in `ThirdpartyToolchain.cmake` is certainly odd, and I'm sure I'm not following best practices in that file in general. I could use some advice on what the proper way to do this would be. Some of the issues: - The CMake property specifying that a path refers to a generated file is scoped only to the current CMake file, so only moving the `externalproject_add` over to `ThirdpartyToolchain.cmake` resulted in the `add_arrow_lib` in `src/arrow/engine` failing due to missing source files. An object library didn't seem to resolve it either, and there are probably portability issues with that anyway, so that's why I ended up just using `add_arrow_lib`. - Unlike all the other third-party dependencies (AFAICT), Substrait can't currently be installed, so `Substrait_SOURCE=SYSTEM` makes no sense. In the end I just decided to override it, but that's probably not ideal. - Substrait doesn't have releases yet, so I had to resort to a git hash instead. </details> Closes apache#12457 from jvanstraten/ARROW-15709-Compilation-of-ARROW-ENGINE-fails-if-doi Authored-by: Jeroen van Straten <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]> (cherry picked from commit 89cc6b3) * ARROW-15830: [C++] Ensure target directory exists before running Substrait generation Closes apache#12548 from pitrou/ARROW-15830-ubuntu-cpp-bundled Authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]> (cherry picked from commit e989fb3) * ARROW-15850: [C++] Engine substrait headers missing from install Closes apache#12569 from westonpace/feature/ARROW-15850--substrait-headers-missing Authored-by: Weston Pace <[email protected]> Signed-off-by: David Li <[email protected]> (cherry picked from commit 8fce593) * Enable TPCH Q6 & Q1 Co-authored-by: Benjamin Kietzman <[email protected]> Co-authored-by: Jeroen van Straten <[email protected]> Co-authored-by: Weston Pace <[email protected]> Co-authored-by: Vibhatha Abeykoon <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR includes the addition of
TableSourceNodeto create aExecNodeeasily using a table as the data source.TODO