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-43506: [Java] Fix TestFragmentScanOptions result not match #43639

Merged
merged 4 commits into from
Aug 18, 2024

Conversation

jinchengchenghh
Copy link
Contributor

@jinchengchenghh jinchengchenghh commented Aug 12, 2024

Rationale for this change

JNI test was not tested in CI. So the test failed but passed the CI.
The parseChar function should return char but return bool, a typo error.

What changes are included in this PR?

Are these changes tested?

Yes

Are there any user-facing changes?

No

Copy link

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

@vibhatha
Copy link
Collaborator

Rename to -> #43506: [Java] Fix TestFragmentScanOptions result not match

@vibhatha
Copy link
Collaborator

@github-actions crossbow submit -g java

Copy link

Revision: ea541c6

Submitted crossbow builds: ursacomputing/crossbow @ actions-acd4c173ff

Task Status
java-jars GitHub Actions
test-conda-python-3.11-spark-master GitHub Actions
verify-rc-source-java-linux-almalinux-8-amd64 GitHub Actions
verify-rc-source-java-linux-conda-latest-amd64 GitHub Actions
verify-rc-source-java-linux-ubuntu-20.04-amd64 GitHub Actions
verify-rc-source-java-linux-ubuntu-22.04-amd64 GitHub Actions
verify-rc-source-java-macos-amd64 GitHub Actions

@vibhatha
Copy link
Collaborator

@jinchengchenghh I think still there is an inaccuracy in the test case. One CI is failing.

@vibhatha vibhatha closed this Aug 12, 2024
@vibhatha vibhatha reopened this Aug 12, 2024
@vibhatha
Copy link
Collaborator

Still the CI is failing...

@vibhatha
Copy link
Collaborator

Still failing right...

@kou kou changed the title [GH-43506] Fix TestFragmentScanOptions result not match GH-43506: [Java] Fix TestFragmentScanOptions result not match Aug 12, 2024
Copy link

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

@jinchengchenghh
Copy link
Contributor Author

I will take a look today.

@jinchengchenghh
Copy link
Contributor Author

Only gandiva failed now. @vibhatha

@vibhatha
Copy link
Collaborator

@jinchengchenghh
Copy link
Contributor Author

Do I need to disable this test in this PR? Or you will create a new PR to disable it?

@vibhatha
Copy link
Collaborator

@kou shall we disable this like the last time but note this as an issue and comment in the code?

@kou
Copy link
Member

kou commented Aug 13, 2024

Yes but why is this happen again? Was #43503 not enough?

@vibhatha
Copy link
Collaborator

Yes but why is this happen again? Was #43503 not enough?

The tests related to Gandiva passed when we merged that PR, but it failed due to the exact reason in this PR. We might have missed something as it seems.

@jinchengchenghh
Copy link
Contributor Author

Yes, this test ProjectorTest.testMakeProjectorParallel is not disabled while ProjectorTest.testMakeProjector is disabled.
https://github.com/apache/arrow/blob/main/java/gandiva/src/test/java/org/apache/arrow/gandiva/evaluator/ProjectorTest.java#L152

Does it not occur before?

@vibhatha
Copy link
Collaborator

We should check that trace. We may have missed it.

@vibhatha
Copy link
Collaborator

@jinchengchenghh let's disable this too.

@jinchengchenghh
Copy link
Contributor Author

Yes, I think we could disable the gandiva test in a separate PR. Can we merge this one first? @vibhatha

@lidavidm
Copy link
Member

@github-actions crossbow submit -g java

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Aug 14, 2024
Copy link

Revision: 855a3af

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

Task Status
java-jars GitHub Actions
test-conda-python-3.11-spark-master GitHub Actions
verify-rc-source-java-linux-almalinux-8-amd64 GitHub Actions
verify-rc-source-java-linux-conda-latest-amd64 GitHub Actions
verify-rc-source-java-linux-ubuntu-20.04-amd64 GitHub Actions
verify-rc-source-java-linux-ubuntu-22.04-amd64 GitHub Actions
verify-rc-source-java-macos-amd64 GitHub Actions

@jinchengchenghh
Copy link
Contributor Author

Can you help merge this one? @vibhatha Thanks!

@vibhatha
Copy link
Collaborator

@jinchengchenghh I don't have merge rights. @lidavidm will definitely merge this as it is already approved. LGTM as well.

@jinchengchenghh
Copy link
Contributor Author

Ok. Thanks for your kindly review. @vibhatha

@vibhatha
Copy link
Collaborator

@jinchengchenghh did we file a new issue for the Gandiva failure?

@jinchengchenghh
Copy link
Contributor Author

Maybe we can use the exists issue apache/arrow-java#63

@vibhatha
Copy link
Collaborator

Sure let's do that.

@lidavidm lidavidm merged commit 5ef7e01 into apache:main Aug 18, 2024
16 of 17 checks passed
@lidavidm lidavidm removed the awaiting merge Awaiting merge label Aug 18, 2024
@lidavidm
Copy link
Member

@vibhatha do we need to ping any Gandiva maintainers to figure out the failures?

@vibhatha
Copy link
Collaborator

@lidavidm I think so. And I think @kou also tried to fix this earlier.

@lidavidm
Copy link
Member

ok, let me dig

@vibhatha
Copy link
Collaborator

Thanks @lidavidm, I am not well versed in this part of the project.

Copy link

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

There were no benchmark performance regressions. 🎉

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

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.

4 participants