Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-15284: [C++] Use DeclarationToExecBatches in Acero plan tests #15288

Merged
merged 12 commits into from
Jan 20, 2023

Conversation

vibhatha
Copy link
Collaborator

@vibhatha vibhatha commented Jan 10, 2023

This PR includes a change to the existing test cases in plan_test.cc for Acero-based plan evaluation.

At the moment, using the StartAndCollect method and manually passing the FutureMatcher and expected results are done and the validation is done on the results obtained from the StartAndCollect method. Most of the updated test cases in this PR uses the a common format.

Replacing that format and using DeclarationToExecBatches makes the code more readable and easy to debug.

TODO

  • Convert other tests with sort options and custom sinks
  • Self review 1
  • Self review 2
  • Check CI Status
  • Mark Ready to Review

@github-actions
Copy link

@vibhatha vibhatha marked this pull request as ready for review January 13, 2023 02:27
Copy link
Member

@westonpace westonpace left a comment

Choose a reason for hiding this comment

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

A few minor suggestions. Thanks for the cleanup.

Looks like you preferred to set use_threads=false which I suppose more accurately reflected the old behavior. Can we instead just not specify it (leave it parallel, the default) unless needed?

Comment on lines +316 to +317
ASSERT_OK_AND_ASSIGN(auto result,
DeclarationToExecBatches(std::move(plan), /*use_threads=*/false));
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason for use_threads=false here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not really, just left the test case as it hasn’t used parallel mode. Should we prefer testing both modes at all times?

PS: I sort of did that, but again removed that thinking I am introducing which is not in the current test cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am setting this to default just now... I will confirm one's it is tested.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One thing I observed is that the following happens, at least once per 20 repetitions on a single bash script test. Total of 100 repetitions (5 bash runs), caused 5 failures. So I would say it is consistently failing.

arrow-compute-plan-test: /home/asus/github/fork/arrow/cpp/src/arrow/util/async_generator.h:1736: arrow::BackgroundGenerator<T>::Cleanup::~Cleanup() [with T = std::optional<arrow::compute::ExecBatch>]: Assertion `state->worker_thread_id.load() != ::arrow::internal::GetThreadId()' failed.
Aborted (core dumped)

I recommend setting use_threads=false for the moment, but investigate why in a separate issue. But do you have an insight on this?

PS: Reading the CleanUp destructor, seems like it is expected.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just for the curiosity I repeated this on the use_threads=false setting (which is the same state this was run before making this change, I guess).

The following was my observation

/home/asus/github/fork/arrow/cpp/src/arrow/compute/exec/plan_test.cc:887: Failure
Value of: sink_gen()
Expected: value value value is equal to ExecBatch
    # Rows: 4
    0: Array[-2147483380,808065407,-681488731,829937304]
    1: Array[true,false,false,false]

  Actual: 16-byte object <F0-5B 46-2E D1-55 00-00 90-75 47-2E D1-55 00-00>, whose value (ExecBatch
    # Rows: 4
    0: Array[-2147483418,113924762,1252144620,-193012790]
    1: Array[false,true,true,true]
) doesn't match, whose value ExecBatch
    # Rows: 4
    0: Array[-2147483418,113924762,1252144620,-193012790]
    1: Array[false,true,true,true]
 doesn't match
Google Test trace:
/home/asus/github/fork/arrow/cpp/src/arrow/compute/exec/plan_test.cc:866: single threaded
/home/asus/github/fork/arrow/cpp/src/arrow/compute/exec/plan_test.cc:863: unslowed


-----------------------------------------------------------------------------------------------------

/home/asus/github/fork/arrow/cpp/src/arrow/compute/exec/plan_test.cc:887: Failure
Value of: sink_gen()
Expected: value value value is equal to ExecBatch
    # Rows: 4
    0: Array[-2147483380,808065407,-681488731,829937304]
    1: Array[true,false,false,false]

  Actual: 16-byte object <00-66 F7-B1 AD-55 00-00 40-DE F7-B1 AD-55 00-00>, whose value (ExecBatch
    # Rows: 4
    0: Array[-2147483522,-417100609,768701078,-1848250513]
    1: Array[true,false,true,true]
) doesn't match, whose value ExecBatch
    # Rows: 4
    0: Array[-2147483522,-417100609,768701078,-1848250513]
    1: Array[true,false,true,true]
 doesn't match
Google Test trace:
/home/asus/github/fork/arrow/cpp/src/arrow/compute/exec/plan_test.cc:866: parallel
/home/asus/github/fork/arrow/cpp/src/arrow/compute/exec/plan_test.cc:863: unslowed


-----------------------------------------------------------------------------------------------------

/home/asus/github/fork/arrow/cpp/src/arrow/compute/exec/plan_test.cc:887: Failure
Value of: sink_gen()
Expected: value value value is equal to ExecBatch
    # Rows: 4
    0: Array[-2147483380,808065407,-681488731,829937304]
    1: Array[true,false,false,false]

  Actual: 16-byte object <B0-75 1A-B3 48-56 00-00 30-06 1D-B3 48-56 00-00>, whose value (ExecBatch
    # Rows: 4
    0: Array[-2147483522,-417100609,768701078,-1848250513]
    1: Array[true,false,true,true]
) doesn't match, whose value ExecBatch
    # Rows: 4
    0: Array[-2147483522,-417100609,768701078,-1848250513]
    1: Array[true,false,true,true]
 doesn't match
Google Test trace:
/home/asus/github/fork/arrow/cpp/src/arrow/compute/exec/plan_test.cc:866: parallel
/home/asus/github/fork/arrow/cpp/src/arrow/compute/exec/plan_test.cc:863: unslowed

For 160 repetitions (bash on 8 runs), I observed 3 failures. Could say 3/8 fails. Do you have a clue why this could be happening? The failing test is ExecPlanExecution.StressSourceSinkStopped. A single run includes all test cases.

But when run just this case over 100 repetitions, it failed only 2 times. I suspect a resource allocation issue. Appreciate your thoughts.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I have a fix for that test here: https://github.com/apache/arrow/pull/15253/files#diff-b4e59a9cedddaaa33c03f220c03ac812f9ebdfb06692c9f91c5f8f5988363b57R853

However, I don't know if that PR will get finished in time for the release. I'll open an independent PR with the fix. The test does this:

  • Generate a bunch of random batches
  • Start the plan
  • Wait for one batch to arrive
  • Cancel/abort the plan
  • Verify the one batch that arrived was the first of the random batches

However, there is no guarantee, when run in parallel, that the first batch to complete will be the first batch generated.

Since the test doesn't fully consume the plan we can't use DeclarationToXyz here. This means when run serially it is using "single-thread mode" which is still slightly parallel enough to cause random ordering.

In my fix I changed it so parallel=false would run synchronously (and have dependent order) and parallel=true would not require it to be the first batch generated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Shall we hold this until this PR is merged?

Copy link
Member

Choose a reason for hiding this comment

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

No, sorry. I thought you were modifying the test you were referring to but I see now you are not. I think this PR is good to go.

cpp/src/arrow/compute/exec/plan_test.cc Outdated Show resolved Hide resolved
cpp/src/arrow/compute/exec/plan_test.cc Outdated Show resolved Hide resolved
cpp/src/arrow/compute/exec/plan_test.cc Show resolved Hide resolved
cpp/src/arrow/compute/exec/plan_test.cc Outdated Show resolved Hide resolved
vibhatha and others added 4 commits January 14, 2023 06:37
Co-authored-by: Weston Pace <weston.pace@gmail.com>
Co-authored-by: Weston Pace <weston.pace@gmail.com>
Co-authored-by: Weston Pace <weston.pace@gmail.com>
Co-authored-by: Weston Pace <weston.pace@gmail.com>
@vibhatha vibhatha requested a review from westonpace January 14, 2023 01:12
@vibhatha
Copy link
Collaborator Author

vibhatha commented Jan 16, 2023

@westonpace does this PR need further changes?

@westonpace westonpace merged commit 0e4a2e1 into apache:master Jan 20, 2023
@ursabot
Copy link

ursabot commented Jan 20, 2023

Benchmark runs are scheduled for baseline = bac8ef2 and contender = 0e4a2e1. 0e4a2e1 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
Buildkite builds:
[Failed] 0e4a2e19 ec2-t3-xlarge-us-east-2
[Failed] 0e4a2e19 test-mac-arm
[Failed] 0e4a2e19 ursa-i9-9960x
[Failed] 0e4a2e19 ursa-thinkcentre-m75q
[Failed] bac8ef2c ec2-t3-xlarge-us-east-2
[Failed] bac8ef2c test-mac-arm
[Failed] bac8ef2c ursa-i9-9960x
[Failed] bac8ef2c 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

sjperkins pushed a commit to sjperkins/arrow that referenced this pull request Feb 10, 2023
apache#15288)

This PR includes a change to the existing test cases in `plan_test.cc` for Acero-based plan evaluation. 

At the moment, using the `StartAndCollect` method and manually passing the `FutureMatcher` and expected results are done and the validation is done on the results obtained from the `StartAndCollect` method. Most of the updated test cases in this PR uses the a common format. 

Replacing that format and using `DeclarationToExecBatches` makes the code more readable and easy to debug. 
### TODO
- [x] Convert other tests with sort options and custom sinks
- [x] Self review 1
- [x] Self review 2
- [x] Check CI Status
- [x] Mark Ready to Review

* Closes: apache#15284

Lead-authored-by: vibhatha <vibhatha@gmail.com>
Co-authored-by: Vibhatha Lakmal Abeykoon <vibhatha@users.noreply.github.com>
Co-authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[C++] Use DeclarationToExecBatches in Acero plan tests
3 participants