ARROW-15238: [C++] ARROW_ENGINE module with substrait consumer - #12279

Closed
jvanstraten wants to merge 116 commits into
apache:masterfrom
jvanstraten:substrait-consumer
Closed

ARROW-15238: [C++] ARROW_ENGINE module with substrait consumer#12279
jvanstraten wants to merge 116 commits into
apache:masterfrom
jvanstraten:substrait-consumer

Conversation

@jvanstraten

@jvanstratenjvanstraten commented Jan 27, 2022

Copy link
Copy Markdown
Contributor

Continuation of #11707. I'm taking over from @bkietz for now because he's unavailable right now for personal reasons.

bkietzand others added 30 commits January 20, 2022 20:34
@bkietz
first pass at using substrait protobufs
ff51e35
@bkietz
add conversion of types and a basic roundtrip test
906aacf
@bkietz
reorganize to engine/substrait/{proto}_internal.cc,h
5fc6c34
@bkietz
add basic literal conversions
151cf72
@bkietz
don't rely on Datum::type in DataEq matcher
1cf679b
@bkietz
add substrait_gen_verify, allow configurable substrait repo and tag
6dce17b
@bkietz
Expose NamedStruct<=>Schema serde
53c7c7d
@bkietz
add if_else <-> IfThen conversion
cd919f3
@bkietz
rebase, catch up to changes in substrait
c079359
@bkietz
finish catching up with substrait's new field references
5c69fb1
@bkietz
add SubstraitFromJSON
2a77d28
@bkietz
port more tests to JSON
a5c25de
@bkietz
add more DataEq matchers
873ec23
@bkietz
use Date64 for substrait::date
05be909
@bkietz
add extension types for interval_*, support deeply nested struct fiel…
7309964
…d names
@bkietz
add basic sketch of ExtensionSet for tracking substrait extensions
27af6b6
@bkietz
get a failing test for arrow::null
37a62de
@bkietz
refactor extension types to index variations alongside types
8397ee5
@bkietz
use an ExtensionSet-local registry
61feb19
@bkietz
add ExtensionSet <-> Plan factories
1eb4bc0
@bkietz
pre merge stash
e63da1e
@bkietz
post rebase cleanup
f37084a
@jvanstraten@bkietz
Changes needed to get branch to compile
2725ed7
Using `--preset ninja-debug`. Build will fail on initially on GCC 11+
due to a use of undefined error from googletest; lower version is needed
to avoid hundreds of instances of an ambiguous print overload, but that
version doesn't have googletest PR 3024 included yet. This change needs
to be made manually in the googletest build tree as downloaded by cmake.
@jvanstraten@bkietz
Fix uninitialized pointers
11e7f3f
@jvanstraten@bkietz
Fix FieldRef/StructField order
96727e1
@bkietz
gcc: use a pointer to the properties tuple instead of a reference
07b259c
@bkietz
advance substrait version
885836a
@bkietz
make JSON utils public, add CheckMessagesEquivalent()
0b94f3a
@jvanstraten@bkietz
Revert now unnecessary part of9255fb6
98c74a8
@jvanstraten@bkietz
Support nested StructFields
6fd2e73
westonpace
westonpace reviewed Feb 5, 2022
View reviewed changes
Comment threadcpp/src/arrow/engine/substrait/relation_internal.cc
westonpace
westonpace reviewed Feb 5, 2022
View reviewed changes
Comment threadcpp/src/arrow/engine/substrait/serde.h Outdated
westonpace
westonpace approved these changes Feb 8, 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.

@jvanstraten and I are going to review this in person tomorrow but at the moment I think most of the major concerns have been addressed.

Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc Outdated
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc Outdated
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
@westonpace

westonpace commented Feb 8, 2022

Copy link
Copy Markdown
Member

Ah, that appveyor Windows error looks like it might be legitimate. Can you investigate that?

jvanstratenand others added 14 commits February 8, 2022 16:54
@jvanstraten
Remove unused test helper function
709ab65
@jvanstraten
Add test for round-tripping Substrait Map<K,V>, and fix implementation
c8e93ea
@westonpace
Added doc strings for SimpleExtensionType
fc9d1a9
@westonpace
Made a simplification pass on the extension set. I added some comment…
8bd00a2
…s explaining the behavior and hid more of the underlying implementation which will help future-proof us in case we move to a map-based version in the future. Fixed a bug in execution plan creation where we were referencing the Substrait function name instead of the Arrow function name.
@jvanstraten
Remove questionable SubstraitToJson & SubstraitFromJson test helper f…
2156716
…unctions
@westonpace
Removing unneeded protobuf cmake flag
f5483f4
@westonpace
Fix up the substrait consumption example to take in a filename instea…
1b8ef91
…d of use hard coded filenames. If no filename is specified we return 0 which is needed because CI will call the examples and we cannot fail. Removed the filter from the example expression because it triggers ARROW-15658 and we need to resolve ARROW-15586 first anyways. Added logic in the scanner to provide a default projection when none is specified.
@westonpace@jvanstraten
Update cpp/src/arrow/engine/substrait/extension_set.h
78d0fca
Co-authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
@westonpace
Cleaned up comments per PR review
2b0131c
@jvanstraten
Merge pull request#1from westonpace/substrait-consumer
cde0b6f
Documented SimpleExtensionType and simplified ExtensionSet
@jvanstraten
Merge pull request#2from westonpace/substrait-consumer-build-fix
a4a5f7a
Substrait consumer build fix
@jvanstraten
Fix lint violation
b7a2a1e
@westonpace
If we build protobuf from source we always build it statically. We ne…
90d7ae3
…ed to reset the USE_STATIC_LIBS flag so that downstream components know it was built statically even if a shared library was requested.
@jvanstraten
Merge pull request#3from westonpace/substrait-consumer-build-fix-2
fe1f784
If we build protobuf from source we always build it statically. We n…
@ursabot

ursabot commented Feb 16, 2022•
edited
Loading

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = f8689a1 and contender = a935c81. a935c81 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Failed] ec2-t3-xlarge-us-east-2
[Failed] test-mac-arm
[Failed] ursa-i9-9960x
[Failed] 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

@jacques-njacques-n mentioned this pull request Mar 28, 2022
Open
marin-ma pushed a commit to marin-ma/arrow-1 that referenced this pull request Mar 28, 2022
@bkietz@jvanstraten
@westonpace@marin-ma
ARROW-15238: [C++] ARROW_ENGINE module with substrait consumer
86efe2b
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>
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>
@asfimportasfimport mentioned this pull request Feb 17, 2022
Closed
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: FlightRPC

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jvanstraten@westonpace@ursabot@bkietz
, '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-15238: [C++] ARROW_ENGINE module with substrait consumer - #12279

Closed
jvanstraten wants to merge 116 commits into
apache:masterfrom
jvanstraten:substrait-consumer
Closed

ARROW-15238: [C++] ARROW_ENGINE module with substrait consumer#12279
jvanstraten wants to merge 116 commits into
apache:masterfrom
jvanstraten:substrait-consumer

Conversation

@jvanstraten

@jvanstratenjvanstraten commented Jan 27, 2022

Copy link
Copy Markdown
Contributor

Continuation of #11707. I'm taking over from @bkietz for now because he's unavailable right now for personal reasons.

bkietzand others added 30 commits January 20, 2022 20:34
@bkietz
first pass at using substrait protobufs
ff51e35
@bkietz
add conversion of types and a basic roundtrip test
906aacf
@bkietz
reorganize to engine/substrait/{proto}_internal.cc,h
5fc6c34
@bkietz
add basic literal conversions
151cf72
@bkietz
don't rely on Datum::type in DataEq matcher
1cf679b
@bkietz
add substrait_gen_verify, allow configurable substrait repo and tag
6dce17b
@bkietz
Expose NamedStruct<=>Schema serde
53c7c7d
@bkietz
add if_else <-> IfThen conversion
cd919f3
@bkietz
rebase, catch up to changes in substrait
c079359
@bkietz
finish catching up with substrait's new field references
5c69fb1
@bkietz
add SubstraitFromJSON
2a77d28
@bkietz
port more tests to JSON
a5c25de
@bkietz
add more DataEq matchers
873ec23
@bkietz
use Date64 for substrait::date
05be909
@bkietz
add extension types for interval_*, support deeply nested struct fiel…
7309964
…d names
@bkietz
add basic sketch of ExtensionSet for tracking substrait extensions
27af6b6
@bkietz
get a failing test for arrow::null
37a62de
@bkietz
refactor extension types to index variations alongside types
8397ee5
@bkietz
use an ExtensionSet-local registry
61feb19
@bkietz
add ExtensionSet <-> Plan factories
1eb4bc0
@bkietz
pre merge stash
e63da1e
@bkietz
post rebase cleanup
f37084a
@jvanstraten@bkietz
Changes needed to get branch to compile
2725ed7
Using `--preset ninja-debug`. Build will fail on initially on GCC 11+
due to a use of undefined error from googletest; lower version is needed
to avoid hundreds of instances of an ambiguous print overload, but that
version doesn't have googletest PR 3024 included yet. This change needs
to be made manually in the googletest build tree as downloaded by cmake.
@jvanstraten@bkietz
Fix uninitialized pointers
11e7f3f
@jvanstraten@bkietz
Fix FieldRef/StructField order
96727e1
@bkietz
gcc: use a pointer to the properties tuple instead of a reference
07b259c
@bkietz
advance substrait version
885836a
@bkietz
make JSON utils public, add CheckMessagesEquivalent()
0b94f3a
@jvanstraten@bkietz
Revert now unnecessary part of9255fb6
98c74a8
@jvanstraten@bkietz
Support nested StructFields
6fd2e73
westonpace
westonpace reviewed Feb 5, 2022
View reviewed changes
Comment threadcpp/src/arrow/engine/substrait/relation_internal.cc
westonpace
westonpace reviewed Feb 5, 2022
View reviewed changes
Comment threadcpp/src/arrow/engine/substrait/serde.h Outdated
westonpace
westonpace approved these changes Feb 8, 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.

@jvanstraten and I are going to review this in person tomorrow but at the moment I think most of the major concerns have been addressed.

Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc Outdated
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc Outdated
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
@westonpace

westonpace commented Feb 8, 2022

Copy link
Copy Markdown
Member

Ah, that appveyor Windows error looks like it might be legitimate. Can you investigate that?

jvanstratenand others added 14 commits February 8, 2022 16:54
@jvanstraten
Remove unused test helper function
709ab65
@jvanstraten
Add test for round-tripping Substrait Map<K,V>, and fix implementation
c8e93ea
@westonpace
Added doc strings for SimpleExtensionType
fc9d1a9
@westonpace
Made a simplification pass on the extension set. I added some comment…
8bd00a2
…s explaining the behavior and hid more of the underlying implementation which will help future-proof us in case we move to a map-based version in the future. Fixed a bug in execution plan creation where we were referencing the Substrait function name instead of the Arrow function name.
@jvanstraten
Remove questionable SubstraitToJson & SubstraitFromJson test helper f…
2156716
…unctions
@westonpace
Removing unneeded protobuf cmake flag
f5483f4
@westonpace
Fix up the substrait consumption example to take in a filename instea…
1b8ef91
…d of use hard coded filenames. If no filename is specified we return 0 which is needed because CI will call the examples and we cannot fail. Removed the filter from the example expression because it triggers ARROW-15658 and we need to resolve ARROW-15586 first anyways. Added logic in the scanner to provide a default projection when none is specified.
@westonpace@jvanstraten
Update cpp/src/arrow/engine/substrait/extension_set.h
78d0fca
Co-authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
@westonpace
Cleaned up comments per PR review
2b0131c
@jvanstraten
Merge pull request#1from westonpace/substrait-consumer
cde0b6f
Documented SimpleExtensionType and simplified ExtensionSet
@jvanstraten
Merge pull request#2from westonpace/substrait-consumer-build-fix
a4a5f7a
Substrait consumer build fix
@jvanstraten
Fix lint violation
b7a2a1e
@westonpace
If we build protobuf from source we always build it statically. We ne…
90d7ae3
…ed to reset the USE_STATIC_LIBS flag so that downstream components know it was built statically even if a shared library was requested.
@jvanstraten
Merge pull request#3from westonpace/substrait-consumer-build-fix-2
fe1f784
If we build protobuf from source we always build it statically. We n…
@ursabot

ursabot commented Feb 16, 2022•
edited
Loading

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = f8689a1 and contender = a935c81. a935c81 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Failed] ec2-t3-xlarge-us-east-2
[Failed] test-mac-arm
[Failed] ursa-i9-9960x
[Failed] 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

@jacques-njacques-n mentioned this pull request Mar 28, 2022
Open
marin-ma pushed a commit to marin-ma/arrow-1 that referenced this pull request Mar 28, 2022
@bkietz@jvanstraten
@westonpace@marin-ma
ARROW-15238: [C++] ARROW_ENGINE module with substrait consumer
86efe2b
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>
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>
@asfimportasfimport mentioned this pull request Feb 17, 2022
Closed
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: FlightRPC

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jvanstraten@westonpace@ursabot@bkietz
, '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-15238: [C++] ARROW_ENGINE module with substrait consumer - #12279

Closed
jvanstraten wants to merge 116 commits into
apache:masterfrom
jvanstraten:substrait-consumer
Closed

ARROW-15238: [C++] ARROW_ENGINE module with substrait consumer#12279
jvanstraten wants to merge 116 commits into
apache:masterfrom
jvanstraten:substrait-consumer

Conversation

@jvanstraten

@jvanstratenjvanstraten commented Jan 27, 2022

Copy link
Copy Markdown
Contributor

Continuation of #11707. I'm taking over from @bkietz for now because he's unavailable right now for personal reasons.

bkietzand others added 30 commits January 20, 2022 20:34
@bkietz
first pass at using substrait protobufs
ff51e35
@bkietz
add conversion of types and a basic roundtrip test
906aacf
@bkietz
reorganize to engine/substrait/{proto}_internal.cc,h
5fc6c34
@bkietz
add basic literal conversions
151cf72
@bkietz
don't rely on Datum::type in DataEq matcher
1cf679b
@bkietz
add substrait_gen_verify, allow configurable substrait repo and tag
6dce17b
@bkietz
Expose NamedStruct<=>Schema serde
53c7c7d
@bkietz
add if_else <-> IfThen conversion
cd919f3
@bkietz
rebase, catch up to changes in substrait
c079359
@bkietz
finish catching up with substrait's new field references
5c69fb1
@bkietz
add SubstraitFromJSON
2a77d28
@bkietz
port more tests to JSON
a5c25de
@bkietz
add more DataEq matchers
873ec23
@bkietz
use Date64 for substrait::date
05be909
@bkietz
add extension types for interval_*, support deeply nested struct fiel…
7309964
…d names
@bkietz
add basic sketch of ExtensionSet for tracking substrait extensions
27af6b6
@bkietz
get a failing test for arrow::null
37a62de
@bkietz
refactor extension types to index variations alongside types
8397ee5
@bkietz
use an ExtensionSet-local registry
61feb19
@bkietz
add ExtensionSet <-> Plan factories
1eb4bc0
@bkietz
pre merge stash
e63da1e
@bkietz
post rebase cleanup
f37084a
@jvanstraten@bkietz
Changes needed to get branch to compile
2725ed7
Using `--preset ninja-debug`. Build will fail on initially on GCC 11+
due to a use of undefined error from googletest; lower version is needed
to avoid hundreds of instances of an ambiguous print overload, but that
version doesn't have googletest PR 3024 included yet. This change needs
to be made manually in the googletest build tree as downloaded by cmake.
@jvanstraten@bkietz
Fix uninitialized pointers
11e7f3f
@jvanstraten@bkietz
Fix FieldRef/StructField order
96727e1
@bkietz
gcc: use a pointer to the properties tuple instead of a reference
07b259c
@bkietz
advance substrait version
885836a
@bkietz
make JSON utils public, add CheckMessagesEquivalent()
0b94f3a
@jvanstraten@bkietz
Revert now unnecessary part of9255fb6
98c74a8
@jvanstraten@bkietz
Support nested StructFields
6fd2e73
westonpace
westonpace reviewed Feb 5, 2022
View reviewed changes
Comment threadcpp/src/arrow/engine/substrait/relation_internal.cc
westonpace
westonpace reviewed Feb 5, 2022
View reviewed changes
Comment threadcpp/src/arrow/engine/substrait/serde.h Outdated
westonpace
westonpace approved these changes Feb 8, 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.

@jvanstraten and I are going to review this in person tomorrow but at the moment I think most of the major concerns have been addressed.

Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc Outdated
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc Outdated
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
@westonpace

westonpace commented Feb 8, 2022

Copy link
Copy Markdown
Member

Ah, that appveyor Windows error looks like it might be legitimate. Can you investigate that?

jvanstratenand others added 14 commits February 8, 2022 16:54
@jvanstraten
Remove unused test helper function
709ab65
@jvanstraten
Add test for round-tripping Substrait Map<K,V>, and fix implementation
c8e93ea
@westonpace
Added doc strings for SimpleExtensionType
fc9d1a9
@westonpace
Made a simplification pass on the extension set. I added some comment…
8bd00a2
…s explaining the behavior and hid more of the underlying implementation which will help future-proof us in case we move to a map-based version in the future. Fixed a bug in execution plan creation where we were referencing the Substrait function name instead of the Arrow function name.
@jvanstraten
Remove questionable SubstraitToJson & SubstraitFromJson test helper f…
2156716
…unctions
@westonpace
Removing unneeded protobuf cmake flag
f5483f4
@westonpace
Fix up the substrait consumption example to take in a filename instea…
1b8ef91
…d of use hard coded filenames. If no filename is specified we return 0 which is needed because CI will call the examples and we cannot fail. Removed the filter from the example expression because it triggers ARROW-15658 and we need to resolve ARROW-15586 first anyways. Added logic in the scanner to provide a default projection when none is specified.
@westonpace@jvanstraten
Update cpp/src/arrow/engine/substrait/extension_set.h
78d0fca
Co-authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
@westonpace
Cleaned up comments per PR review
2b0131c
@jvanstraten
Merge pull request#1from westonpace/substrait-consumer
cde0b6f
Documented SimpleExtensionType and simplified ExtensionSet
@jvanstraten
Merge pull request#2from westonpace/substrait-consumer-build-fix
a4a5f7a
Substrait consumer build fix
@jvanstraten
Fix lint violation
b7a2a1e
@westonpace
If we build protobuf from source we always build it statically. We ne…
90d7ae3
…ed to reset the USE_STATIC_LIBS flag so that downstream components know it was built statically even if a shared library was requested.
@jvanstraten
Merge pull request#3from westonpace/substrait-consumer-build-fix-2
fe1f784
If we build protobuf from source we always build it statically. We n…
@ursabot

ursabot commented Feb 16, 2022•
edited
Loading

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = f8689a1 and contender = a935c81. a935c81 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Failed] ec2-t3-xlarge-us-east-2
[Failed] test-mac-arm
[Failed] ursa-i9-9960x
[Failed] 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

@jacques-njacques-n mentioned this pull request Mar 28, 2022
Open
marin-ma pushed a commit to marin-ma/arrow-1 that referenced this pull request Mar 28, 2022
@bkietz@jvanstraten
@westonpace@marin-ma
ARROW-15238: [C++] ARROW_ENGINE module with substrait consumer
86efe2b
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>
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>
@asfimportasfimport mentioned this pull request Feb 17, 2022
Closed
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: FlightRPC

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jvanstraten@westonpace@ursabot@bkietz
, '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-15238: [C++] ARROW_ENGINE module with substrait consumer - #12279

Closed
jvanstraten wants to merge 116 commits into
apache:masterfrom
jvanstraten:substrait-consumer
Closed

ARROW-15238: [C++] ARROW_ENGINE module with substrait consumer#12279
jvanstraten wants to merge 116 commits into
apache:masterfrom
jvanstraten:substrait-consumer

Conversation

@jvanstraten

@jvanstratenjvanstraten commented Jan 27, 2022

Copy link
Copy Markdown
Contributor

Continuation of #11707. I'm taking over from @bkietz for now because he's unavailable right now for personal reasons.

bkietzand others added 30 commits January 20, 2022 20:34
@bkietz
first pass at using substrait protobufs
ff51e35
@bkietz
add conversion of types and a basic roundtrip test
906aacf
@bkietz
reorganize to engine/substrait/{proto}_internal.cc,h
5fc6c34
@bkietz
add basic literal conversions
151cf72
@bkietz
don't rely on Datum::type in DataEq matcher
1cf679b
@bkietz
add substrait_gen_verify, allow configurable substrait repo and tag
6dce17b
@bkietz
Expose NamedStruct<=>Schema serde
53c7c7d
@bkietz
add if_else <-> IfThen conversion
cd919f3
@bkietz
rebase, catch up to changes in substrait
c079359
@bkietz
finish catching up with substrait's new field references
5c69fb1
@bkietz
add SubstraitFromJSON
2a77d28
@bkietz
port more tests to JSON
a5c25de
@bkietz
add more DataEq matchers
873ec23
@bkietz
use Date64 for substrait::date
05be909
@bkietz
add extension types for interval_*, support deeply nested struct fiel…
7309964
…d names
@bkietz
add basic sketch of ExtensionSet for tracking substrait extensions
27af6b6
@bkietz
get a failing test for arrow::null
37a62de
@bkietz
refactor extension types to index variations alongside types
8397ee5
@bkietz
use an ExtensionSet-local registry
61feb19
@bkietz
add ExtensionSet <-> Plan factories
1eb4bc0
@bkietz
pre merge stash
e63da1e
@bkietz
post rebase cleanup
f37084a
@jvanstraten@bkietz
Changes needed to get branch to compile
2725ed7
Using `--preset ninja-debug`. Build will fail on initially on GCC 11+
due to a use of undefined error from googletest; lower version is needed
to avoid hundreds of instances of an ambiguous print overload, but that
version doesn't have googletest PR 3024 included yet. This change needs
to be made manually in the googletest build tree as downloaded by cmake.
@jvanstraten@bkietz
Fix uninitialized pointers
11e7f3f
@jvanstraten@bkietz
Fix FieldRef/StructField order
96727e1
@bkietz
gcc: use a pointer to the properties tuple instead of a reference
07b259c
@bkietz
advance substrait version
885836a
@bkietz
make JSON utils public, add CheckMessagesEquivalent()
0b94f3a
@jvanstraten@bkietz
Revert now unnecessary part of9255fb6
98c74a8
@jvanstraten@bkietz
Support nested StructFields
6fd2e73
westonpace
westonpace reviewed Feb 5, 2022
View reviewed changes
Comment threadcpp/src/arrow/engine/substrait/relation_internal.cc
westonpace
westonpace reviewed Feb 5, 2022
View reviewed changes
Comment threadcpp/src/arrow/engine/substrait/serde.h Outdated
westonpace
westonpace approved these changes Feb 8, 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.

@jvanstraten and I are going to review this in person tomorrow but at the moment I think most of the major concerns have been addressed.

Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc Outdated
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc Outdated
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
@westonpace

westonpace commented Feb 8, 2022

Copy link
Copy Markdown
Member

Ah, that appveyor Windows error looks like it might be legitimate. Can you investigate that?

jvanstratenand others added 14 commits February 8, 2022 16:54
@jvanstraten
Remove unused test helper function
709ab65
@jvanstraten
Add test for round-tripping Substrait Map<K,V>, and fix implementation
c8e93ea
@westonpace
Added doc strings for SimpleExtensionType
fc9d1a9
@westonpace
Made a simplification pass on the extension set. I added some comment…
8bd00a2
…s explaining the behavior and hid more of the underlying implementation which will help future-proof us in case we move to a map-based version in the future. Fixed a bug in execution plan creation where we were referencing the Substrait function name instead of the Arrow function name.
@jvanstraten
Remove questionable SubstraitToJson & SubstraitFromJson test helper f…
2156716
…unctions
@westonpace
Removing unneeded protobuf cmake flag
f5483f4
@westonpace
Fix up the substrait consumption example to take in a filename instea…
1b8ef91
…d of use hard coded filenames. If no filename is specified we return 0 which is needed because CI will call the examples and we cannot fail. Removed the filter from the example expression because it triggers ARROW-15658 and we need to resolve ARROW-15586 first anyways. Added logic in the scanner to provide a default projection when none is specified.
@westonpace@jvanstraten
Update cpp/src/arrow/engine/substrait/extension_set.h
78d0fca
Co-authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
@westonpace
Cleaned up comments per PR review
2b0131c
@jvanstraten
Merge pull request#1from westonpace/substrait-consumer
cde0b6f
Documented SimpleExtensionType and simplified ExtensionSet
@jvanstraten
Merge pull request#2from westonpace/substrait-consumer-build-fix
a4a5f7a
Substrait consumer build fix
@jvanstraten
Fix lint violation
b7a2a1e
@westonpace
If we build protobuf from source we always build it statically. We ne…
90d7ae3
…ed to reset the USE_STATIC_LIBS flag so that downstream components know it was built statically even if a shared library was requested.
@jvanstraten
Merge pull request#3from westonpace/substrait-consumer-build-fix-2
fe1f784
If we build protobuf from source we always build it statically. We n…
@ursabot

ursabot commented Feb 16, 2022•
edited
Loading

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = f8689a1 and contender = a935c81. a935c81 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Failed] ec2-t3-xlarge-us-east-2
[Failed] test-mac-arm
[Failed] ursa-i9-9960x
[Failed] 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

@jacques-njacques-n mentioned this pull request Mar 28, 2022
Open
marin-ma pushed a commit to marin-ma/arrow-1 that referenced this pull request Mar 28, 2022
@bkietz@jvanstraten
@westonpace@marin-ma
ARROW-15238: [C++] ARROW_ENGINE module with substrait consumer
86efe2b
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>
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>
@asfimportasfimport mentioned this pull request Feb 17, 2022
Closed
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: FlightRPC

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jvanstraten@westonpace@ursabot@bkietz
, '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-15238: [C++] ARROW_ENGINE module with substrait consumer - #12279

Closed
jvanstraten wants to merge 116 commits into
apache:masterfrom
jvanstraten:substrait-consumer
Closed

ARROW-15238: [C++] ARROW_ENGINE module with substrait consumer#12279
jvanstraten wants to merge 116 commits into
apache:masterfrom
jvanstraten:substrait-consumer

Conversation

@jvanstraten

@jvanstratenjvanstraten commented Jan 27, 2022

Copy link
Copy Markdown
Contributor

Continuation of #11707. I'm taking over from @bkietz for now because he's unavailable right now for personal reasons.

bkietzand others added 30 commits January 20, 2022 20:34
@bkietz
first pass at using substrait protobufs
ff51e35
@bkietz
add conversion of types and a basic roundtrip test
906aacf
@bkietz
reorganize to engine/substrait/{proto}_internal.cc,h
5fc6c34
@bkietz
add basic literal conversions
151cf72
@bkietz
don't rely on Datum::type in DataEq matcher
1cf679b
@bkietz
add substrait_gen_verify, allow configurable substrait repo and tag
6dce17b
@bkietz
Expose NamedStruct<=>Schema serde
53c7c7d
@bkietz
add if_else <-> IfThen conversion
cd919f3
@bkietz
rebase, catch up to changes in substrait
c079359
@bkietz
finish catching up with substrait's new field references
5c69fb1
@bkietz
add SubstraitFromJSON
2a77d28
@bkietz
port more tests to JSON
a5c25de
@bkietz
add more DataEq matchers
873ec23
@bkietz
use Date64 for substrait::date
05be909
@bkietz
add extension types for interval_*, support deeply nested struct fiel…
7309964
…d names
@bkietz
add basic sketch of ExtensionSet for tracking substrait extensions
27af6b6
@bkietz
get a failing test for arrow::null
37a62de
@bkietz
refactor extension types to index variations alongside types
8397ee5
@bkietz
use an ExtensionSet-local registry
61feb19
@bkietz
add ExtensionSet <-> Plan factories
1eb4bc0
@bkietz
pre merge stash
e63da1e
@bkietz
post rebase cleanup
f37084a
@jvanstraten@bkietz
Changes needed to get branch to compile
2725ed7
Using `--preset ninja-debug`. Build will fail on initially on GCC 11+
due to a use of undefined error from googletest; lower version is needed
to avoid hundreds of instances of an ambiguous print overload, but that
version doesn't have googletest PR 3024 included yet. This change needs
to be made manually in the googletest build tree as downloaded by cmake.
@jvanstraten@bkietz
Fix uninitialized pointers
11e7f3f
@jvanstraten@bkietz
Fix FieldRef/StructField order
96727e1
@bkietz
gcc: use a pointer to the properties tuple instead of a reference
07b259c
@bkietz
advance substrait version
885836a
@bkietz
make JSON utils public, add CheckMessagesEquivalent()
0b94f3a
@jvanstraten@bkietz
Revert now unnecessary part of9255fb6
98c74a8
@jvanstraten@bkietz
Support nested StructFields
6fd2e73
westonpace
westonpace reviewed Feb 5, 2022
View reviewed changes
Comment threadcpp/src/arrow/engine/substrait/relation_internal.cc
westonpace
westonpace reviewed Feb 5, 2022
View reviewed changes
Comment threadcpp/src/arrow/engine/substrait/serde.h Outdated
westonpace
westonpace approved these changes Feb 8, 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.

@jvanstraten and I are going to review this in person tomorrow but at the moment I think most of the major concerns have been addressed.

Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc Outdated
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc Outdated
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
@westonpace

westonpace commented Feb 8, 2022

Copy link
Copy Markdown
Member

Ah, that appveyor Windows error looks like it might be legitimate. Can you investigate that?

jvanstratenand others added 14 commits February 8, 2022 16:54
@jvanstraten
Remove unused test helper function
709ab65
@jvanstraten
Add test for round-tripping Substrait Map<K,V>, and fix implementation
c8e93ea
@westonpace
Added doc strings for SimpleExtensionType
fc9d1a9
@westonpace
Made a simplification pass on the extension set. I added some comment…
8bd00a2
…s explaining the behavior and hid more of the underlying implementation which will help future-proof us in case we move to a map-based version in the future. Fixed a bug in execution plan creation where we were referencing the Substrait function name instead of the Arrow function name.
@jvanstraten
Remove questionable SubstraitToJson & SubstraitFromJson test helper f…
2156716
…unctions
@westonpace
Removing unneeded protobuf cmake flag
f5483f4
@westonpace
Fix up the substrait consumption example to take in a filename instea…
1b8ef91
…d of use hard coded filenames. If no filename is specified we return 0 which is needed because CI will call the examples and we cannot fail. Removed the filter from the example expression because it triggers ARROW-15658 and we need to resolve ARROW-15586 first anyways. Added logic in the scanner to provide a default projection when none is specified.
@westonpace@jvanstraten
Update cpp/src/arrow/engine/substrait/extension_set.h
78d0fca
Co-authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
@westonpace
Cleaned up comments per PR review
2b0131c
@jvanstraten
Merge pull request#1from westonpace/substrait-consumer
cde0b6f
Documented SimpleExtensionType and simplified ExtensionSet
@jvanstraten
Merge pull request#2from westonpace/substrait-consumer-build-fix
a4a5f7a
Substrait consumer build fix
@jvanstraten
Fix lint violation
b7a2a1e
@westonpace
If we build protobuf from source we always build it statically. We ne…
90d7ae3
…ed to reset the USE_STATIC_LIBS flag so that downstream components know it was built statically even if a shared library was requested.
@jvanstraten
Merge pull request#3from westonpace/substrait-consumer-build-fix-2
fe1f784
If we build protobuf from source we always build it statically. We n…
@ursabot

ursabot commented Feb 16, 2022•
edited
Loading

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = f8689a1 and contender = a935c81. a935c81 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Failed] ec2-t3-xlarge-us-east-2
[Failed] test-mac-arm
[Failed] ursa-i9-9960x
[Failed] 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

@jacques-njacques-n mentioned this pull request Mar 28, 2022
Open
marin-ma pushed a commit to marin-ma/arrow-1 that referenced this pull request Mar 28, 2022
@bkietz@jvanstraten
@westonpace@marin-ma
ARROW-15238: [C++] ARROW_ENGINE module with substrait consumer
86efe2b
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>
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>
@asfimportasfimport mentioned this pull request Feb 17, 2022
Closed
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: FlightRPC

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jvanstraten@westonpace@ursabot@bkietz
, '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-15238: [C++] ARROW_ENGINE module with substrait consumer - #12279

Closed
jvanstraten wants to merge 116 commits into
apache:masterfrom
jvanstraten:substrait-consumer
Closed

ARROW-15238: [C++] ARROW_ENGINE module with substrait consumer#12279
jvanstraten wants to merge 116 commits into
apache:masterfrom
jvanstraten:substrait-consumer

Conversation

@jvanstraten

@jvanstratenjvanstraten commented Jan 27, 2022

Copy link
Copy Markdown
Contributor

Continuation of #11707. I'm taking over from @bkietz for now because he's unavailable right now for personal reasons.

bkietzand others added 30 commits January 20, 2022 20:34
@bkietz
first pass at using substrait protobufs
ff51e35
@bkietz
add conversion of types and a basic roundtrip test
906aacf
@bkietz
reorganize to engine/substrait/{proto}_internal.cc,h
5fc6c34
@bkietz
add basic literal conversions
151cf72
@bkietz
don't rely on Datum::type in DataEq matcher
1cf679b
@bkietz
add substrait_gen_verify, allow configurable substrait repo and tag
6dce17b
@bkietz
Expose NamedStruct<=>Schema serde
53c7c7d
@bkietz
add if_else <-> IfThen conversion
cd919f3
@bkietz
rebase, catch up to changes in substrait
c079359
@bkietz
finish catching up with substrait's new field references
5c69fb1
@bkietz
add SubstraitFromJSON
2a77d28
@bkietz
port more tests to JSON
a5c25de
@bkietz
add more DataEq matchers
873ec23
@bkietz
use Date64 for substrait::date
05be909
@bkietz
add extension types for interval_*, support deeply nested struct fiel…
7309964
…d names
@bkietz
add basic sketch of ExtensionSet for tracking substrait extensions
27af6b6
@bkietz
get a failing test for arrow::null
37a62de
@bkietz
refactor extension types to index variations alongside types
8397ee5
@bkietz
use an ExtensionSet-local registry
61feb19
@bkietz
add ExtensionSet <-> Plan factories
1eb4bc0
@bkietz
pre merge stash
e63da1e
@bkietz
post rebase cleanup
f37084a
@jvanstraten@bkietz
Changes needed to get branch to compile
2725ed7
Using `--preset ninja-debug`. Build will fail on initially on GCC 11+
due to a use of undefined error from googletest; lower version is needed
to avoid hundreds of instances of an ambiguous print overload, but that
version doesn't have googletest PR 3024 included yet. This change needs
to be made manually in the googletest build tree as downloaded by cmake.
@jvanstraten@bkietz
Fix uninitialized pointers
11e7f3f
@jvanstraten@bkietz
Fix FieldRef/StructField order
96727e1
@bkietz
gcc: use a pointer to the properties tuple instead of a reference
07b259c
@bkietz
advance substrait version
885836a
@bkietz
make JSON utils public, add CheckMessagesEquivalent()
0b94f3a
@jvanstraten@bkietz
Revert now unnecessary part of9255fb6
98c74a8
@jvanstraten@bkietz
Support nested StructFields
6fd2e73
westonpace
westonpace reviewed Feb 5, 2022
View reviewed changes
Comment threadcpp/src/arrow/engine/substrait/relation_internal.cc
westonpace
westonpace reviewed Feb 5, 2022
View reviewed changes
Comment threadcpp/src/arrow/engine/substrait/serde.h Outdated
westonpace
westonpace approved these changes Feb 8, 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.

@jvanstraten and I are going to review this in person tomorrow but at the moment I think most of the major concerns have been addressed.

Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc Outdated
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc Outdated
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
@westonpace

westonpace commented Feb 8, 2022

Copy link
Copy Markdown
Member

Ah, that appveyor Windows error looks like it might be legitimate. Can you investigate that?

jvanstratenand others added 14 commits February 8, 2022 16:54
@jvanstraten
Remove unused test helper function
709ab65
@jvanstraten
Add test for round-tripping Substrait Map<K,V>, and fix implementation
c8e93ea
@westonpace
Added doc strings for SimpleExtensionType
fc9d1a9
@westonpace
Made a simplification pass on the extension set. I added some comment…
8bd00a2
…s explaining the behavior and hid more of the underlying implementation which will help future-proof us in case we move to a map-based version in the future. Fixed a bug in execution plan creation where we were referencing the Substrait function name instead of the Arrow function name.
@jvanstraten
Remove questionable SubstraitToJson & SubstraitFromJson test helper f…
2156716
…unctions
@westonpace
Removing unneeded protobuf cmake flag
f5483f4
@westonpace
Fix up the substrait consumption example to take in a filename instea…
1b8ef91
…d of use hard coded filenames. If no filename is specified we return 0 which is needed because CI will call the examples and we cannot fail. Removed the filter from the example expression because it triggers ARROW-15658 and we need to resolve ARROW-15586 first anyways. Added logic in the scanner to provide a default projection when none is specified.
@westonpace@jvanstraten
Update cpp/src/arrow/engine/substrait/extension_set.h
78d0fca
Co-authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
@westonpace
Cleaned up comments per PR review
2b0131c
@jvanstraten
Merge pull request#1from westonpace/substrait-consumer
cde0b6f
Documented SimpleExtensionType and simplified ExtensionSet
@jvanstraten
Merge pull request#2from westonpace/substrait-consumer-build-fix
a4a5f7a
Substrait consumer build fix
@jvanstraten
Fix lint violation
b7a2a1e
@westonpace
If we build protobuf from source we always build it statically. We ne…
90d7ae3
…ed to reset the USE_STATIC_LIBS flag so that downstream components know it was built statically even if a shared library was requested.
@jvanstraten
Merge pull request#3from westonpace/substrait-consumer-build-fix-2
fe1f784
If we build protobuf from source we always build it statically. We n…
@ursabot

ursabot commented Feb 16, 2022•
edited
Loading

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = f8689a1 and contender = a935c81. a935c81 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Failed] ec2-t3-xlarge-us-east-2
[Failed] test-mac-arm
[Failed] ursa-i9-9960x
[Failed] 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

@jacques-njacques-n mentioned this pull request Mar 28, 2022
Open
marin-ma pushed a commit to marin-ma/arrow-1 that referenced this pull request Mar 28, 2022
@bkietz@jvanstraten
@westonpace@marin-ma
ARROW-15238: [C++] ARROW_ENGINE module with substrait consumer
86efe2b
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>
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>
@asfimportasfimport mentioned this pull request Feb 17, 2022
Closed
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: FlightRPC

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jvanstraten@westonpace@ursabot@bkietz
, '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-15238: [C++] ARROW_ENGINE module with substrait consumer - #12279

Closed
jvanstraten wants to merge 116 commits into
apache:masterfrom
jvanstraten:substrait-consumer
Closed

ARROW-15238: [C++] ARROW_ENGINE module with substrait consumer#12279
jvanstraten wants to merge 116 commits into
apache:masterfrom
jvanstraten:substrait-consumer

Conversation

@jvanstraten

@jvanstratenjvanstraten commented Jan 27, 2022

Copy link
Copy Markdown
Contributor

Continuation of #11707. I'm taking over from @bkietz for now because he's unavailable right now for personal reasons.

bkietzand others added 30 commits January 20, 2022 20:34
@bkietz
first pass at using substrait protobufs
ff51e35
@bkietz
add conversion of types and a basic roundtrip test
906aacf
@bkietz
reorganize to engine/substrait/{proto}_internal.cc,h
5fc6c34
@bkietz
add basic literal conversions
151cf72
@bkietz
don't rely on Datum::type in DataEq matcher
1cf679b
@bkietz
add substrait_gen_verify, allow configurable substrait repo and tag
6dce17b
@bkietz
Expose NamedStruct<=>Schema serde
53c7c7d
@bkietz
add if_else <-> IfThen conversion
cd919f3
@bkietz
rebase, catch up to changes in substrait
c079359
@bkietz
finish catching up with substrait's new field references
5c69fb1
@bkietz
add SubstraitFromJSON
2a77d28
@bkietz
port more tests to JSON
a5c25de
@bkietz
add more DataEq matchers
873ec23
@bkietz
use Date64 for substrait::date
05be909
@bkietz
add extension types for interval_*, support deeply nested struct fiel…
7309964
…d names
@bkietz
add basic sketch of ExtensionSet for tracking substrait extensions
27af6b6
@bkietz
get a failing test for arrow::null
37a62de
@bkietz
refactor extension types to index variations alongside types
8397ee5
@bkietz
use an ExtensionSet-local registry
61feb19
@bkietz
add ExtensionSet <-> Plan factories
1eb4bc0
@bkietz
pre merge stash
e63da1e
@bkietz
post rebase cleanup
f37084a
@jvanstraten@bkietz
Changes needed to get branch to compile
2725ed7
Using `--preset ninja-debug`. Build will fail on initially on GCC 11+
due to a use of undefined error from googletest; lower version is needed
to avoid hundreds of instances of an ambiguous print overload, but that
version doesn't have googletest PR 3024 included yet. This change needs
to be made manually in the googletest build tree as downloaded by cmake.
@jvanstraten@bkietz
Fix uninitialized pointers
11e7f3f
@jvanstraten@bkietz
Fix FieldRef/StructField order
96727e1
@bkietz
gcc: use a pointer to the properties tuple instead of a reference
07b259c
@bkietz
advance substrait version
885836a
@bkietz
make JSON utils public, add CheckMessagesEquivalent()
0b94f3a
@jvanstraten@bkietz
Revert now unnecessary part of9255fb6
98c74a8
@jvanstraten@bkietz
Support nested StructFields
6fd2e73
westonpace
westonpace reviewed Feb 5, 2022
View reviewed changes
Comment threadcpp/src/arrow/engine/substrait/relation_internal.cc
westonpace
westonpace reviewed Feb 5, 2022
View reviewed changes
Comment threadcpp/src/arrow/engine/substrait/serde.h Outdated
westonpace
westonpace approved these changes Feb 8, 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.

@jvanstraten and I are going to review this in person tomorrow but at the moment I think most of the major concerns have been addressed.

Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc Outdated
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc Outdated
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
@westonpace

westonpace commented Feb 8, 2022

Copy link
Copy Markdown
Member

Ah, that appveyor Windows error looks like it might be legitimate. Can you investigate that?

jvanstratenand others added 14 commits February 8, 2022 16:54
@jvanstraten
Remove unused test helper function
709ab65
@jvanstraten
Add test for round-tripping Substrait Map<K,V>, and fix implementation
c8e93ea
@westonpace
Added doc strings for SimpleExtensionType
fc9d1a9
@westonpace
Made a simplification pass on the extension set. I added some comment…
8bd00a2
…s explaining the behavior and hid more of the underlying implementation which will help future-proof us in case we move to a map-based version in the future. Fixed a bug in execution plan creation where we were referencing the Substrait function name instead of the Arrow function name.
@jvanstraten
Remove questionable SubstraitToJson & SubstraitFromJson test helper f…
2156716
…unctions
@westonpace
Removing unneeded protobuf cmake flag
f5483f4
@westonpace
Fix up the substrait consumption example to take in a filename instea…
1b8ef91
…d of use hard coded filenames. If no filename is specified we return 0 which is needed because CI will call the examples and we cannot fail. Removed the filter from the example expression because it triggers ARROW-15658 and we need to resolve ARROW-15586 first anyways. Added logic in the scanner to provide a default projection when none is specified.
@westonpace@jvanstraten
Update cpp/src/arrow/engine/substrait/extension_set.h
78d0fca
Co-authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
@westonpace
Cleaned up comments per PR review
2b0131c
@jvanstraten
Merge pull request#1from westonpace/substrait-consumer
cde0b6f
Documented SimpleExtensionType and simplified ExtensionSet
@jvanstraten
Merge pull request#2from westonpace/substrait-consumer-build-fix
a4a5f7a
Substrait consumer build fix
@jvanstraten
Fix lint violation
b7a2a1e
@westonpace
If we build protobuf from source we always build it statically. We ne…
90d7ae3
…ed to reset the USE_STATIC_LIBS flag so that downstream components know it was built statically even if a shared library was requested.
@jvanstraten
Merge pull request#3from westonpace/substrait-consumer-build-fix-2
fe1f784
If we build protobuf from source we always build it statically. We n…
@ursabot

ursabot commented Feb 16, 2022•
edited
Loading

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = f8689a1 and contender = a935c81. a935c81 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Failed] ec2-t3-xlarge-us-east-2
[Failed] test-mac-arm
[Failed] ursa-i9-9960x
[Failed] 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

@jacques-njacques-n mentioned this pull request Mar 28, 2022
Open
marin-ma pushed a commit to marin-ma/arrow-1 that referenced this pull request Mar 28, 2022
@bkietz@jvanstraten
@westonpace@marin-ma
ARROW-15238: [C++] ARROW_ENGINE module with substrait consumer
86efe2b
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>
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>
@asfimportasfimport mentioned this pull request Feb 17, 2022
Closed
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: FlightRPC

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jvanstraten@westonpace@ursabot@bkietz
, '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-15238: [C++] ARROW_ENGINE module with substrait consumer - #12279

Closed
jvanstraten wants to merge 116 commits into
apache:masterfrom
jvanstraten:substrait-consumer
Closed

ARROW-15238: [C++] ARROW_ENGINE module with substrait consumer#12279
jvanstraten wants to merge 116 commits into
apache:masterfrom
jvanstraten:substrait-consumer

Conversation

@jvanstraten

@jvanstratenjvanstraten commented Jan 27, 2022

Copy link
Copy Markdown
Contributor

Continuation of #11707. I'm taking over from @bkietz for now because he's unavailable right now for personal reasons.

bkietzand others added 30 commits January 20, 2022 20:34
@bkietz
first pass at using substrait protobufs
ff51e35
@bkietz
add conversion of types and a basic roundtrip test
906aacf
@bkietz
reorganize to engine/substrait/{proto}_internal.cc,h
5fc6c34
@bkietz
add basic literal conversions
151cf72
@bkietz
don't rely on Datum::type in DataEq matcher
1cf679b
@bkietz
add substrait_gen_verify, allow configurable substrait repo and tag
6dce17b
@bkietz
Expose NamedStruct<=>Schema serde
53c7c7d
@bkietz
add if_else <-> IfThen conversion
cd919f3
@bkietz
rebase, catch up to changes in substrait
c079359
@bkietz
finish catching up with substrait's new field references
5c69fb1
@bkietz
add SubstraitFromJSON
2a77d28
@bkietz
port more tests to JSON
a5c25de
@bkietz
add more DataEq matchers
873ec23
@bkietz
use Date64 for substrait::date
05be909
@bkietz
add extension types for interval_*, support deeply nested struct fiel…
7309964
…d names
@bkietz
add basic sketch of ExtensionSet for tracking substrait extensions
27af6b6
@bkietz
get a failing test for arrow::null
37a62de
@bkietz
refactor extension types to index variations alongside types
8397ee5
@bkietz
use an ExtensionSet-local registry
61feb19
@bkietz
add ExtensionSet <-> Plan factories
1eb4bc0
@bkietz
pre merge stash
e63da1e
@bkietz
post rebase cleanup
f37084a
@jvanstraten@bkietz
Changes needed to get branch to compile
2725ed7
Using `--preset ninja-debug`. Build will fail on initially on GCC 11+
due to a use of undefined error from googletest; lower version is needed
to avoid hundreds of instances of an ambiguous print overload, but that
version doesn't have googletest PR 3024 included yet. This change needs
to be made manually in the googletest build tree as downloaded by cmake.
@jvanstraten@bkietz
Fix uninitialized pointers
11e7f3f
@jvanstraten@bkietz
Fix FieldRef/StructField order
96727e1
@bkietz
gcc: use a pointer to the properties tuple instead of a reference
07b259c
@bkietz
advance substrait version
885836a
@bkietz
make JSON utils public, add CheckMessagesEquivalent()
0b94f3a
@jvanstraten@bkietz
Revert now unnecessary part of9255fb6
98c74a8
@jvanstraten@bkietz
Support nested StructFields
6fd2e73
westonpace
westonpace reviewed Feb 5, 2022
View reviewed changes
Comment threadcpp/src/arrow/engine/substrait/relation_internal.cc
westonpace
westonpace reviewed Feb 5, 2022
View reviewed changes
Comment threadcpp/src/arrow/engine/substrait/serde.h Outdated
westonpace
westonpace approved these changes Feb 8, 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.

@jvanstraten and I are going to review this in person tomorrow but at the moment I think most of the major concerns have been addressed.

Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc Outdated
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc Outdated
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
Comment threadcpp/src/arrow/engine/substrait/serde_test.cc
@westonpace

westonpace commented Feb 8, 2022

Copy link
Copy Markdown
Member

Ah, that appveyor Windows error looks like it might be legitimate. Can you investigate that?

jvanstratenand others added 14 commits February 8, 2022 16:54
@jvanstraten
Remove unused test helper function
709ab65
@jvanstraten
Add test for round-tripping Substrait Map<K,V>, and fix implementation
c8e93ea
@westonpace
Added doc strings for SimpleExtensionType
fc9d1a9
@westonpace
Made a simplification pass on the extension set. I added some comment…
8bd00a2
…s explaining the behavior and hid more of the underlying implementation which will help future-proof us in case we move to a map-based version in the future. Fixed a bug in execution plan creation where we were referencing the Substrait function name instead of the Arrow function name.
@jvanstraten
Remove questionable SubstraitToJson & SubstraitFromJson test helper f…
2156716
…unctions
@westonpace
Removing unneeded protobuf cmake flag
f5483f4
@westonpace
Fix up the substrait consumption example to take in a filename instea…
1b8ef91
…d of use hard coded filenames. If no filename is specified we return 0 which is needed because CI will call the examples and we cannot fail. Removed the filter from the example expression because it triggers ARROW-15658 and we need to resolve ARROW-15586 first anyways. Added logic in the scanner to provide a default projection when none is specified.
@westonpace@jvanstraten
Update cpp/src/arrow/engine/substrait/extension_set.h
78d0fca
Co-authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com>
@westonpace
Cleaned up comments per PR review
2b0131c
@jvanstraten
Merge pull request#1from westonpace/substrait-consumer
cde0b6f
Documented SimpleExtensionType and simplified ExtensionSet
@jvanstraten
Merge pull request#2from westonpace/substrait-consumer-build-fix
a4a5f7a
Substrait consumer build fix
@jvanstraten
Fix lint violation
b7a2a1e
@westonpace
If we build protobuf from source we always build it statically. We ne…
90d7ae3
…ed to reset the USE_STATIC_LIBS flag so that downstream components know it was built statically even if a shared library was requested.
@jvanstraten
Merge pull request#3from westonpace/substrait-consumer-build-fix-2
fe1f784
If we build protobuf from source we always build it statically. We n…
@ursabot

ursabot commented Feb 16, 2022•
edited
Loading

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = f8689a1 and contender = a935c81. a935c81 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Failed] ec2-t3-xlarge-us-east-2
[Failed] test-mac-arm
[Failed] ursa-i9-9960x
[Failed] 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

@jacques-njacques-n mentioned this pull request Mar 28, 2022
Open
marin-ma pushed a commit to marin-ma/arrow-1 that referenced this pull request Mar 28, 2022
@bkietz@jvanstraten
@westonpace@marin-ma
ARROW-15238: [C++] ARROW_ENGINE module with substrait consumer
86efe2b
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>
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>
@asfimportasfimport mentioned this pull request Feb 17, 2022
Closed
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: FlightRPC

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jvanstraten@westonpace@ursabot@bkietz