ARROW-17980: [C++] As-of-Join Substrait extension by rtpsw · Pull Request #14385 · apache/arrow

rtpsw · 2022-10-12T12:46:49Z

See https://issues.apache.org/jira/browse/ARROW-17980

rtpsw · 2022-10-12T12:56:39Z

cc @icexelloss @westonpace

rtpsw · 2022-10-12T13:32:03Z

Note that this PR includes code from #14386 which is currently pending.

icexelloss · 2022-10-12T13:50:10Z

@rtpsw Why do we need #14386 for this? Can we separate out these two changes?

icexelloss · 2022-10-12T13:52:51Z

@westonpace The changes here depends on your local change in https://github.com/westonpace/arrow/tree/experiment/substrait-extension

How do you want to proceed here? We can either
(1) merge changes in experiment/substrait-extension and rebase this PR
(2) combine changes in experiment/substrait-extension and the asof join message together in this PR and review together

rtpsw · 2022-10-12T13:54:52Z

@rtpsw Why do we need #14386 for this? Can we separate out these two changes?

Technically, we don't, or no longer do. I used #14386 during test cases development of this PR. If #14386 will be rejected, we could remove its code here too. Otherwise, the code can stay here and will merge cleanly with #14386.

rtpsw · 2022-10-12T13:58:36Z

@westonpace The changes here depends on your local change in https://github.com/westonpace/arrow/tree/experiment/substrait-extension

The code here uses most of Weston's code, except for mostly that the "dummy" DelayRel was replaced with AsOfJoinRel (including corresponding test code) and that the code was refactored a bit. So, I think the code here should go in without a merge or a rebase.

github-actions · 2022-10-12T15:07:48Z

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

rtpsw · 2022-10-13T09:06:03Z

@westonpace, any idea why some jobs, like this one, failed? It seems to be complaining about missing proto classes; I don't know why the proto classes would get built in some jobs and not in others.

icexelloss · 2022-10-13T14:38:21Z


+class ARROW_ENGINE_EXPORT ExtensionProvider {
+ public:
+ static std::shared_ptr<ExtensionProvider> kDefaultExtensionProvider;


Should we define this the same way that kDefaultNamedTableProvider is defined? I don't see a reason why those two should be different

Not sure, but I think it's fine to leave as is in this PR.

icexelloss · 2022-10-13T14:41:47Z

@rtpsw I took one around of review. At the high level I think this makes lot of sense. Left some comments for refinement.

…OW-17980

rtpsw · 2022-10-15T08:41:27Z

CI failures look unrelated the changes here. @icexelloss, this may be ready for an Arrow review, WDYT?

…reads is false

rtpsw · 2022-10-19T10:13:27Z

@rtpsw Looks pretty good! There seems to be many style changes that seems orthogonal. Are those changes automatically made? Is it easy to separate out the style change from the actual change?
If we insist, we could take these style changes out to a separate PR, get that merged, and then come back here to rebase.

Anja suggested changing arrow::engine::substrait to arrow::engine::substrait_ext to avoid MSVC name conflicts with symbols in ::substrait. This should allow us to keep using substrait:: instead of ::substrait:: and avoid a large orthogonal change.

icexelloss · 2022-10-19T12:48:13Z

@rtpsw Looks pretty good! There seems to be many style changes that seems orthogonal. Are those changes automatically made? Is it easy to separate out the style change from the actual change?
If we insist, we could take these style changes out to a separate PR, get that merged, and then come back here to rebase.
Anja suggested changing arrow::engine::substrait to arrow::engine::substrait_ext to avoid MSVC name conflicts with symbols in ::substrait. This should allow us to keep using substrait:: instead of ::substrait:: and avoid a large orthogonal change.

Does arrow::engine::substrait::extension work?

rtpsw · 2022-10-19T12:52:19Z

Does arrow::engine::substrait::extension work?

My guess is this would confuse MSVC just the same because when it sees a substrait::* symbol it would look under arrow::engine::substrait and won't fall back to ::substrait.

* ARROW-15732: Lint, bugfix * ARROW-15732: Lint * ARROW-15732: Added default value to StartProducing to use CPU thread pool * ARROW-15732: Disabled broken hash-join test for now as it isn't important for our current purposes * ARROW-15732: Fix pyarrow build. Some tests still failing * ARROW-15732: Removed requirement that write dataset be run with threads * ARROW-15732: Simplified python consumption of exec plans. Should fix a number of python tests. * ARROW-15732: Lint * ARROW-15732: A test was relying on a stable sort behavior that is not present in the order by node. * ARROW-15732: Fix for unit test relying on deterministic order

…ger (apache#16)

…nts (apache#15) * ARROW-17966: Updated to latest Substrait version. Switched from optional enum args to proper options. Added check for minimum Substrait version * ARROW-17966: Add version to python substrait examples. Fix version handling to check major version and not just minor * ARROW-17966: Update cpp/src/arrow/engine/substrait/extension_set.cc Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com> * ARROW-17966: Update cpp/src/arrow/engine/substrait/extension_set.cc Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com> * ARROW-17966: Update cpp/src/arrow/engine/substrait/extension_set.cc Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com> * ARROW-17966: Update cpp/src/arrow/engine/substrait/extension_set.cc Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com> * ARROW-17966: Display the available choices when a user enters a valid substrait option that Acero doesn't support * ARROW-17966: Simplify parsing boilerplate per review comments * ARROW-17966: Gracefully error if the user does not supply any preferences for an option * ARROW-17966: Prefer range loops where possible * ARROW-17966: Rebase cleanup * ARROW-17966: Minor fix to failing unit tests: remove enum="unspecified" * ARROW-17966: Minor lint fix * ARROW-17966: Cmake format Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com>

* ARROW-17980: [C++] As-of-Join Substrait extension * add missing file * add missing proto * CI fixes * distinct keys per input table * CI fixes * resolve conflict * fix typo * ARROW-17980: Change extensions package from arrow::substrait to arrow::substrait_ext * ARROW-17980: Remove more instances of ::substrait Co-authored-by: Yaron Gvili <rtpsw@hotmail.com>

rtpsw · 2022-10-23T10:58:41Z

@rtpsw Looks pretty good! There seems to be many style changes that seems orthogonal. Are those changes automatically made? Is it easy to separate out the style change from the actual change?

@icexelloss, I managed to minimize the orthogonal change (namespace substrait = ::substrait added to a few files). I think this should be enough to make reviewing convenient.

rtpsw · 2022-10-24T10:46:05Z

The commit history got messed up. I'll try to open a fresh PR.

icexelloss · 2022-10-24T12:33:40Z

@rtpsw You don't need to open a new PR - you can just force push to your branch ARROW-17980 and fix the commit history

Replacing #14385 Lead-authored-by: Yaron Gvili <rtpsw@hotmail.com> Co-authored-by: Weston Pace <weston.pace@gmail.com> Signed-off-by: Weston Pace <weston.pace@gmail.com>

Replacing apache#14385 Lead-authored-by: Yaron Gvili <rtpsw@hotmail.com> Co-authored-by: Weston Pace <weston.pace@gmail.com> Signed-off-by: Weston Pace <weston.pace@gmail.com>

ARROW-17980: [C++] As-of-Join Substrait extension
9e9dfcd

icexelloss reviewed Oct 12, 2022
View reviewed changes

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

github-actionsBot added the Component: C++ label Oct 12, 2022

rtpsw added 3 commits October 12, 2022 17:28

add missing file
963ed7b

add missing proto
b7e647f

CI fixes
b71037e

icexelloss reviewed Oct 13, 2022
View reviewed changes

Comment threadcpp/proto/substrait/extension_rels.proto Outdated

icexelloss reviewed Oct 13, 2022
View reviewed changes

Comment threadcpp/src/arrow/compare.cc Outdated

icexelloss reviewed Oct 13, 2022
View reviewed changes

Comment threadcpp/src/arrow/compute/exec/asof_join_node.cc

icexelloss reviewed Oct 13, 2022
View reviewed changes

Comment threadcpp/src/arrow/compute/exec/asof_join_node.cc

icexelloss requested changes Oct 13, 2022
View reviewed changes

rtpsw added 6 commits October 14, 2022 06:03

distinct keys per input table
32d31e1

CI fixes
bca5447

resolve conflict
89906a4

Merge branch 'master' into ARROW-17980
0741286

fix typo
0e08af1

Merge branch 'ARROW-17980' ofhttps://github.com/rtpsw/arrowinto ARR…
8e180d6
…OW-17980

Merge branch 'master' into ARROW-17980
cea57a9

icexelloss reviewed Oct 17, 2022
View reviewed changes

Comment threadcpp/proto/substrait/extension_rels.proto

icexelloss reviewed Oct 17, 2022
View reviewed changes

Comment threadcpp/src/arrow/engine/substrait/expression_internal.cc Outdated

ARROW-15732: Do not use any CPU threads in execution plan when use_th…
ba6fa88
…reads is false

westonpaceand others added 9 commits October 19, 2022 14:17

DO-NOT-MERGE: Remove CI jobs that aren't needed for this demo
553fc25

DO-NOT-MERGE: Remove CI jobs that aren't needed for this demo
0ac6f11

DO-NOT-MERGE: Removed double-ci caused by having a pr and a push trig…
efd5ae0
…ger (apache#16)

try smaller fix of MSVC namespace errors
5545a5e

second try
87e32c5

third try
88ec515

rtpsw added 2 commits October 23, 2022 07:23

requested changes
01a9543

Merge branch 'feature/bamboo-demo' into ARROW-17980
2525c7d

github-actionsBot added Component: Python Component: Parquet labels Oct 23, 2022

rtpsw added 2 commits October 23, 2022 09:29

fix serde test
3615159

Merge branch 'master' into ARROW-17980
3ad68bc

rtpsw closed this Oct 24, 2022

rtpsw deleted the ARROW-17980 branch October 24, 2022 10:47

rtpsw mentioned this pull request Oct 24, 2022
ARROW-17980: [C++] As-of-Join Substrait extension #14485
Merged

rtpsw mentioned this pull request Nov 14, 2022
ARROW-17642: [C++] Add ordered aggregation #14352
Closed

asfimport mentioned this pull request Jan 1, 2023
[C++] As-of-Join Substrait extension #33186
Closed

Uh oh!

Conversation

rtpsw commented Oct 12, 2022

Uh oh!

rtpsw commented Oct 12, 2022

Uh oh!

Uh oh!

rtpsw commented Oct 12, 2022

Uh oh!

icexelloss commented Oct 12, 2022

Uh oh!

icexelloss commented Oct 12, 2022

Uh oh!

rtpsw commented Oct 12, 2022

Uh oh!

rtpsw commented Oct 12, 2022

Uh oh!

github-actionsBot commented Oct 12, 2022

Uh oh!

rtpsw commented Oct 13, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

icexellossOct 13, 2022

Choose a reason for hiding this comment

Uh oh!

rtpswOct 15, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

icexelloss commented Oct 13, 2022

Uh oh!

rtpsw commented Oct 15, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rtpsw commented Oct 19, 2022

Uh oh!

icexelloss commented Oct 19, 2022

Uh oh!

rtpsw commented Oct 19, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rtpsw commented Oct 23, 2022

Uh oh!

rtpsw commented Oct 24, 2022

Uh oh!

icexelloss commented Oct 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rtpsw commented Oct 15, 2022•
edited
Loading

rtpsw commented Oct 19, 2022•
edited
Loading