Skip to content

build(isthmus-cli): use GraalVM 25#515

Merged
bestbeforetoday merged 1 commit into
substrait-io:mainfrom
bestbeforetoday:graal25
Sep 22, 2025
Merged

build(isthmus-cli): use GraalVM 25#515
bestbeforetoday merged 1 commit into
substrait-io:mainfrom
bestbeforetoday:graal25

Conversation

@bestbeforetoday

@bestbeforetoday bestbeforetoday commented Sep 19, 2025

Copy link
Copy Markdown
Member

Benefits from Java 25 performance optimizations.

Closes #218

@bestbeforetoday
bestbeforetoday marked this pull request as ready for review September 19, 2025 14:07
@nielspardon

Copy link
Copy Markdown
Member

Also resolves an issue where SLF4J log statements were output by the CLI command since the JDK 1.4 logging implementation was specified as a runtime dependency.

This makes me wonder what the behavior should be. Should the CLI have a --debug or --verbose flag to control whether and which log messages should be printed instead of dropping them?

@bestbeforetoday

bestbeforetoday commented Sep 19, 2025

Copy link
Copy Markdown
Member Author

This makes me wonder what the behavior should be. Should the CLI have a --debug or --verbose flag to control whether and which log messages should be printed instead of dropping them?

It seems to me pretty unusual for a CLI command to output logging information to the console by default. I could revert it to behave that way again while we think about it, or we could merge this as-is and think about it. Removing the console logging made it slightly easier for me to diff output before and after the GraalVM version change to ensure behaviour was the same. The logging behaviour is separate to the use of the latest GraalVM so just let me know what you prefer either way.

@nielspardon

Copy link
Copy Markdown
Member

It seems to me pretty unusual for a CLI command to output logging information to the console by default.

I think it depends on what CLI commands you take into consideration. If you think of something like grep or jq then it is unusual. If you think of mvn, gradle then it is not so unusual to get log messages printed to the stdout. It feels to me that the logging behavior for the CLI could probably be improved though it feels to me like that is a separate concern from updating the Graal version and maybe should be handled in a separate PR. I do understand why it was useful for your validation of the Graal version update. I'm not sure whether the change as-is with dropping all log messages is what in general we want the CLI behavior to be or whether it should be more configurable via various flags.

Benefits from Java 25 performance optimizations.

Signed-off-by: Mark S. Lewis <Mark.S.Lewis@outlook.com>
@bestbeforetoday

Copy link
Copy Markdown
Member Author

@nielspardon I have removed the logging change.

@nielspardon nielspardon 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.

LGTM

@bestbeforetoday
bestbeforetoday merged commit 5c91d87 into substrait-io:main Sep 22, 2025
12 checks passed
@bestbeforetoday
bestbeforetoday deleted the graal25 branch September 22, 2025 09:18
ZorinAnton pushed a commit to ZorinAnton/substrait-java that referenced this pull request Sep 23, 2025
Benefits from Java 25 performance optimizations.

Signed-off-by: Mark S. Lewis <Mark.S.Lewis@outlook.com>
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.

[Isthmu][CLI][GraalVM] upgrade to latest GraalVM

2 participants