GH-46224: [C++][Acero] Fix the hang in asof join #46300

zanmato1984 · 2025-05-03T06:50:16Z

Rationale for this change

A hang of asof join is reported in #46224 . To explain the cause of the hang, I shall first brief the basic processing of asof join.

There is an outstanding processing thread for the asof join, whose lifespan is bound to the asof node. This thread will wait on a command queue. Once a command is pushed into the queue, the thread pops it and do one round of processing. The command is issued as the batches are received from either left or right side input of the asof join node. Each round of processing will advance the accumulated left and right inputs as much as possible to see if it is sufficient to output a batch (if yes, i.e., either side of the asof join can be concluded by the latest input timestamp, then emit it, otherwise adjust the internal state according to the latest input timestamp).

Notably, the processing is by design to advance the inputs "as much as possible". However there is a bug (seemingly since day 1) when advancing the right side input in a tricky condition: if all right side rows are at the times before the least time of the left rows, then advancing the right side input won't cross batches in one round of processing. This is OK (i.e., no hang) as long as there are enough commands issued into the queue to trigger enough rounds of processing. However if lucky enough, say, the left input batches come late, then the processing with consume some commands issued by the right side input but advance nothing (because the left side input is empty), resulting in insufficient commands to fully advance both the input.

What changes are included in this PR?

Fix the issue that advancing the right side input won't cross batches when the minimal left time is bigger than all the right times.

Are these changes tested?

Yes, added a dedicated case.

Are there any user-facing changes?

None.

GitHub Issue: [C++][Acero] Apparent deadlock in Table.join_asof #46224

github-actions · 2025-05-03T06:50:46Z

⚠️ GitHub issue #46224 has been automatically assigned in GitHub to PR creator.

zanmato1984 · 2025-05-03T06:51:07Z

@github-actions crossbow submit -g cpp -g r

github-actions · 2025-05-03T06:54:09Z

Revision: aa5a9be

Submitted crossbow builds: ursacomputing/crossbow @ actions-d733af0ab3

Task	Status
example-cpp-minimal-build-static
example-cpp-minimal-build-static-system-dependency
example-cpp-tutorial
r-binary-packages
r-recheck-most
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-meson
test-conda-cpp-valgrind
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-fedora-39-cpp
test-r-arrow-backwards-compatibility
test-r-clang-sanitizer
test-r-depsource-bundled
test-r-depsource-system
test-r-dev-duckdb
test-r-devdocs
test-r-extra-packages
test-r-gcc-11
test-r-gcc-12
test-r-install-local
test-r-install-local-minsizerel
test-r-linux-as-cran
test-r-linux-rchk
test-r-linux-sanitizer
test-r-linux-valgrind
test-r-macos-as-cran
test-r-minimal-build
test-r-offline-maximal
test-r-offline-minimal
test-r-rhub-debian-gcc-devel-lto-latest
test-r-rhub-debian-gcc-release-custom-ccache
test-r-rhub-ubuntu-release-latest
test-r-rocker-r-ver-latest
test-r-rstudio-r-base-4.1-opensuse155
test-r-rstudio-r-base-4.2-focal
test-r-ubuntu-22.04
test-r-versions
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-bundled
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-bundled-offline
test-ubuntu-24.04-cpp-gcc-13-bundled
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-24.04-cpp-minimal-with-formats
test-ubuntu-24.04-cpp-thread-sanitizer

zanmato1984 · 2025-05-03T07:07:02Z

@github-actions crossbow submit -g cpp -g r

github-actions · 2025-05-03T07:10:06Z

Revision: 6ea4624

Submitted crossbow builds: ursacomputing/crossbow @ actions-a2fa793a58

Task	Status
example-cpp-minimal-build-static
example-cpp-minimal-build-static-system-dependency
example-cpp-tutorial
r-binary-packages
r-recheck-most
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-meson
test-conda-cpp-valgrind
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-fedora-39-cpp
test-r-arrow-backwards-compatibility
test-r-clang-sanitizer
test-r-depsource-bundled
test-r-depsource-system
test-r-dev-duckdb
test-r-devdocs
test-r-extra-packages
test-r-gcc-11
test-r-gcc-12
test-r-install-local
test-r-install-local-minsizerel
test-r-linux-as-cran
test-r-linux-rchk
test-r-linux-sanitizer
test-r-linux-valgrind
test-r-macos-as-cran
test-r-minimal-build
test-r-offline-maximal
test-r-offline-minimal
test-r-rhub-debian-gcc-devel-lto-latest
test-r-rhub-debian-gcc-release-custom-ccache
test-r-rhub-ubuntu-release-latest
test-r-rocker-r-ver-latest
test-r-rstudio-r-base-4.1-opensuse155
test-r-rstudio-r-base-4.2-focal
test-r-ubuntu-22.04
test-r-versions
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-bundled
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-bundled-offline
test-ubuntu-24.04-cpp-gcc-13-bundled
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-24.04-cpp-minimal-with-formats
test-ubuntu-24.04-cpp-thread-sanitizer

zanmato1984 · 2025-05-06T23:14:07Z

cpp/src/arrow/acero/asof_join_node.cc

@@ -639,12 +639,8 @@ class InputState : public util::SerialSequencingQueue::Processor {
        // hit the end of the batch, need to get the next batch if possible.
        ++batches_processed_;
        latest_ref_row_ = 0;
-        have_active_batch &= !queue_.TryPop();


IMO this code is apparently NOT doing what it is supposed to do: by here the have_active_batch must be true, meaning queue_ must be non-empty. Then popping the queue always gets a valid std::optional and effectively setting have_active_batch to false. The if branch below won't be executed.

I noticed there are two fixes made before in the following if branch #36094 and #36499. Given that this have_active_batch &= !queue_.TryPop() exists since #13028 where it was introduced, and the lack of corresponding tests, I'm not sure how they fixed anything and what the bugs even were. So I just remove them.

zanmato1984 · 2025-05-06T23:15:11Z

Hi, @westonpace @icexelloss @pitrou would you help to review? Thanks.

zanmato1984 · 2025-05-07T01:21:51Z

@github-actions crossbow submit -g cpp -g python -g r

github-actions · 2025-05-07T01:25:53Z

Revision: af70071

Submitted crossbow builds: ursacomputing/crossbow @ actions-ccf7708cfd

Task	Status
example-cpp-minimal-build-static
example-cpp-minimal-build-static-system-dependency
example-cpp-tutorial
example-python-minimal-build-fedora-conda
example-python-minimal-build-ubuntu-venv
r-binary-packages
r-recheck-most
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-meson
test-conda-cpp-valgrind
test-conda-python-3.10
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1
test-conda-python-3.10-pandas-latest-numpy-latest
test-conda-python-3.11
test-conda-python-3.11-dask-latest
test-conda-python-3.11-dask-upstream_devel
test-conda-python-3.11-hypothesis
test-conda-python-3.11-pandas-latest-numpy-1.26
test-conda-python-3.11-pandas-latest-numpy-latest
test-conda-python-3.11-pandas-nightly-numpy-nightly
test-conda-python-3.11-pandas-upstream_devel-numpy-nightly
test-conda-python-3.11-spark-master
test-conda-python-3.12
test-conda-python-3.12-cpython-debug
test-conda-python-3.13
test-conda-python-3.9
test-conda-python-3.9-pandas-1.1.3-numpy-1.19.5
test-conda-python-emscripten
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1
test-cuda-python-ubuntu-22.04-cuda-11.7.1
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-debian-12-python-3-amd64
test-debian-12-python-3-i386
test-fedora-39-cpp
test-fedora-39-python-3
test-r-arrow-backwards-compatibility
test-r-clang-asan
test-r-clang-ubsan
test-r-depsource-bundled
test-r-depsource-system
test-r-dev-duckdb
test-r-devdocs
test-r-extra-packages
test-r-gcc-11
test-r-gcc-12
test-r-install-local
test-r-install-local-minsizerel
test-r-linux-as-cran
test-r-linux-rchk
test-r-linux-sanitizer
test-r-linux-valgrind
test-r-m1-san
test-r-macos-as-cran
test-r-minimal-build
test-r-offline-maximal
test-r-offline-minimal
test-r-rhub-debian-gcc-devel-lto-latest
test-r-rhub-debian-gcc-release-custom-ccache
test-r-rhub-ubuntu-release-latest
test-r-rocker-r-ver-latest
test-r-rstudio-r-base-4.1-opensuse155
test-r-rstudio-r-base-4.2-focal
test-r-ubuntu-22.04
test-r-versions
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-bundled
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-22.04-python-3
test-ubuntu-22.04-python-313-freethreading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-bundled-offline
test-ubuntu-24.04-cpp-gcc-13-bundled
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-24.04-cpp-minimal-with-formats
test-ubuntu-24.04-cpp-thread-sanitizer
test-ubuntu-24.04-python-3

cpp/src/arrow/acero/asof_join_node.cc

cpp/src/arrow/acero/asof_join_node_test.cc

pitrou · 2025-05-07T07:39:02Z

I'm curious, this does only happen when the right side is concerned? The update logic is not symmetric?

zanmato1984 · 2025-05-07T17:49:23Z

I'm curious, this does only happen when the right side is concerned? The update logic is not symmetric?

Right, not symmetric as one might would naturally expect (the same situation for hash join as well - right side to build the hash table and left side to probe the hash table).

The left VS. right differs in the following aspects:

There is only one left side input but possibly multiple right side inputs. The right side inputs are flattened and equally iterated during the whole processing (as opposed to traversed in a tree-like fashion).
Several processing logic, and in addition some methods are right side inputs only, e.g.:

arrow/cpp/src/arrow/acero/asof_join_node.cc

Line 949 in 03bdb2a

Result<RhsUpdateState> UpdateRhs() {

and

arrow/cpp/src/arrow/acero/asof_join_node.cc

Line 963 in 03bdb2a

rhs.AdvanceAndMemoize(lhs_latest_time, rhs_empty));

So it's normal that some issues are right side only.

But it does remind me that the method I was fixing, namely ‎InputState::Advance, also applies to the left side input, so I think I should see if the issue exists for the left side as well, probably by adding a symmetric case you suggested.

Thanks for the question!

Co-authored-by: Antoine Pitrou <pitrou@free.fr>

zanmato1984 · 2025-05-07T19:55:28Z

Hi @pitrou , comments all addressed. Would you take a look again? Thanks.

cpp/src/arrow/acero/asof_join_node_test.cc

zanmato1984 · 2025-05-12T17:23:07Z

@github-actions crossbow submit -g cpp

github-actions · 2025-05-12T17:25:46Z

Revision: dd7a66b

Submitted crossbow builds: ursacomputing/crossbow @ actions-7d095c8a25

Task	Status
example-cpp-minimal-build-static
example-cpp-minimal-build-static-system-dependency
example-cpp-tutorial
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-meson
test-conda-cpp-valgrind
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-fedora-39-cpp
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-bundled
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-bundled-offline
test-ubuntu-24.04-cpp-gcc-13-bundled
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-24.04-cpp-minimal-with-formats
test-ubuntu-24.04-cpp-thread-sanitizer

zanmato1984 · 2025-05-12T18:01:26Z

@github-actions crossbow submit -g cpp

github-actions · 2025-05-12T18:04:12Z

Revision: 5f5b895

Submitted crossbow builds: ursacomputing/crossbow @ actions-973e5e92ed

Task	Status
example-cpp-minimal-build-static
example-cpp-minimal-build-static-system-dependency
example-cpp-tutorial
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-meson
test-conda-cpp-valgrind
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-fedora-39-cpp
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-bundled
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-bundled-offline
test-ubuntu-24.04-cpp-gcc-13-bundled
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-24.04-cpp-minimal-with-formats
test-ubuntu-24.04-cpp-thread-sanitizer

zanmato1984 · 2025-05-13T17:05:29Z

Thanks @pitrou for reviewing. Is there anything else to address?

pitrou

LGTM

conbench-apache-arrow · 2025-05-14T23:22:30Z

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit fe29b7d.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 19 possible false positives for unstable benchmarks that are known to sometimes produce them.

github-actions bot added Component: C++ awaiting review Awaiting review labels May 3, 2025

github-actions bot added Component: Documentation and removed Component: Documentation labels May 3, 2025

zanmato1984 commented May 6, 2025

View reviewed changes

zanmato1984 marked this pull request as ready for review May 6, 2025 23:14

zanmato1984 requested a review from westonpace as a code owner May 6, 2025 23:14

zanmato1984 requested a review from icexelloss May 6, 2025 23:14

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels May 6, 2025

zanmato1984 added 6 commits May 6, 2025 17:24

Draft the fix

b4b0cf8

Fix build

d1e11ae

Delete cpp/src/arrow/compute/row/doc/row_table.md

f1397a1

Remove hack for reproduction

4b9db70

Remove hack for reproduction

ecf58a0

Describe the test

af70071

zanmato1984 force-pushed the fix/gh-46224 branch from 13515f0 to af70071 Compare May 7, 2025 00:24

pitrou reviewed May 7, 2025

View reviewed changes

cpp/src/arrow/acero/asof_join_node.cc Outdated Show resolved Hide resolved

pitrou reviewed May 7, 2025

View reviewed changes

cpp/src/arrow/acero/asof_join_node_test.cc Show resolved Hide resolved

Update cpp/src/arrow/acero/asof_join_node.cc

3a273b3

Co-authored-by: Antoine Pitrou <pitrou@free.fr>

Update test according to the review comment

dd7a66b

pitrou reviewed May 12, 2025

View reviewed changes

cpp/src/arrow/acero/asof_join_node_test.cc Show resolved Hide resolved

Reduce test iteration for sanitizier and valgrind

5f5b895

pitrou approved these changes May 14, 2025

View reviewed changes

zanmato1984 merged commit fe29b7d into apache:main May 14, 2025
39 of 40 checks passed

zanmato1984 removed the awaiting committer review Awaiting committer review label May 14, 2025

zanmato1984 mentioned this pull request May 14, 2025

[C++][Acero] Apparent deadlock in Table.join_asof #46224

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-46224: [C++][Acero] Fix the hang in asof join #46300

GH-46224: [C++][Acero] Fix the hang in asof join #46300

zanmato1984 commented May 3, 2025 •

edited

Loading

github-actions bot commented May 3, 2025

zanmato1984 commented May 3, 2025

github-actions bot commented May 3, 2025

zanmato1984 commented May 3, 2025

github-actions bot commented May 3, 2025

zanmato1984 May 6, 2025

zanmato1984 May 7, 2025

zanmato1984 commented May 6, 2025

zanmato1984 commented May 7, 2025

github-actions bot commented May 7, 2025

pitrou commented May 7, 2025

zanmato1984 commented May 7, 2025

zanmato1984 commented May 7, 2025 •

edited

Loading

zanmato1984 commented May 12, 2025

github-actions bot commented May 12, 2025

zanmato1984 commented May 12, 2025

github-actions bot commented May 12, 2025

zanmato1984 commented May 13, 2025

pitrou left a comment

conbench-apache-arrow bot commented May 14, 2025

GH-46224: [C++][Acero] Fix the hang in asof join #46300

GH-46224: [C++][Acero] Fix the hang in asof join #46300

Conversation

zanmato1984 commented May 3, 2025 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented May 3, 2025

zanmato1984 commented May 3, 2025

github-actions bot commented May 3, 2025

zanmato1984 commented May 3, 2025

github-actions bot commented May 3, 2025

zanmato1984 May 6, 2025

Choose a reason for hiding this comment

zanmato1984 May 7, 2025

Choose a reason for hiding this comment

zanmato1984 commented May 6, 2025

zanmato1984 commented May 7, 2025

github-actions bot commented May 7, 2025

pitrou commented May 7, 2025

zanmato1984 commented May 7, 2025

zanmato1984 commented May 7, 2025 • edited Loading

zanmato1984 commented May 12, 2025

github-actions bot commented May 12, 2025

zanmato1984 commented May 12, 2025

github-actions bot commented May 12, 2025

zanmato1984 commented May 13, 2025

pitrou left a comment

Choose a reason for hiding this comment

conbench-apache-arrow bot commented May 14, 2025

zanmato1984 commented May 3, 2025 •

edited

Loading

zanmato1984 commented May 7, 2025 •

edited

Loading