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-38997: [Java] Modularize format and vector #38995

Merged

Conversation

jduo
Copy link
Member

@jduo jduo commented Nov 30, 2023

Rationale for this change

This PR depends on #39011 .

Make arrow-vector and arrow-format JPMS modules for improved compatibility with newer JDKs
and tools that require modules.

What changes are included in this PR?

  • add module-info.java files for vector and format.

Are these changes tested?

Yes, these modules are run as JPMS modules in JDK9+ unit tests.

Are there any user-facing changes?

Yes, some test classes have moved and users can now run these as JPMS modules.

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:/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:

@jduo jduo force-pushed the modularize-format-and-vector-and-add-plugin branch from 4947db9 to 54446ec Compare November 30, 2023 00:28
@jduo jduo changed the title Modularize format and vector and add plugin for compiling module-info files with JDK8 builds GH-38997: [Java] Modularize format and vector and add plugin for compiling module-info files with JDK8 builds Nov 30, 2023
Copy link

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

@jduo jduo marked this pull request as ready for review November 30, 2023 03:12
@jduo jduo requested a review from lidavidm as a code owner November 30, 2023 03:12
@jduo jduo force-pushed the modularize-format-and-vector-and-add-plugin branch from 0b49cfa to 83bab66 Compare December 1, 2023 12:36
@lidavidm
Copy link
Member

lidavidm commented Dec 4, 2023

This is on my backlog - I'm hoping to give this a review this week

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Dec 5, 2023
@jduo
Copy link
Member Author

jduo commented Dec 5, 2023

Thanks for the review @lidavidm . This shouldn't go in yet until we're ready to bite the bullet on modularizing memory-core + making the command-line change though (or we figure out how to avoid the command-line change). I'm changing this back to draft for the time being.

@jduo jduo marked this pull request as draft December 5, 2023 20:02
@jduo jduo force-pushed the modularize-format-and-vector-and-add-plugin branch from 83bab66 to 0342cb3 Compare December 7, 2023 23:41
@jduo jduo marked this pull request as ready for review December 8, 2023 00:23
@jduo jduo changed the title GH-38997: [Java] Modularize format and vector and add plugin for compiling module-info files with JDK8 builds GH-38997: [Java] Modularize format and vector Dec 8, 2023
@jduo jduo force-pushed the modularize-format-and-vector-and-add-plugin branch 3 times, most recently from 47030a0 to f39d07b Compare December 11, 2023 21:02
@jduo jduo force-pushed the modularize-format-and-vector-and-add-plugin branch 4 times, most recently from b99f597 to ccd5ffc Compare January 5, 2024 21:51
@jduo jduo force-pushed the modularize-format-and-vector-and-add-plugin branch from ccd5ffc to 038549a Compare January 5, 2024 22:24
@jduo jduo force-pushed the modularize-format-and-vector-and-add-plugin branch from 038549a to d0c891b Compare January 5, 2024 22:32
@jduo
Copy link
Member Author

jduo commented Jan 6, 2024

Note that this one didn't require doc changes.

@jduo jduo force-pushed the modularize-format-and-vector-and-add-plugin branch 2 times, most recently from cedbd00 to ae7bacb Compare January 9, 2024 19:22
@lidavidm
Copy link
Member

lidavidm commented Jan 9, 2024

@davisusanibar can you help take a look at the nullness annotation issues here?

@jduo jduo force-pushed the modularize-format-and-vector-and-add-plugin branch from ae7bacb to 1e7d5e9 Compare January 10, 2024 01:01
@jduo
Copy link
Member Author

jduo commented Jan 10, 2024

@davisusanibar can you help take a look at the nullness annotation issues here?

I fixed a null annotation issue with CountingAllocationListener in the parent patch (#39011).

@lidavidm
Copy link
Member

Ah whoops, I thought I was looking at that one.

@lidavidm
Copy link
Member

I think this just needs to be rebased?

Do not add tests in org.apache.arrow.util because
that is an exported package for arrow-memory-core
and causes module conflicts
Having an implicit dependency from arrow-memory-core
does not put immutables on the module-path when
running tests and causes module issues.
Export public package for arrow-vector module.
Allow jackson to use arrow-vector pojo classes
reflectively for serialization.
Needed because Vector classes use MemoryUtil.UNSAFE.
Also don't depend on jdk.unsupported
Needed because it uses MemoryUtil.UNSAFE
@jduo jduo force-pushed the modularize-format-and-vector-and-add-plugin branch from 1e7d5e9 to 94b9c20 Compare January 11, 2024 17:47
@jduo
Copy link
Member Author

jduo commented Jan 11, 2024

I think this just needs to be rebased?

Yes, this is rebased now @lidavidm

@jduo jduo merged commit 1a622ec into apache:main Jan 11, 2024
15 checks passed
Copy link

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

There were no benchmark performance regressions. 🎉

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

clayburn pushed a commit to clayburn/arrow that referenced this pull request Jan 23, 2024
### Rationale for this change
This PR depends on apache#39011 .

Make arrow-vector and arrow-format JPMS modules for improved
compatibility with newer JDKs and tools that require modules.

### What changes are included in this PR?
* add module-info.java files for vector and format.

### Are these changes tested?
Yes, these modules are run as JPMS modules in JDK9+ unit tests.

### Are there any user-facing changes?
Yes, some test classes have moved and users can now run these as JPMS
modules.

* Closes: apache#38997
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
### Rationale for this change
This PR depends on apache#39011 .

Make arrow-vector and arrow-format JPMS modules for improved
compatibility with newer JDKs and tools that require modules.

### What changes are included in this PR?
* add module-info.java files for vector and format.

### Are these changes tested?
Yes, these modules are run as JPMS modules in JDK9+ unit tests.

### Are there any user-facing changes?
Yes, some test classes have moved and users can now run these as JPMS
modules.

* Closes: apache#38997
zanmato1984 pushed a commit to zanmato1984/arrow that referenced this pull request Feb 28, 2024
### Rationale for this change
This PR depends on apache#39011 .

Make arrow-vector and arrow-format JPMS modules for improved
compatibility with newer JDKs and tools that require modules.

### What changes are included in this PR?
* add module-info.java files for vector and format.

### Are these changes tested?
Yes, these modules are run as JPMS modules in JDK9+ unit tests.

### Are there any user-facing changes?
Yes, some test classes have moved and users can now run these as JPMS
modules.

* Closes: apache#38997
thisisnic pushed a commit to thisisnic/arrow that referenced this pull request Mar 8, 2024
### Rationale for this change
This PR depends on apache#39011 .

Make arrow-vector and arrow-format JPMS modules for improved
compatibility with newer JDKs and tools that require modules.

### What changes are included in this PR?
* add module-info.java files for vector and format.

### Are these changes tested?
Yes, these modules are run as JPMS modules in JDK9+ unit tests.

### Are there any user-facing changes?
Yes, some test classes have moved and users can now run these as JPMS
modules.

* Closes: apache#38997
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.

[Java] Build arrow-format and arrow-vector as JPMS modules
2 participants