Skip to content

GH-34871: [C++] Fixed the add_dataset_test function to properly refer to the test file#34872

Merged
kou merged 1 commit into
apache:mainfrom
westonpace:bugfix/GH-34871--reenable-dataset-tests-and-fix-up
Apr 4, 2023
Merged

GH-34871: [C++] Fixed the add_dataset_test function to properly refer to the test file#34872
kou merged 1 commit into
apache:mainfrom
westonpace:bugfix/GH-34871--reenable-dataset-tests-and-fix-up

Conversation

@westonpace

@westonpace westonpace commented Apr 3, 2023 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Member

Rationale for this change

After the Acero refactoring, dataset tests were falsely reporting PASS without actually running any tests.

What changes are included in this PR?

The add_dataset_test function is fixed. This enabled the actual tests and revealed some compiler errors. I've addressed those compiler errors as well.

Are these changes tested?

Now they are :)

Are there any user-facing changes?

No

@westonpace
Fixed the add_dataset_test function to properly refer to the test fil…
7df10f5 
…e. Fixed tests which needed some compute->acero refactoring
@westonpace

westonpace commented Apr 3, 2023

Copy link
Copy Markdown
Member Author

CC @icexelloss please review

@github-actions

github-actions Bot commented Apr 3, 2023

Copy link
Copy Markdown

@icexelloss icexelloss self-requested a review April 3, 2023 22:06
icexelloss
icexelloss approved these changes Apr 3, 2023
View reviewed changes

@icexelloss icexelloss left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.

@github-actions github-actions Bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Apr 3, 2023
@icexelloss

icexelloss commented Apr 3, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

@westonpace I approved.

Although, I am curious how did this happen? Looking this the acero refactor PR:
f137f29#diff-1855e31fd38e7fa63c95344ad9992c27d2f609f015a06d45ca3dd3ecc708243d

It doesn't seem to remove anything from add_arrow_test in dataset/CMakeList.txt . What causes the dataset test to be disabled?

@icexelloss

icexelloss commented Apr 3, 2023

Copy link
Copy Markdown
Contributor

Perhaps this was a bug before and ${REL_TEST_NAME}.cc should be in add_arrow_test in the first place?

@westonpace

westonpace commented Apr 4, 2023

Copy link
Copy Markdown
Member Author

Although, I am curious how did this happen?

add_function_test can optionally take in a SOURCES argument. If it does not receive one then it defaults to ${REL_TEST_NAME}.cc. On the other hand, if it does receive one, then it assumes SOURCES overrides the default. The PR added SOURCES and set it equal to ["test_util_internal.cc"] when it should have been ["${REL_TEST_NAME.cc}", "test_util_internal.cc"]

@HaochengLIU HaochengLIU mentioned this pull request Apr 4, 2023
Merged
@kou kou changed the title GH-34871: [C++] Fixed the add_dataset_test function to properly refer to the test file. GH-34871: [C++] Fixed the add_dataset_test function to properly refer to the test file Apr 4, 2023
kou
kou approved these changes Apr 4, 2023
View reviewed changes

@kou kou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

@kou kou merged commit 84ac7d5 into apache:main Apr 4, 2023
@ursabot

ursabot commented Apr 4, 2023

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 6d109fb and contender = 84ac7d5. 84ac7d5 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 ⬇️0.56% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Failed ⬇️2.86% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 84ac7d5b ec2-t3-xlarge-us-east-2
[Failed] 84ac7d5b test-mac-arm
[Finished] 84ac7d5b ursa-i9-9960x
[Failed] 84ac7d5b ursa-thinkcentre-m75q
[Finished] 6d109fbf ec2-t3-xlarge-us-east-2
[Failed] 6d109fbf test-mac-arm
[Finished] 6d109fbf ursa-i9-9960x
[Failed] 6d109fbf 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

ArgusLi pushed a commit to Bit-Quill/arrow that referenced this pull request May 15, 2023
@westonpace @ArgusLi
apacheGH-34871: [C++] Fixed the add_dataset_test function to properly…
aa68fa5 
… refer to the test file (apache#34872)

### Rationale for this change

After the Acero refactoring, dataset tests were falsely reporting PASS without actually running any tests.

### What changes are included in this PR?

The add_dataset_test function is fixed.  This enabled the actual tests and revealed some compiler errors.  I've addressed those compiler errors as well.

### Are these changes tested?

Now they are :)

### Are there any user-facing changes?

No
* Closes: apache#34871

Authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kou kou kou approved these changes
@icexelloss icexelloss icexelloss approved these changes

Assignees

No one assigned

Labels

awaiting merge Awaiting merge Component: C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[C++] Dataset tests not running with Acero refactor

4 participants

@westonpace @icexelloss @ursabot @kou