ARROW-15258: [C++] Easy options to create a source node from a table - #12267

Closed
vibhatha wants to merge 18 commits into
apache:masterfrom
vibhatha:arrow-15258-rb
Closed

ARROW-15258: [C++] Easy options to create a source node from a table#12267
vibhatha wants to merge 18 commits into
apache:masterfrom
vibhatha:arrow-15258-rb

Conversation

@vibhatha

@vibhathavibhatha commented Jan 26, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

This PR includes the addition of TableSourceNode to create a ExecNode easily using a table as the data source.

TODO

  • Fix test case for chunk_size

@github-actions

github-actionsBot commented Jan 26, 2022

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/ARROW-15258

@vibhathavibhatha changed the title ARROW-15258: [C++] Easy options to create a source node from a tableARROW-15258: [C++] Easy options to create a source node from a table [WIP]Jan 26, 2022
@vibhatha
vibhatha marked this pull request as draft January 26, 2022 09:44
@vibhatha
vibhatha marked this pull request as ready for review January 26, 2022 09:44
@vibhathavibhatha changed the title ARROW-15258: [C++] Easy options to create a source node from a table [WIP]ARROW-15258: [C++] Easy options to create a source node from a tableJan 26, 2022
@vibhatha
vibhatha marked this pull request as draft January 26, 2022 11:26
westonpace
westonpace reviewed Jan 26, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
@vibhatha
vibhatha marked this pull request as ready for review January 27, 2022 13:13
westonpace
westonpace requested changes Jan 28, 2022
View reviewed changes

@westonpacewestonpace left a comment

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.

This looks great. I have a lot of minor comments about style but the actual substance looks correct to me.

Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated

@westonpacewestonpaceJan 28, 2022

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.

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).

@vibhathavibhathaFeb 1, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes, this is indeed neater and safe.

@vibhathavibhathaFeb 1, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

@vibhathavibhathaFeb 1, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Instead what if we add

auto batch_res = reader->Next();
if (batch_res.ok()) {
batch = batch_res.ValueOrDie();
}

Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/plan_test.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
westonpace
westonpace requested changes Feb 14, 2022
View reviewed changes

@westonpacewestonpace left a comment

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.

Thanks for fixing things up. Sorry for the delay on this review.

Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
@westonpacewestonpace mentioned this pull request Feb 14, 2022
Closed
@ursabot

ursabot commented Feb 24, 2022•
edited
Loading

Copy link
Copy Markdown

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.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.5% ⬆️0.0%] test-mac-arm
[Finished ⬇️1.07% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.13% ⬆️0.04%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

marin-ma pushed a commit to marin-ma/arrow-1 that referenced this pull request Mar 28, 2022
@vibhatha@marin-ma
ARROW-15258: [C++] Easy options to create a source node from a table
64dbf85
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
Closesapache#12267 from vibhatha/arrow-15258-rb
Authored-by: Vibhatha Abeykoon <vibhatha@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
(cherry picked from commit fffdca2)
zhouyuan pushed a commit to oap-project/arrow that referenced this pull request Apr 18, 2022
@marin-ma@bkietz
@jvanstraten@westonpace@vibhatha@pitrou
Enable TPCH Q6 for gazelle-jni arrow backend (#93)
175266c
* 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.
Closesapache#12279 from jvanstraten/substrait-consumer
Lead-authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Co-authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Co-authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
* 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.
Closesapache#12448 from jvanstraten/ARROW-15700-Compilation-error-on-Ubuntu-18-04
Authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
(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
Closesapache#12267 from vibhatha/arrow-15258-rb
Authored-by: Vibhatha Abeykoon <vibhatha@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
(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>
Closesapache#12457 from jvanstraten/ARROW-15709-Compilation-of-ARROW-ENGINE-fails-if-doi
Authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
(cherry picked from commit 89cc6b3)
* ARROW-15830: [C++] Ensure target directory exists before running Substrait generation
Closesapache#12548 from pitrou/ARROW-15830-ubuntu-cpp-bundled
Authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
(cherry picked from commit e989fb3)
* ARROW-15850: [C++] Engine substrait headers missing from install
Closesapache#12569 from westonpace/feature/ARROW-15850--substrait-headers-missing
Authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
(cherry picked from commit 8fce593)
* Enable TPCH Q6 & Q1
Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Co-authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Co-authored-by: Weston Pace <weston.pace@gmail.com>
Co-authored-by: Vibhatha Abeykoon <vibhatha@gmail.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@westonpacewestonpacewestonpace approved these changes

Assignees

No one assigned

Labels

Component: C++Component: C#Component: FlightRPCComponent: GandivaComponent: GLibComponent: GoComponent: JavaComponent: JavaScriptComponent: MATLABComponent: ParquetComponent: PythonComponent: RComponent: Ruby

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vibhatha@ursabot@westonpace
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all \u003cpre\u003e\u003ccode\u003e blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks"); } } catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); } })(); (function(){ try { var __m = "github.com"; var __re = new RegExp('^' + "github\\.com" + '
Skip to content

ARROW-15258: [C++] Easy options to create a source node from a table - #12267

Closed
vibhatha wants to merge 18 commits into
apache:masterfrom
vibhatha:arrow-15258-rb
Closed

ARROW-15258: [C++] Easy options to create a source node from a table#12267
vibhatha wants to merge 18 commits into
apache:masterfrom
vibhatha:arrow-15258-rb

Conversation

@vibhatha

@vibhathavibhatha commented Jan 26, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

This PR includes the addition of TableSourceNode to create a ExecNode easily using a table as the data source.

TODO

  • Fix test case for chunk_size

@github-actions

github-actionsBot commented Jan 26, 2022

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/ARROW-15258

@vibhathavibhatha changed the title ARROW-15258: [C++] Easy options to create a source node from a tableARROW-15258: [C++] Easy options to create a source node from a table [WIP]Jan 26, 2022
@vibhatha
vibhatha marked this pull request as draft January 26, 2022 09:44
@vibhatha
vibhatha marked this pull request as ready for review January 26, 2022 09:44
@vibhathavibhatha changed the title ARROW-15258: [C++] Easy options to create a source node from a table [WIP]ARROW-15258: [C++] Easy options to create a source node from a tableJan 26, 2022
@vibhatha
vibhatha marked this pull request as draft January 26, 2022 11:26
westonpace
westonpace reviewed Jan 26, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
@vibhatha
vibhatha marked this pull request as ready for review January 27, 2022 13:13
westonpace
westonpace requested changes Jan 28, 2022
View reviewed changes

@westonpacewestonpace left a comment

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.

This looks great. I have a lot of minor comments about style but the actual substance looks correct to me.

Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated

@westonpacewestonpaceJan 28, 2022

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.

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).

@vibhathavibhathaFeb 1, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes, this is indeed neater and safe.

@vibhathavibhathaFeb 1, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

@vibhathavibhathaFeb 1, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Instead what if we add

auto batch_res = reader->Next();
if (batch_res.ok()) {
batch = batch_res.ValueOrDie();
}

Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/plan_test.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
westonpace
westonpace requested changes Feb 14, 2022
View reviewed changes

@westonpacewestonpace left a comment

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.

Thanks for fixing things up. Sorry for the delay on this review.

Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
@westonpacewestonpace mentioned this pull request Feb 14, 2022
Closed
@ursabot

ursabot commented Feb 24, 2022•
edited
Loading

Copy link
Copy Markdown

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.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.5% ⬆️0.0%] test-mac-arm
[Finished ⬇️1.07% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.13% ⬆️0.04%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

marin-ma pushed a commit to marin-ma/arrow-1 that referenced this pull request Mar 28, 2022
@vibhatha@marin-ma
ARROW-15258: [C++] Easy options to create a source node from a table
64dbf85
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
Closesapache#12267 from vibhatha/arrow-15258-rb
Authored-by: Vibhatha Abeykoon <vibhatha@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
(cherry picked from commit fffdca2)
zhouyuan pushed a commit to oap-project/arrow that referenced this pull request Apr 18, 2022
@marin-ma@bkietz
@jvanstraten@westonpace@vibhatha@pitrou
Enable TPCH Q6 for gazelle-jni arrow backend (#93)
175266c
* 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.
Closesapache#12279 from jvanstraten/substrait-consumer
Lead-authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Co-authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Co-authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
* 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.
Closesapache#12448 from jvanstraten/ARROW-15700-Compilation-error-on-Ubuntu-18-04
Authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
(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
Closesapache#12267 from vibhatha/arrow-15258-rb
Authored-by: Vibhatha Abeykoon <vibhatha@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
(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>
Closesapache#12457 from jvanstraten/ARROW-15709-Compilation-of-ARROW-ENGINE-fails-if-doi
Authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
(cherry picked from commit 89cc6b3)
* ARROW-15830: [C++] Ensure target directory exists before running Substrait generation
Closesapache#12548 from pitrou/ARROW-15830-ubuntu-cpp-bundled
Authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
(cherry picked from commit e989fb3)
* ARROW-15850: [C++] Engine substrait headers missing from install
Closesapache#12569 from westonpace/feature/ARROW-15850--substrait-headers-missing
Authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
(cherry picked from commit 8fce593)
* Enable TPCH Q6 & Q1
Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Co-authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Co-authored-by: Weston Pace <weston.pace@gmail.com>
Co-authored-by: Vibhatha Abeykoon <vibhatha@gmail.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@westonpacewestonpacewestonpace approved these changes

Assignees

No one assigned

Labels

Component: C++Component: C#Component: FlightRPCComponent: GandivaComponent: GLibComponent: GoComponent: JavaComponent: JavaScriptComponent: MATLABComponent: ParquetComponent: PythonComponent: RComponent: Ruby

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vibhatha@ursabot@westonpace
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

ARROW-15258: [C++] Easy options to create a source node from a table - #12267

Closed
vibhatha wants to merge 18 commits into
apache:masterfrom
vibhatha:arrow-15258-rb
Closed

ARROW-15258: [C++] Easy options to create a source node from a table#12267
vibhatha wants to merge 18 commits into
apache:masterfrom
vibhatha:arrow-15258-rb

Conversation

@vibhatha

@vibhathavibhatha commented Jan 26, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

This PR includes the addition of TableSourceNode to create a ExecNode easily using a table as the data source.

TODO

  • Fix test case for chunk_size

@github-actions

github-actionsBot commented Jan 26, 2022

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/ARROW-15258

@vibhathavibhatha changed the title ARROW-15258: [C++] Easy options to create a source node from a tableARROW-15258: [C++] Easy options to create a source node from a table [WIP]Jan 26, 2022
@vibhatha
vibhatha marked this pull request as draft January 26, 2022 09:44
@vibhatha
vibhatha marked this pull request as ready for review January 26, 2022 09:44
@vibhathavibhatha changed the title ARROW-15258: [C++] Easy options to create a source node from a table [WIP]ARROW-15258: [C++] Easy options to create a source node from a tableJan 26, 2022
@vibhatha
vibhatha marked this pull request as draft January 26, 2022 11:26
westonpace
westonpace reviewed Jan 26, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
@vibhatha
vibhatha marked this pull request as ready for review January 27, 2022 13:13
westonpace
westonpace requested changes Jan 28, 2022
View reviewed changes

@westonpacewestonpace left a comment

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.

This looks great. I have a lot of minor comments about style but the actual substance looks correct to me.

Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated

@westonpacewestonpaceJan 28, 2022

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.

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).

@vibhathavibhathaFeb 1, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes, this is indeed neater and safe.

@vibhathavibhathaFeb 1, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

@vibhathavibhathaFeb 1, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Instead what if we add

auto batch_res = reader->Next();
if (batch_res.ok()) {
batch = batch_res.ValueOrDie();
}

Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/plan_test.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
westonpace
westonpace requested changes Feb 14, 2022
View reviewed changes

@westonpacewestonpace left a comment

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.

Thanks for fixing things up. Sorry for the delay on this review.

Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
@westonpacewestonpace mentioned this pull request Feb 14, 2022
Closed
@ursabot

ursabot commented Feb 24, 2022•
edited
Loading

Copy link
Copy Markdown

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.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.5% ⬆️0.0%] test-mac-arm
[Finished ⬇️1.07% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.13% ⬆️0.04%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

marin-ma pushed a commit to marin-ma/arrow-1 that referenced this pull request Mar 28, 2022
@vibhatha@marin-ma
ARROW-15258: [C++] Easy options to create a source node from a table
64dbf85
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
Closesapache#12267 from vibhatha/arrow-15258-rb
Authored-by: Vibhatha Abeykoon <vibhatha@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
(cherry picked from commit fffdca2)
zhouyuan pushed a commit to oap-project/arrow that referenced this pull request Apr 18, 2022
@marin-ma@bkietz
@jvanstraten@westonpace@vibhatha@pitrou
Enable TPCH Q6 for gazelle-jni arrow backend (#93)
175266c
* 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.
Closesapache#12279 from jvanstraten/substrait-consumer
Lead-authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Co-authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Co-authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
* 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.
Closesapache#12448 from jvanstraten/ARROW-15700-Compilation-error-on-Ubuntu-18-04
Authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
(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
Closesapache#12267 from vibhatha/arrow-15258-rb
Authored-by: Vibhatha Abeykoon <vibhatha@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
(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>
Closesapache#12457 from jvanstraten/ARROW-15709-Compilation-of-ARROW-ENGINE-fails-if-doi
Authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
(cherry picked from commit 89cc6b3)
* ARROW-15830: [C++] Ensure target directory exists before running Substrait generation
Closesapache#12548 from pitrou/ARROW-15830-ubuntu-cpp-bundled
Authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
(cherry picked from commit e989fb3)
* ARROW-15850: [C++] Engine substrait headers missing from install
Closesapache#12569 from westonpace/feature/ARROW-15850--substrait-headers-missing
Authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
(cherry picked from commit 8fce593)
* Enable TPCH Q6 & Q1
Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Co-authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Co-authored-by: Weston Pace <weston.pace@gmail.com>
Co-authored-by: Vibhatha Abeykoon <vibhatha@gmail.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@westonpacewestonpacewestonpace approved these changes

Assignees

No one assigned

Labels

Component: C++Component: C#Component: FlightRPCComponent: GandivaComponent: GLibComponent: GoComponent: JavaComponent: JavaScriptComponent: MATLABComponent: ParquetComponent: PythonComponent: RComponent: Ruby

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vibhatha@ursabot@westonpace
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length \u003e 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

ARROW-15258: [C++] Easy options to create a source node from a table - #12267

Closed
vibhatha wants to merge 18 commits into
apache:masterfrom
vibhatha:arrow-15258-rb
Closed

ARROW-15258: [C++] Easy options to create a source node from a table#12267
vibhatha wants to merge 18 commits into
apache:masterfrom
vibhatha:arrow-15258-rb

Conversation

@vibhatha

@vibhathavibhatha commented Jan 26, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

This PR includes the addition of TableSourceNode to create a ExecNode easily using a table as the data source.

TODO

  • Fix test case for chunk_size

@github-actions

github-actionsBot commented Jan 26, 2022

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/ARROW-15258

@vibhathavibhatha changed the title ARROW-15258: [C++] Easy options to create a source node from a tableARROW-15258: [C++] Easy options to create a source node from a table [WIP]Jan 26, 2022
@vibhatha
vibhatha marked this pull request as draft January 26, 2022 09:44
@vibhatha
vibhatha marked this pull request as ready for review January 26, 2022 09:44
@vibhathavibhatha changed the title ARROW-15258: [C++] Easy options to create a source node from a table [WIP]ARROW-15258: [C++] Easy options to create a source node from a tableJan 26, 2022
@vibhatha
vibhatha marked this pull request as draft January 26, 2022 11:26
westonpace
westonpace reviewed Jan 26, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
@vibhatha
vibhatha marked this pull request as ready for review January 27, 2022 13:13
westonpace
westonpace requested changes Jan 28, 2022
View reviewed changes

@westonpacewestonpace left a comment

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.

This looks great. I have a lot of minor comments about style but the actual substance looks correct to me.

Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated

@westonpacewestonpaceJan 28, 2022

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.

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).

@vibhathavibhathaFeb 1, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes, this is indeed neater and safe.

@vibhathavibhathaFeb 1, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

@vibhathavibhathaFeb 1, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Instead what if we add

auto batch_res = reader->Next();
if (batch_res.ok()) {
batch = batch_res.ValueOrDie();
}

Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/plan_test.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
westonpace
westonpace requested changes Feb 14, 2022
View reviewed changes

@westonpacewestonpace left a comment

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.

Thanks for fixing things up. Sorry for the delay on this review.

Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
@westonpacewestonpace mentioned this pull request Feb 14, 2022
Closed
@ursabot

ursabot commented Feb 24, 2022•
edited
Loading

Copy link
Copy Markdown

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.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.5% ⬆️0.0%] test-mac-arm
[Finished ⬇️1.07% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.13% ⬆️0.04%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

marin-ma pushed a commit to marin-ma/arrow-1 that referenced this pull request Mar 28, 2022
@vibhatha@marin-ma
ARROW-15258: [C++] Easy options to create a source node from a table
64dbf85
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
Closesapache#12267 from vibhatha/arrow-15258-rb
Authored-by: Vibhatha Abeykoon <vibhatha@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
(cherry picked from commit fffdca2)
zhouyuan pushed a commit to oap-project/arrow that referenced this pull request Apr 18, 2022
@marin-ma@bkietz
@jvanstraten@westonpace@vibhatha@pitrou
Enable TPCH Q6 for gazelle-jni arrow backend (#93)
175266c
* 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.
Closesapache#12279 from jvanstraten/substrait-consumer
Lead-authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Co-authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Co-authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
* 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.
Closesapache#12448 from jvanstraten/ARROW-15700-Compilation-error-on-Ubuntu-18-04
Authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
(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
Closesapache#12267 from vibhatha/arrow-15258-rb
Authored-by: Vibhatha Abeykoon <vibhatha@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
(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>
Closesapache#12457 from jvanstraten/ARROW-15709-Compilation-of-ARROW-ENGINE-fails-if-doi
Authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
(cherry picked from commit 89cc6b3)
* ARROW-15830: [C++] Ensure target directory exists before running Substrait generation
Closesapache#12548 from pitrou/ARROW-15830-ubuntu-cpp-bundled
Authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
(cherry picked from commit e989fb3)
* ARROW-15850: [C++] Engine substrait headers missing from install
Closesapache#12569 from westonpace/feature/ARROW-15850--substrait-headers-missing
Authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
(cherry picked from commit 8fce593)
* Enable TPCH Q6 & Q1
Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Co-authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Co-authored-by: Weston Pace <weston.pace@gmail.com>
Co-authored-by: Vibhatha Abeykoon <vibhatha@gmail.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@westonpacewestonpacewestonpace approved these changes

Assignees

No one assigned

Labels

Component: C++Component: C#Component: FlightRPCComponent: GandivaComponent: GLibComponent: GoComponent: JavaComponent: JavaScriptComponent: MATLABComponent: ParquetComponent: PythonComponent: RComponent: Ruby

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vibhatha@ursabot@westonpace
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

ARROW-15258: [C++] Easy options to create a source node from a table - #12267

Closed
vibhatha wants to merge 18 commits into
apache:masterfrom
vibhatha:arrow-15258-rb
Closed

ARROW-15258: [C++] Easy options to create a source node from a table#12267
vibhatha wants to merge 18 commits into
apache:masterfrom
vibhatha:arrow-15258-rb

Conversation

@vibhatha

@vibhathavibhatha commented Jan 26, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

This PR includes the addition of TableSourceNode to create a ExecNode easily using a table as the data source.

TODO

  • Fix test case for chunk_size

@github-actions

github-actionsBot commented Jan 26, 2022

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/ARROW-15258

@vibhathavibhatha changed the title ARROW-15258: [C++] Easy options to create a source node from a tableARROW-15258: [C++] Easy options to create a source node from a table [WIP]Jan 26, 2022
@vibhatha
vibhatha marked this pull request as draft January 26, 2022 09:44
@vibhatha
vibhatha marked this pull request as ready for review January 26, 2022 09:44
@vibhathavibhatha changed the title ARROW-15258: [C++] Easy options to create a source node from a table [WIP]ARROW-15258: [C++] Easy options to create a source node from a tableJan 26, 2022
@vibhatha
vibhatha marked this pull request as draft January 26, 2022 11:26
westonpace
westonpace reviewed Jan 26, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
@vibhatha
vibhatha marked this pull request as ready for review January 27, 2022 13:13
westonpace
westonpace requested changes Jan 28, 2022
View reviewed changes

@westonpacewestonpace left a comment

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.

This looks great. I have a lot of minor comments about style but the actual substance looks correct to me.

Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated

@westonpacewestonpaceJan 28, 2022

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.

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).

@vibhathavibhathaFeb 1, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes, this is indeed neater and safe.

@vibhathavibhathaFeb 1, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

@vibhathavibhathaFeb 1, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Instead what if we add

auto batch_res = reader->Next();
if (batch_res.ok()) {
batch = batch_res.ValueOrDie();
}

Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/plan_test.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
westonpace
westonpace requested changes Feb 14, 2022
View reviewed changes

@westonpacewestonpace left a comment

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.

Thanks for fixing things up. Sorry for the delay on this review.

Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
@westonpacewestonpace mentioned this pull request Feb 14, 2022
Closed
@ursabot

ursabot commented Feb 24, 2022•
edited
Loading

Copy link
Copy Markdown

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.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.5% ⬆️0.0%] test-mac-arm
[Finished ⬇️1.07% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.13% ⬆️0.04%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

marin-ma pushed a commit to marin-ma/arrow-1 that referenced this pull request Mar 28, 2022
@vibhatha@marin-ma
ARROW-15258: [C++] Easy options to create a source node from a table
64dbf85
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
Closesapache#12267 from vibhatha/arrow-15258-rb
Authored-by: Vibhatha Abeykoon <vibhatha@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
(cherry picked from commit fffdca2)
zhouyuan pushed a commit to oap-project/arrow that referenced this pull request Apr 18, 2022
@marin-ma@bkietz
@jvanstraten@westonpace@vibhatha@pitrou
Enable TPCH Q6 for gazelle-jni arrow backend (#93)
175266c
* 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.
Closesapache#12279 from jvanstraten/substrait-consumer
Lead-authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Co-authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Co-authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
* 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.
Closesapache#12448 from jvanstraten/ARROW-15700-Compilation-error-on-Ubuntu-18-04
Authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
(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
Closesapache#12267 from vibhatha/arrow-15258-rb
Authored-by: Vibhatha Abeykoon <vibhatha@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
(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>
Closesapache#12457 from jvanstraten/ARROW-15709-Compilation-of-ARROW-ENGINE-fails-if-doi
Authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
(cherry picked from commit 89cc6b3)
* ARROW-15830: [C++] Ensure target directory exists before running Substrait generation
Closesapache#12548 from pitrou/ARROW-15830-ubuntu-cpp-bundled
Authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
(cherry picked from commit e989fb3)
* ARROW-15850: [C++] Engine substrait headers missing from install
Closesapache#12569 from westonpace/feature/ARROW-15850--substrait-headers-missing
Authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
(cherry picked from commit 8fce593)
* Enable TPCH Q6 & Q1
Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Co-authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Co-authored-by: Weston Pace <weston.pace@gmail.com>
Co-authored-by: Vibhatha Abeykoon <vibhatha@gmail.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@westonpacewestonpacewestonpace approved these changes

Assignees

No one assigned

Labels

Component: C++Component: C#Component: FlightRPCComponent: GandivaComponent: GLibComponent: GoComponent: JavaComponent: JavaScriptComponent: MATLABComponent: ParquetComponent: PythonComponent: RComponent: Ruby

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vibhatha@ursabot@westonpace
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

ARROW-15258: [C++] Easy options to create a source node from a table - #12267

Closed
vibhatha wants to merge 18 commits into
apache:masterfrom
vibhatha:arrow-15258-rb
Closed

ARROW-15258: [C++] Easy options to create a source node from a table#12267
vibhatha wants to merge 18 commits into
apache:masterfrom
vibhatha:arrow-15258-rb

Conversation

@vibhatha

@vibhathavibhatha commented Jan 26, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

This PR includes the addition of TableSourceNode to create a ExecNode easily using a table as the data source.

TODO

  • Fix test case for chunk_size

@github-actions

github-actionsBot commented Jan 26, 2022

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/ARROW-15258

@vibhathavibhatha changed the title ARROW-15258: [C++] Easy options to create a source node from a tableARROW-15258: [C++] Easy options to create a source node from a table [WIP]Jan 26, 2022
@vibhatha
vibhatha marked this pull request as draft January 26, 2022 09:44
@vibhatha
vibhatha marked this pull request as ready for review January 26, 2022 09:44
@vibhathavibhatha changed the title ARROW-15258: [C++] Easy options to create a source node from a table [WIP]ARROW-15258: [C++] Easy options to create a source node from a tableJan 26, 2022
@vibhatha
vibhatha marked this pull request as draft January 26, 2022 11:26
westonpace
westonpace reviewed Jan 26, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
@vibhatha
vibhatha marked this pull request as ready for review January 27, 2022 13:13
westonpace
westonpace requested changes Jan 28, 2022
View reviewed changes

@westonpacewestonpace left a comment

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.

This looks great. I have a lot of minor comments about style but the actual substance looks correct to me.

Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated

@westonpacewestonpaceJan 28, 2022

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.

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).

@vibhathavibhathaFeb 1, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes, this is indeed neater and safe.

@vibhathavibhathaFeb 1, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

@vibhathavibhathaFeb 1, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Instead what if we add

auto batch_res = reader->Next();
if (batch_res.ok()) {
batch = batch_res.ValueOrDie();
}

Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/plan_test.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
westonpace
westonpace requested changes Feb 14, 2022
View reviewed changes

@westonpacewestonpace left a comment

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.

Thanks for fixing things up. Sorry for the delay on this review.

Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
@westonpacewestonpace mentioned this pull request Feb 14, 2022
Closed
@ursabot

ursabot commented Feb 24, 2022•
edited
Loading

Copy link
Copy Markdown

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.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.5% ⬆️0.0%] test-mac-arm
[Finished ⬇️1.07% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.13% ⬆️0.04%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

marin-ma pushed a commit to marin-ma/arrow-1 that referenced this pull request Mar 28, 2022
@vibhatha@marin-ma
ARROW-15258: [C++] Easy options to create a source node from a table
64dbf85
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
Closesapache#12267 from vibhatha/arrow-15258-rb
Authored-by: Vibhatha Abeykoon <vibhatha@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
(cherry picked from commit fffdca2)
zhouyuan pushed a commit to oap-project/arrow that referenced this pull request Apr 18, 2022
@marin-ma@bkietz
@jvanstraten@westonpace@vibhatha@pitrou
Enable TPCH Q6 for gazelle-jni arrow backend (#93)
175266c
* 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.
Closesapache#12279 from jvanstraten/substrait-consumer
Lead-authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Co-authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Co-authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
* 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.
Closesapache#12448 from jvanstraten/ARROW-15700-Compilation-error-on-Ubuntu-18-04
Authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
(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
Closesapache#12267 from vibhatha/arrow-15258-rb
Authored-by: Vibhatha Abeykoon <vibhatha@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
(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>
Closesapache#12457 from jvanstraten/ARROW-15709-Compilation-of-ARROW-ENGINE-fails-if-doi
Authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
(cherry picked from commit 89cc6b3)
* ARROW-15830: [C++] Ensure target directory exists before running Substrait generation
Closesapache#12548 from pitrou/ARROW-15830-ubuntu-cpp-bundled
Authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
(cherry picked from commit e989fb3)
* ARROW-15850: [C++] Engine substrait headers missing from install
Closesapache#12569 from westonpace/feature/ARROW-15850--substrait-headers-missing
Authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
(cherry picked from commit 8fce593)
* Enable TPCH Q6 & Q1
Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Co-authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Co-authored-by: Weston Pace <weston.pace@gmail.com>
Co-authored-by: Vibhatha Abeykoon <vibhatha@gmail.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@westonpacewestonpacewestonpace approved these changes

Assignees

No one assigned

Labels

Component: C++Component: C#Component: FlightRPCComponent: GandivaComponent: GLibComponent: GoComponent: JavaComponent: JavaScriptComponent: MATLABComponent: ParquetComponent: PythonComponent: RComponent: Ruby

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vibhatha@ursabot@westonpace
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

ARROW-15258: [C++] Easy options to create a source node from a table - #12267

Closed
vibhatha wants to merge 18 commits into
apache:masterfrom
vibhatha:arrow-15258-rb
Closed

ARROW-15258: [C++] Easy options to create a source node from a table#12267
vibhatha wants to merge 18 commits into
apache:masterfrom
vibhatha:arrow-15258-rb

Conversation

@vibhatha

@vibhathavibhatha commented Jan 26, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

This PR includes the addition of TableSourceNode to create a ExecNode easily using a table as the data source.

TODO

  • Fix test case for chunk_size

@github-actions

github-actionsBot commented Jan 26, 2022

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/ARROW-15258

@vibhathavibhatha changed the title ARROW-15258: [C++] Easy options to create a source node from a tableARROW-15258: [C++] Easy options to create a source node from a table [WIP]Jan 26, 2022
@vibhatha
vibhatha marked this pull request as draft January 26, 2022 09:44
@vibhatha
vibhatha marked this pull request as ready for review January 26, 2022 09:44
@vibhathavibhatha changed the title ARROW-15258: [C++] Easy options to create a source node from a table [WIP]ARROW-15258: [C++] Easy options to create a source node from a tableJan 26, 2022
@vibhatha
vibhatha marked this pull request as draft January 26, 2022 11:26
westonpace
westonpace reviewed Jan 26, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
@vibhatha
vibhatha marked this pull request as ready for review January 27, 2022 13:13
westonpace
westonpace requested changes Jan 28, 2022
View reviewed changes

@westonpacewestonpace left a comment

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.

This looks great. I have a lot of minor comments about style but the actual substance looks correct to me.

Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated

@westonpacewestonpaceJan 28, 2022

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.

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).

@vibhathavibhathaFeb 1, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes, this is indeed neater and safe.

@vibhathavibhathaFeb 1, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

@vibhathavibhathaFeb 1, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Instead what if we add

auto batch_res = reader->Next();
if (batch_res.ok()) {
batch = batch_res.ValueOrDie();
}

Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/plan_test.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
westonpace
westonpace requested changes Feb 14, 2022
View reviewed changes

@westonpacewestonpace left a comment

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.

Thanks for fixing things up. Sorry for the delay on this review.

Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
@westonpacewestonpace mentioned this pull request Feb 14, 2022
Closed
@ursabot

ursabot commented Feb 24, 2022•
edited
Loading

Copy link
Copy Markdown

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.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.5% ⬆️0.0%] test-mac-arm
[Finished ⬇️1.07% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.13% ⬆️0.04%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

marin-ma pushed a commit to marin-ma/arrow-1 that referenced this pull request Mar 28, 2022
@vibhatha@marin-ma
ARROW-15258: [C++] Easy options to create a source node from a table
64dbf85
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
Closesapache#12267 from vibhatha/arrow-15258-rb
Authored-by: Vibhatha Abeykoon <vibhatha@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
(cherry picked from commit fffdca2)
zhouyuan pushed a commit to oap-project/arrow that referenced this pull request Apr 18, 2022
@marin-ma@bkietz
@jvanstraten@westonpace@vibhatha@pitrou
Enable TPCH Q6 for gazelle-jni arrow backend (#93)
175266c
* 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.
Closesapache#12279 from jvanstraten/substrait-consumer
Lead-authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Co-authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Co-authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
* 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.
Closesapache#12448 from jvanstraten/ARROW-15700-Compilation-error-on-Ubuntu-18-04
Authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
(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
Closesapache#12267 from vibhatha/arrow-15258-rb
Authored-by: Vibhatha Abeykoon <vibhatha@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
(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>
Closesapache#12457 from jvanstraten/ARROW-15709-Compilation-of-ARROW-ENGINE-fails-if-doi
Authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
(cherry picked from commit 89cc6b3)
* ARROW-15830: [C++] Ensure target directory exists before running Substrait generation
Closesapache#12548 from pitrou/ARROW-15830-ubuntu-cpp-bundled
Authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
(cherry picked from commit e989fb3)
* ARROW-15850: [C++] Engine substrait headers missing from install
Closesapache#12569 from westonpace/feature/ARROW-15850--substrait-headers-missing
Authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
(cherry picked from commit 8fce593)
* Enable TPCH Q6 & Q1
Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Co-authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Co-authored-by: Weston Pace <weston.pace@gmail.com>
Co-authored-by: Vibhatha Abeykoon <vibhatha@gmail.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@westonpacewestonpacewestonpace approved these changes

Assignees

No one assigned

Labels

Component: C++Component: C#Component: FlightRPCComponent: GandivaComponent: GLibComponent: GoComponent: JavaComponent: JavaScriptComponent: MATLABComponent: ParquetComponent: PythonComponent: RComponent: Ruby

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vibhatha@ursabot@westonpace
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

ARROW-15258: [C++] Easy options to create a source node from a table - #12267

Closed
vibhatha wants to merge 18 commits into
apache:masterfrom
vibhatha:arrow-15258-rb
Closed

ARROW-15258: [C++] Easy options to create a source node from a table#12267
vibhatha wants to merge 18 commits into
apache:masterfrom
vibhatha:arrow-15258-rb

Conversation

@vibhatha

@vibhathavibhatha commented Jan 26, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

This PR includes the addition of TableSourceNode to create a ExecNode easily using a table as the data source.

TODO

  • Fix test case for chunk_size

@github-actions

github-actionsBot commented Jan 26, 2022

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/ARROW-15258

@vibhathavibhatha changed the title ARROW-15258: [C++] Easy options to create a source node from a tableARROW-15258: [C++] Easy options to create a source node from a table [WIP]Jan 26, 2022
@vibhatha
vibhatha marked this pull request as draft January 26, 2022 09:44
@vibhatha
vibhatha marked this pull request as ready for review January 26, 2022 09:44
@vibhathavibhatha changed the title ARROW-15258: [C++] Easy options to create a source node from a table [WIP]ARROW-15258: [C++] Easy options to create a source node from a tableJan 26, 2022
@vibhatha
vibhatha marked this pull request as draft January 26, 2022 11:26
westonpace
westonpace reviewed Jan 26, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
@vibhatha
vibhatha marked this pull request as ready for review January 27, 2022 13:13
westonpace
westonpace requested changes Jan 28, 2022
View reviewed changes

@westonpacewestonpace left a comment

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.

This looks great. I have a lot of minor comments about style but the actual substance looks correct to me.

Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated

@westonpacewestonpaceJan 28, 2022

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.

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).

@vibhathavibhathaFeb 1, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes, this is indeed neater and safe.

@vibhathavibhathaFeb 1, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

@vibhathavibhathaFeb 1, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Instead what if we add

auto batch_res = reader->Next();
if (batch_res.ok()) {
batch = batch_res.ValueOrDie();
}

Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/plan_test.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
westonpace
westonpace requested changes Feb 14, 2022
View reviewed changes

@westonpacewestonpace left a comment

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.

Thanks for fixing things up. Sorry for the delay on this review.

Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/options.h Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
Comment threadcpp/src/arrow/compute/exec/source_node.cc Outdated
@westonpacewestonpace mentioned this pull request Feb 14, 2022
Closed
@ursabot

ursabot commented Feb 24, 2022•
edited
Loading

Copy link
Copy Markdown

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.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.5% ⬆️0.0%] test-mac-arm
[Finished ⬇️1.07% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.13% ⬆️0.04%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

marin-ma pushed a commit to marin-ma/arrow-1 that referenced this pull request Mar 28, 2022
@vibhatha@marin-ma
ARROW-15258: [C++] Easy options to create a source node from a table
64dbf85
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
Closesapache#12267 from vibhatha/arrow-15258-rb
Authored-by: Vibhatha Abeykoon <vibhatha@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
(cherry picked from commit fffdca2)
zhouyuan pushed a commit to oap-project/arrow that referenced this pull request Apr 18, 2022
@marin-ma@bkietz
@jvanstraten@westonpace@vibhatha@pitrou
Enable TPCH Q6 for gazelle-jni arrow backend (#93)
175266c
* 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.
Closesapache#12279 from jvanstraten/substrait-consumer
Lead-authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Co-authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Co-authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
* 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.
Closesapache#12448 from jvanstraten/ARROW-15700-Compilation-error-on-Ubuntu-18-04
Authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
(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
Closesapache#12267 from vibhatha/arrow-15258-rb
Authored-by: Vibhatha Abeykoon <vibhatha@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
(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>
Closesapache#12457 from jvanstraten/ARROW-15709-Compilation-of-ARROW-ENGINE-fails-if-doi
Authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
(cherry picked from commit 89cc6b3)
* ARROW-15830: [C++] Ensure target directory exists before running Substrait generation
Closesapache#12548 from pitrou/ARROW-15830-ubuntu-cpp-bundled
Authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
(cherry picked from commit e989fb3)
* ARROW-15850: [C++] Engine substrait headers missing from install
Closesapache#12569 from westonpace/feature/ARROW-15850--substrait-headers-missing
Authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
(cherry picked from commit 8fce593)
* Enable TPCH Q6 & Q1
Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Co-authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
Co-authored-by: Weston Pace <weston.pace@gmail.com>
Co-authored-by: Vibhatha Abeykoon <vibhatha@gmail.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@westonpacewestonpacewestonpace approved these changes

Assignees

No one assigned

Labels

Component: C++Component: C#Component: FlightRPCComponent: GandivaComponent: GLibComponent: GoComponent: JavaComponent: JavaScriptComponent: MATLABComponent: ParquetComponent: PythonComponent: RComponent: Ruby

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vibhatha@ursabot@westonpace