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-35144: [C++] Fix a unit test broken when the output order of the aggregate node changed #35145

Conversation

westonpace
Copy link
Member

@westonpace westonpace commented Apr 14, 2023

Rationale for this change

The unit test was broken by a recent change and needed to be updated.

What changes are included in this PR?

The unit test has been updated to expect the correct order.

Are these changes tested?

Yes

Are there any user-facing changes?

No

@westonpace westonpace requested a review from icexelloss April 14, 2023 19:24
@github-actions
Copy link

@github-actions
Copy link

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

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Apr 14, 2023
@icexelloss
Copy link
Contributor

Thanks @westonpace - I found the same issue. I thought all tests passed before I merged it but I must have missed somehow. Thanks for fixing!

@westonpace westonpace merged commit d4f7332 into apache:main Apr 14, 2023
@westonpace
Copy link
Member Author

I thought all tests passed before I merged it but I must have missed somehow.

They probably did all pass but they ran before #34904 was merged in. This kind of thing happens every so often when two PRs conflict with each other.

@ursabot
Copy link

ursabot commented Apr 17, 2023

Benchmark runs are scheduled for baseline = 2ba6628 and contender = d4f7332. d4f7332 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] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.34% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] d4f73322 ec2-t3-xlarge-us-east-2
[Failed] d4f73322 test-mac-arm
[Finished] d4f73322 ursa-i9-9960x
[Finished] d4f73322 ursa-thinkcentre-m75q
[Finished] 2ba66285 ec2-t3-xlarge-us-east-2
[Failed] 2ba66285 test-mac-arm
[Finished] 2ba66285 ursa-i9-9960x
[Finished] 2ba66285 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

liujiacheng777 pushed a commit to LoongArch-Python/arrow that referenced this pull request May 11, 2023
… the aggregate node changed (apache#35145)

### Rationale for this change

The unit test was broken by a recent change and needed to be updated.

### What changes are included in this PR?

The unit test has been updated to expect the correct order.

### Are these changes tested?

Yes

### Are there any user-facing changes?

No
* Closes: apache#35144

Authored-by: Weston Pace <[email protected]>
Signed-off-by: Weston Pace <[email protected]>
ArgusLi pushed a commit to Bit-Quill/arrow that referenced this pull request May 15, 2023
… the aggregate node changed (apache#35145)

### Rationale for this change

The unit test was broken by a recent change and needed to be updated.

### What changes are included in this PR?

The unit test has been updated to expect the correct order.

### Are these changes tested?

Yes

### Are there any user-facing changes?

No
* Closes: apache#35144

Authored-by: Weston Pace <[email protected]>
Signed-off-by: Weston Pace <[email protected]>
rtpsw pushed a commit to rtpsw/arrow that referenced this pull request May 16, 2023
… the aggregate node changed (apache#35145)

### Rationale for this change

The unit test was broken by a recent change and needed to be updated.

### What changes are included in this PR?

The unit test has been updated to expect the correct order.

### Are these changes tested?

Yes

### Are there any user-facing changes?

No
* Closes: apache#35144

Authored-by: Weston Pace <[email protected]>
Signed-off-by: Weston Pace <[email protected]>
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++] Unit test regression introduced by #34931
3 participants