You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/12/31 20:47:57 UTC
[GitHub] [arrow] westonpace opened a new pull request, #15142: GH-15141: [C++] fix for unstable test due to unstable sort
westonpace opened a new pull request, #15142:
URL: https://github.com/apache/arrow/pull/15142
The sorting done by orderby is not stable. This means, given the input:
a | b
--- | ---
1 | false
1 | true
the test could have generated both `[false, true]` and `[true, false]` for the `b` column. We likely did not encounter this before https://github.com/apache/arrow/commit/498b645e1d09306bf5399a9a019a5caa99513815 because the entire thing was run serially (even though there was a `parallel` option it was not setup correctly).
Now that things are properly running parallel the results are non-deterministic. We could remove the `b` column but I feel it is a better stress test to have at least one payload column. So I changed the test to only compare the key array and not the payload array.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace merged pull request #15142: GH-15141: [C++] fix for unstable test due to unstable sort
Posted by GitBox <gi...@apache.org>.
westonpace merged PR #15142:
URL: https://github.com/apache/arrow/pull/15142
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #15142: GH-15141: [C++] fix for unstable test due to unstable sort
Posted by GitBox <gi...@apache.org>.
westonpace commented on PR #15142:
URL: https://github.com/apache/arrow/pull/15142#issuecomment-1368359669
> Yes, I also think it’s better that way.
Hmm, I tried this but it turns out not to be so simple. I'm going to proceed with this how it is for now. We can worry about a full comparison later when we add a stable sort. I'll add a new issue requesting that. I'll merge this so it doesn't bother CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] vibhatha commented on pull request #15142: GH-15141: [C++] fix for unstable test due to unstable sort
Posted by GitBox <gi...@apache.org>.
vibhatha commented on PR #15142:
URL: https://github.com/apache/arrow/pull/15142#issuecomment-1368316900
> However, now that I write this, I realize it might be best to only apply my change when testing the parallel case, and to use the old comparison in the non-parallel case.
Yes, I also think it’s better that way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #15142: GH-15141: [C++] fix for unstable test due to unstable sort
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15142:
URL: https://github.com/apache/arrow/pull/15142#issuecomment-1368278157
* Closes: #15141
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #15142: GH-15141: [C++] fix for unstable test due to unstable sort
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15142:
URL: https://github.com/apache/arrow/pull/15142#issuecomment-1368278163
:warning: GitHub issue #15141 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #15142: GH-15141: [C++] fix for unstable test due to unstable sort
Posted by GitBox <gi...@apache.org>.
westonpace commented on PR #15142:
URL: https://github.com/apache/arrow/pull/15142#issuecomment-1368316255
> Just a question, is there a sort option which gives precedence to the index of the row and decide which comes first, when we have a tie like this?
That's called a "stable sort". The underlying sort kernel (SortIndices) **is** stable. However, if the plan is run in parallel, then there is no guarantee the batches will accumulate in the same order. So even if the sort kernel is stable the sort node is not.
Once we add proper ordering we can add a stable option to the sort node which resequences the data before sorting so that the sort node can remain stable.
However, now that I write this, I realize it might be best to only apply my change when testing the parallel case, and to use the old comparison in the non-parallel case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #15142: GH-15141: [C++] fix for unstable test due to unstable sort
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #15142:
URL: https://github.com/apache/arrow/pull/15142#issuecomment-1368387782
Benchmark runs are scheduled for baseline = db6c59d12ceb45634e568f5dbbe44e1c1c549eb0 and contender = 5a57e6dd1e4b1d89ecabe86b45d063619a0f2f54. 5a57e6dd1e4b1d89ecabe86b45d063619a0f2f54 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/c09e4f1b22ed4c7b950d5da102fbd072...aeea28f100b84f4c992ea7746d08f293/)
[Failed :arrow_down:0.56% :arrow_up:1.11%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/f8fcef8428f445b8969b7b5a71be3c9c...3303f2e7c354474bac57a4c7115a4124/)
[Finished :arrow_down:1.79% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/382e2a5025664432bffb70b4f7920c06...675dc32187454f8094b2945e31e9343f/)
[Failed :arrow_down:0.0% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/0e7cfb2461074edda0f5219d2206531b...b78e6968deee40a2b3391c7a1a91d391/)
Buildkite builds:
[Finished] [`5a57e6dd` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2090)
[Failed] [`5a57e6dd` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2112)
[Finished] [`5a57e6dd` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2084)
[Failed] [`5a57e6dd` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2104)
[Finished] [`db6c59d1` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2089)
[Failed] [`db6c59d1` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2111)
[Finished] [`db6c59d1` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2083)
[Failed] [`db6c59d1` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2103)
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #15142: GH-15141: [C++] fix for unstable test due to unstable sort
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #15142:
URL: https://github.com/apache/arrow/pull/15142#issuecomment-1368387812
['Python', 'R'] benchmarks have high level of regressions.
[ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/382e2a5025664432bffb70b4f7920c06...675dc32187454f8094b2945e31e9343f/)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] vibhatha commented on pull request #15142: GH-15141: [C++] fix for unstable test due to unstable sort
Posted by GitBox <gi...@apache.org>.
vibhatha commented on PR #15142:
URL: https://github.com/apache/arrow/pull/15142#issuecomment-1368294187
> @vibhatha would you be interested in providing a review / sanity check?
Sure @westonpace, I will.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #15142: GH-15141: [C++] fix for unstable test due to unstable sort
Posted by GitBox <gi...@apache.org>.
westonpace commented on PR #15142:
URL: https://github.com/apache/arrow/pull/15142#issuecomment-1368278801
@vibhatha would you be interested in providing a review / sanity check?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #15142: GH-15141: [C++] fix for unstable test due to unstable sort
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15142:
URL: https://github.com/apache/arrow/pull/15142#issuecomment-1368278166
:warning: GitHub issue #15141 **has no components**, please add labels for components.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org