Skip to content

GH-43506: [Java] Fix TestFragmentScanOptions result not match - #43639

Merged
lidavidm merged 4 commits into
apache:mainfrom
jinchengchenghh:option
Aug 18, 2024
Merged

GH-43506: [Java] Fix TestFragmentScanOptions result not match#43639
lidavidm merged 4 commits into
apache:mainfrom
jinchengchenghh:option

Conversation

@jinchengchenghh

@jinchengchenghhjinchengchenghh commented Aug 12, 2024

Copy link
Copy Markdown
Contributor

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

@github-actions

Copy link
Copy Markdown

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
Copy Markdown
Contributor

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

@vibhatha

Copy link
Copy Markdown
Contributor

@github-actions crossbow submit -g java

@github-actions

Copy link
Copy Markdown

Revision: ea541c6b8fdcb0f97f25d52dd07cb893fd2f15de

Submitted crossbow builds: ursacomputing/crossbow @ actions-acd4c173ff

TaskStatus
java-jarsGitHub Actions
test-conda-python-3.11-spark-masterGitHub Actions
verify-rc-source-java-linux-almalinux-8-amd64GitHub Actions
verify-rc-source-java-linux-conda-latest-amd64GitHub Actions
verify-rc-source-java-linux-ubuntu-20.04-amd64GitHub Actions
verify-rc-source-java-linux-ubuntu-22.04-amd64GitHub Actions
verify-rc-source-java-macos-amd64GitHub Actions

@vibhatha

Copy link
Copy Markdown
Contributor

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

@vibhathavibhatha reopened this Aug 12, 2024
@vibhatha

Copy link
Copy Markdown
Contributor

Still the CI is failing...

@vibhatha

Copy link
Copy Markdown
Contributor

Still failing right...

@koukou changed the title [GH-43506] Fix TestFragmentScanOptions result not matchGH-43506: [Java] Fix TestFragmentScanOptions result not matchAug 12, 2024
@github-actions

Copy link
Copy Markdown

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

@jinchengchenghh

Copy link
Copy Markdown
ContributorAuthor

I will take a look today.

@jinchengchenghh

Copy link
Copy Markdown
ContributorAuthor

Only gandiva failed now. @vibhatha

@vibhatha

Copy link
Copy Markdown
Contributor

@jinchengchenghh

Copy link
Copy Markdown
ContributorAuthor

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

@vibhatha

Copy link
Copy Markdown
Contributor

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

@kou

kou commented Aug 13, 2024

Copy link
Copy Markdown
Member

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

@vibhatha

Copy link
Copy Markdown
Contributor

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
Copy Markdown
ContributorAuthor

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
Copy Markdown
Contributor

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

@vibhatha

Copy link
Copy Markdown
Contributor

@jinchengchenghh let's disable this too.

@jinchengchenghh

Copy link
Copy Markdown
ContributorAuthor

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

@lidavidm

Copy link
Copy Markdown
Member

@github-actions crossbow submit -g java

@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Aug 14, 2024
@github-actions

Copy link
Copy Markdown

Revision: 855a3af

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

TaskStatus
java-jarsGitHub Actions
test-conda-python-3.11-spark-masterGitHub Actions
verify-rc-source-java-linux-almalinux-8-amd64GitHub Actions
verify-rc-source-java-linux-conda-latest-amd64GitHub Actions
verify-rc-source-java-linux-ubuntu-20.04-amd64GitHub Actions
verify-rc-source-java-linux-ubuntu-22.04-amd64GitHub Actions
verify-rc-source-java-macos-amd64GitHub Actions

@jinchengchenghh

Copy link
Copy Markdown
ContributorAuthor

Can you help merge this one? @vibhatha Thanks!

@vibhatha

Copy link
Copy Markdown
Contributor

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

@jinchengchenghh

Copy link
Copy Markdown
ContributorAuthor

Ok. Thanks for your kindly review. @vibhatha

@vibhatha

Copy link
Copy Markdown
Contributor

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

@jinchengchenghh

Copy link
Copy Markdown
ContributorAuthor

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

@vibhatha

Copy link
Copy Markdown
Contributor

Sure let's do that.

@lidavidm
lidavidm merged commit 5ef7e01 into apache:mainAug 18, 2024
@lidavidmlidavidm removed the awaiting merge Awaiting merge label Aug 18, 2024
@lidavidm

Copy link
Copy Markdown
Member

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

@vibhatha

Copy link
Copy Markdown
Contributor

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

@lidavidm

Copy link
Copy Markdown
Member

ok, let me dig

@vibhatha

Copy link
Copy Markdown
Contributor

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

@conbench-apache-arrow

Copy link
Copy Markdown

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.

pribor pushed a commit to GlobalWebIndex/arrow that referenced this pull request Oct 24, 2025
…pache#43639)
### 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
* GitHub Issue: apache#43506
Authored-by: Chengcheng Jin <chengcheng.jin@intel.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
Sign up for freeto 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

@jinchengchenghh@vibhatha@kou@lidavidm