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

build: Support built with java 1.8 #45

Merged
merged 2 commits into from
Feb 21, 2024

Conversation

advancedxy
Copy link
Contributor

Which issue does this PR close?

Closes #43

Rationale for this change

Add support for java 1.8

What changes are included in this PR?

  1. downgrade io.github.git-commit-id to 4.9.9 to be compatible with java 1.8
  2. change GenTPCHData.scala to be compatible with java 1.8
  3. make spotless happy with both java 1.8 and java 17(or even java 11)
  4. amend CI actions to run with multiple java versions

How are these changes tested?

updated ci runs.

Copy link
Contributor Author

@advancedxy advancedxy left a comment

Choose a reason for hiding this comment

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

@sunchao @viirya please take a look when you have time.

This changes to actual code should be small.

.github/actions/java-test/action.yaml Outdated Show resolved Hide resolved
.github/actions/rust-test/action.yaml Show resolved Hide resolved
.github/actions/setup-builder/action.yaml Show resolved Hide resolved
@@ -76,7 +76,7 @@ object GenTPCHData {
// Generate data
// Since dbgen may uses stdout to output the data, tables.genData needs to run table by table
val tableNames =
if (config.tableFilter.isBlank) tables.tables.map(_.name) else Seq(config.tableFilter)
if (config.tableFilter.trim.isEmpty) tables.tables.map(_.name) else Seq(config.tableFilter)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

java8 compatible.

Copy link

Choose a reason for hiding this comment

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

How about org.apache.commons.lang3.StringUtils.isBlank?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

trim.isBlank is sufficient. I don't think we should use StringUtils.

Copy link
Contributor

Choose a reason for hiding this comment

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

In general I would avoid using apache commons if the java standard library has to way to achieve the same. Unless of course there is a measurable performance gain :).

@sunchao sunchao changed the title support built with java 1.8 build: Support built with java 1.8 Feb 19, 2024
Copy link
Member

@viirya viirya left a comment

Choose a reason for hiding this comment

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

CI failure looks like due to #41.

@advancedxy
Copy link
Contributor Author

CI passes, please let me if there's any other issues to be addressed.

.github/actions/java-test/action.yaml Outdated Show resolved Hide resolved
.github/workflows/pr_build.yml Outdated Show resolved Hide resolved
common/pom.xml Show resolved Hide resolved
@advancedxy
Copy link
Contributor Author

All cleared. cc @sunchao @viirya

runs-on: macos-14
env:
JAVA_VERSION: ${{ matrix.java_version == 8 && '1.8' || format('{0}', matrix.java_version) }}
Copy link
Member

Choose a reason for hiding this comment

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

This syntax matrix.java_version == 8 && '1.8' looks weird. It is evaluated to '1.8'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, see this example: https://docs.github.com/en/actions/learn-github-actions/expressions#example

And also the actual log:
image

Co-authored-by: Liang-Chi Hsieh <[email protected]>
Copy link
Member

@viirya viirya left a comment

Choose a reason for hiding this comment

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

Looks good to me. See if @sunchao has more comments or not.

Copy link
Member

@sunchao sunchao left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@sunchao sunchao merged commit 1acb56f into apache:main Feb 21, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support build with Java 1.8
5 participants