ARROW-17980: [C++] As-of-Join Substrait extension #14485

rtpsw · 2022-10-24T10:54:53Z

Replacing #14385

github-actions · 2022-10-24T10:55:17Z

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

cpp/src/arrow/compute/exec/asof_join_benchmark.cc

cpp/src/arrow/engine/substrait/test_plan_builder.cc

westonpace

I'm a little torn on this PR. It seems that it includes #14415 which hasn't merged yet, although that is primarily because it blocked waiting for substrait-io/substrait#342 . At this point I think @bkietz remaining comments were fairly minor and I had addressed them. Also, since things are a bit squashed, it is unclear to me how much of #14415 this includes. Do you know if you were able to squash in all the commits from #14415 ?

Also, this effectively means we are temporarily based on a Substrait PR while we wait for Substrait to catch up.

I'm not a big fan of the fact that we need the using statements for the substrait namespace but if we really want that package name then it seems protobuf gives us little choice.

That being said, this PR works, I don't see anything functionally wrong with it, it is moving in the right direction, and it will make adopting the official Substrait release a smaller task. We should probably proceed by merging in #14415 properly, instead of forcing it through integrated as part of this PR though.

cpp/cmake_modules/ThirdpartyToolchain.cmake

cpp/proto/substrait/extension_rels.proto

cpp/src/arrow/engine/substrait/plan_internal.cc

cpp/src/arrow/engine/substrait/test_plan_builder.cc

cpp/src/arrow/engine/substrait/util.cc

cpp/src/arrow/compute/exec/asof_join_benchmark.cc

rtpsw · 2022-10-24T18:08:22Z

I'm a little torn on this PR. It seems that it includes #14415 which hasn't merged yet, although that is primarily because it blocked waiting for substrait-io/substrait#342 . At this point I think @bkietz remaining comments were fairly minor and I had addressed them. Also, since things are a bit squashed, it is unclear to me how much of #14415 this includes. Do you know if you were able to squash in all the commits from #14415 ?

Unfortunately, I can't say whether this PR includes all of #14415. To avoid confusion, I'm fine with marking this PR a draft. I'll hold to see how you and @bkietz suggest we should proceed.

cpp/src/arrow/engine/substrait/options.h

anjakefala

I left one comment about whether we want to require protobuf headers in order to build libarrow_python with substrait.

Additionally, find_package(Protobuf) seems to be missing from the CMakeLists

I would add the following here https://github.com/rtpsw/arrow/blob/fce4dff7df8577731cccb220457ade5e24a33e60/python/CMakeLists.txt#L232

if(ARROW_PROTOBUF_USE_SHARED)
  find_package(Protobuf)
  include_directories(${PROTOBUF_INCLUDE_DIRS})
  get_property(dirs DIRECTORY PROPERTY INCLUDE_DIRECTORIES)
  foreach(dir ${dirs})
    message("dir='${dir}'")
  endforeach()
  get_cmake_property(_variableNames VARIABLES)
  foreach (_variableName ${_variableNames})
    message("${_variableName}=${${_variableName}}")
  endforeach()
endif()

But, there might be a better place for it.

cpp/src/arrow/engine/substrait/options.h

westonpace · 2022-11-03T21:48:32Z

I think before merging we should address if this is behaviour we want to keep.

@anjakefala It is not. I would like to hide protobuf from python (I suspect an options_internal.h is doable somehow).

westonpace

Now that #14415 has merged we can move forwards on this. However, the code here is out of date (it was based on an older revision of #14415) and I worry it might not merge so cleanly. Let me know if you have trouble and I can take a stab at it.

cpp/cmake_modules/ThirdpartyToolchain.cmake

cpp/proto/substrait/extension_rels.proto

cpp/src/arrow/compute/exec/options.h

cpp/src/arrow/engine/substrait/expression_internal.cc

cpp/src/arrow/engine/substrait/options.cc

rtpsw · 2022-11-25T11:16:44Z

Now that #14415 has merged we can move forwards on this. However, the code here is out of date (it was based on an older revision of #14415) and I worry it might not merge so cleanly. Let me know if you have trouble and I can take a stab at it.

I will try to create a replacement PR.

rtpsw · 2022-11-25T16:34:46Z

I ended up merging. @westonpace, does it look like what you expected?

rtpsw · 2022-11-27T10:29:47Z

How to deal with the following CI job's error?

/arrow/cpp/src/arrow/engine/substrait/options.h:37: error: documented symbol 'enum ARROW_ENGINE_EXPORT arrow::engine::arrow::engine::ConversionStrictness' was not declared or defined. (warning treated as error, aborting now)

rtpsw · 2022-12-02T08:53:22Z

I'm not a big fan of the fact that we need the using statements for the substrait namespace but if we really want that package name then it seems protobuf gives us little choice.

@westonpace, note that this PR is currently using a substrait_ext namespace

rtpsw · 2022-12-05T15:55:58Z

How to deal with the following CI job's error?

/arrow/cpp/src/arrow/engine/substrait/options.h:37: error: documented symbol 'enum ARROW_ENGINE_EXPORT arrow::engine::arrow::engine::ConversionStrictness' was not declared or defined. (warning treated as error, aborting now)

Fixed in https://issues.apache.org/jira/browse/ARROW-18424

westonpace · 2022-12-06T07:59:18Z

@rtpsw Looks like the merge didn't go through cleanly. Let me know if you want me to clean this up. Have you been able to address the concern here: #14485 (review) ?

rtpsw · 2022-12-06T09:39:53Z

@rtpsw Looks like the merge didn't go through cleanly. Let me know if you want me to clean this up.

Note that this is a result of a GitHub automated rebase. @westonpace, if you can easily clean this up, that would be helpful.

Have you been able to address the concern here: #14485 (review) ?

I have removed the dependence on protobuf in the header, but I haven't added the proposed cmake snippet. It sounded like the place for this snippet is under discussion. I'm also not sure whether it is required in this PR or can be deferred.

rtpsw · 2022-12-10T09:39:06Z

@rtpsw I had to rebase this again. I've looked through it and I think this is in good shape now. If you're ok with it I will merge once the tests pass.

I compared the codes, mostly manually, and couldn't find anything missing.

westonpace · 2022-12-15T23:40:47Z

The tests were failing after a rebase due to a recent change to CheckRoundTrip (doesn't need a schema). I pushed a small fix.

Unfortunately, the test is now failing for me because the process thread marks the plan complete and then tries to join itself. I copied over the change we made in b9cda43 to address this but that change relies on an executor always being present which won't be true until ARROW-15732 merges.

rtpsw · 2022-12-22T14:06:56Z

@westonpace, I see you approved - is there anything holding up this PR?

westonpace · 2022-12-22T17:48:58Z

@rtpsw I believe it's the same failure you noticed before, which is that there is a segfault because schedule task is synchronous. In the other PR you fixed it by requiring an executor but that introduces ordering problems if I remember correctly. Although it may not matter for a test and will allow us to move forward on this. I will try this real quick.

rtpsw · 2022-12-23T09:16:20Z

@rtpsw I believe it's the same failure you noticed before, which is that there is a segfault because schedule task is synchronous. In the other PR you fixed it by requiring an executor but that introduces ordering problems if I remember correctly. Although it may not matter for a test and will allow us to move forward on this. I will try this real quick.

If indeed it introduces these ordering problems, there are alternatives fixes for the segfault, e.g., use a standalone thread for that task or use a new ExecContext::async_executor() method that is guaranteed to return an asynchronous executor.

westonpace · 2022-12-26T15:35:45Z

If indeed it introduces these ordering problems, there are alternatives fixes for the segfault, e.g., use a standalone thread for that task or use a new ExecContext::async_executor() method that is guaranteed to return an asynchronous executor.

Agreed. I think we can address this later though. I've rebased (had to address the query context change) and would like to merge this when it is green.

…he asof join node is not marked finished from the process thread. Doesn't currently work because executor can be null.

…executor for asof-join

westonpace · 2022-12-30T19:26:50Z

Agreed. I think we can address this later though. I've rebased (had to address the query context change) and would like to merge this when it is green.

The problem was still happening. I've fixed it properly in #15104 using the same fix that was in the demo branch (ensuring that we always have an executor and the async scheduler is never synchronous).

I've rebased this (once more) and the problem should no longer occur.

westonpace · 2022-12-30T23:43:24Z

CI failure seems surprising but unrelated. I filed #15137 to follow up.

westonpace · 2022-12-31T20:29:39Z

Failure appears unrelated. I will merge.

westonpace · 2022-12-31T20:30:58Z

@rtpsw thanks for your persistence on this one. Sorry for the rebase troubles at the end.

ursabot · 2022-12-31T23:12:33Z

Benchmark runs are scheduled for baseline = 9857891 and contender = 85db6f7. 85db6f7 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️7.22% ⬆️6.67%] test-mac-arm
[Finished ⬇️2.04% ⬆️0.26%] ursa-i9-9960x
[Failed ⬇️0.0% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 85db6f7b ec2-t3-xlarge-us-east-2
[Failed] 85db6f7b test-mac-arm
[Finished] 85db6f7b ursa-i9-9960x
[Failed] 85db6f7b ursa-thinkcentre-m75q
[Finished] 98578916 ec2-t3-xlarge-us-east-2
[Failed] 98578916 test-mac-arm
[Finished] 98578916 ursa-i9-9960x
[Failed] 98578916 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. 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

ursabot · 2022-12-31T23:12:50Z

['Python', 'R'] benchmarks have high level of regressions.
test-mac-arm
ursa-i9-9960x

rtpsw · 2023-01-01T21:28:27Z

@westonpace, thanks for wrapping this up.

Replacing apache#14385 Lead-authored-by: Yaron Gvili <[email protected]> Co-authored-by: Weston Pace <[email protected]> Signed-off-by: Weston Pace <[email protected]>

github-actions bot added the Component: C++ label Oct 24, 2022

rtpsw commented Oct 24, 2022

View reviewed changes

cpp/src/arrow/compute/exec/asof_join_benchmark.cc Outdated Show resolved Hide resolved

cpp/src/arrow/engine/substrait/test_plan_builder.cc Outdated Show resolved Hide resolved

westonpace reviewed Oct 24, 2022

View reviewed changes

anjakefala reviewed Nov 3, 2022

View reviewed changes

cpp/src/arrow/engine/substrait/options.h Outdated Show resolved Hide resolved

anjakefala reviewed Nov 3, 2022

View reviewed changes

cpp/src/arrow/engine/substrait/options.h Outdated Show resolved Hide resolved

westonpace requested changes Nov 25, 2022

View reviewed changes

github-actions bot added Component: Documentation Component: FlightRPC Component: C++ - Gandiva Component: GLib Component: Go Component: Java Component: Python Component: R Component: Parquet labels Nov 25, 2022

westonpace force-pushed the ARROW-17980 branch from 1c6f6d0 to 29cc649 Compare December 7, 2022 01:01

github-actions bot removed Component: Go Component: R Component: Parquet Component: GLib labels Dec 7, 2022

westonpace force-pushed the ARROW-17980 branch from fd8fedb to 80afd92 Compare December 26, 2022 15:34

rtpsw and others added 9 commits December 30, 2022 11:01

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

5272574

remove options.h dependency on protobuf

ad04595

requested changes

05d6aa6

requested fixes

498db94

Cleaning up ::substrait that was introduced again during rebase

149380c

Fix test error introduced by rebase. Add in logic to try and ensure t…

bb30d53

…he asof join node is not marked finished from the process thread. Doesn't currently work because executor can be null.

Small workaround for a bug related to ARROW-15732. We have to use an …

e7cc46b

…executor for asof-join

clang format

4462a3c

Minor fix after rebase

ddaef9d

westonpace force-pushed the ARROW-17980 branch from 80afd92 to ddaef9d Compare December 30, 2022 19:25

westonpace merged commit 85db6f7 into apache:master Dec 31, 2022

rtpsw deleted the ARROW-17980 branch January 1, 2023 21:28

This was referenced Jan 1, 2023

[C++] As-of-Join Substrait extension #33186

Closed

[C++] Expose DeclarationInfo #33565

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-17980: [C++] As-of-Join Substrait extension #14485

ARROW-17980: [C++] As-of-Join Substrait extension #14485

rtpsw commented Oct 24, 2022

github-actions bot commented Oct 24, 2022

westonpace left a comment

rtpsw commented Oct 24, 2022

anjakefala left a comment

westonpace commented Nov 3, 2022

westonpace left a comment

rtpsw commented Nov 25, 2022

rtpsw commented Nov 25, 2022

rtpsw commented Nov 27, 2022

rtpsw commented Dec 2, 2022

rtpsw commented Dec 5, 2022

westonpace commented Dec 6, 2022

rtpsw commented Dec 6, 2022 •

edited

Loading

rtpsw commented Dec 10, 2022

westonpace commented Dec 15, 2022

rtpsw commented Dec 22, 2022

westonpace commented Dec 22, 2022

rtpsw commented Dec 23, 2022

westonpace commented Dec 26, 2022

westonpace commented Dec 30, 2022

westonpace commented Dec 30, 2022

westonpace commented Dec 31, 2022

westonpace commented Dec 31, 2022

ursabot commented Dec 31, 2022

ursabot commented Dec 31, 2022

rtpsw commented Jan 1, 2023

ARROW-17980: [C++] As-of-Join Substrait extension #14485

ARROW-17980: [C++] As-of-Join Substrait extension #14485

Conversation

rtpsw commented Oct 24, 2022

github-actions bot commented Oct 24, 2022

westonpace left a comment

Choose a reason for hiding this comment

rtpsw commented Oct 24, 2022

anjakefala left a comment

Choose a reason for hiding this comment

westonpace commented Nov 3, 2022

westonpace left a comment

Choose a reason for hiding this comment

rtpsw commented Nov 25, 2022

rtpsw commented Nov 25, 2022

rtpsw commented Nov 27, 2022

rtpsw commented Dec 2, 2022

rtpsw commented Dec 5, 2022

westonpace commented Dec 6, 2022

rtpsw commented Dec 6, 2022 • edited Loading

rtpsw commented Dec 10, 2022

westonpace commented Dec 15, 2022

rtpsw commented Dec 22, 2022

westonpace commented Dec 22, 2022

rtpsw commented Dec 23, 2022

westonpace commented Dec 26, 2022

westonpace commented Dec 30, 2022

westonpace commented Dec 30, 2022

westonpace commented Dec 31, 2022

westonpace commented Dec 31, 2022

ursabot commented Dec 31, 2022

ursabot commented Dec 31, 2022

rtpsw commented Jan 1, 2023

rtpsw commented Dec 6, 2022 •

edited

Loading