Skip to content

style: replace RuntimeException with more specific exceptions#437

Merged
bestbeforetoday merged 5 commits into
substrait-io:mainfrom
nielspardon:par-runtime-exception
Jul 24, 2025
Merged

style: replace RuntimeException with more specific exceptions#437
bestbeforetoday merged 5 commits into
substrait-io:mainfrom
nielspardon:par-runtime-exception

Conversation

@nielspardon
Copy link
Copy Markdown
Member

@nielspardon nielspardon commented Jul 13, 2025

replaces throwing a generic RuntimeException with a more specific exception like IllegalStateException, IllegalArgumentException or UnsupportedOperationException

@nielspardon nielspardon force-pushed the par-runtime-exception branch from 84bfbf1 to 3111d67 Compare July 15, 2025 09:33
bestbeforetoday
bestbeforetoday reviewed Jul 17, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@bestbeforetoday bestbeforetoday left a comment

Choose a reason for hiding this comment

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

One suggestion. Otherwise this looks good to me.

Comment thread core/src/main/java/io/substrait/extension/SimpleExtension.java Outdated
@nielspardon nielspardon force-pushed the par-runtime-exception branch from 3111d67 to c5dd15c Compare July 17, 2025 14:09
@nielspardon
Copy link
Copy Markdown
Member Author

nielspardon commented Jul 17, 2025

rebased on main and added a commit that uses UncheckedIOException

bestbeforetoday
bestbeforetoday approved these changes Jul 17, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@bestbeforetoday bestbeforetoday 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. It is yet another PR that looks like it will introduce merge conflicts with #433 though.

@nielspardon nielspardon force-pushed the par-runtime-exception branch from d721d32 to 19189ef Compare July 18, 2025 08:24
@nielspardon
Copy link
Copy Markdown
Member Author

nielspardon commented Jul 18, 2025

I pushed a commit which adds a substrait.java-conventions Gradle plugin which configures PMD to check the AvoidThrowingRawExceptionTypes rule

@nielspardon nielspardon force-pushed the par-runtime-exception branch from 19189ef to 483c590 Compare July 23, 2025 06:54
bestbeforetoday
bestbeforetoday requested changes Jul 23, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@bestbeforetoday bestbeforetoday left a comment

Choose a reason for hiding this comment

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

One import issue (see comment), but otherwise looks good to me.

Comment thread isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java Outdated
Comment on lines 3 to 4
import static io.substrait.isthmus.SqlConverterBase.EXTENSION_COLLECTION;
import static io.substrait.isthmus.SqlToSubstrait.EXTENSION_COLLECTION;
Copy link
Copy Markdown
Member

@bestbeforetoday bestbeforetoday Jul 23, 2025

Choose a reason for hiding this comment

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

I am surprised there wasn't a compile error with two static imports of the same name. I guess because they actually refer to the same constant. I think there should only be one import here though.

Copy link
Copy Markdown
Member Author

@nielspardon nielspardon Jul 23, 2025

Choose a reason for hiding this comment

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

thanks, this is a bit annoying. this has been happening a couple of times now. I think it's VSCode somehow understanding that EXTENSION_COLLECTION is actually defined in SqlConverterBase which SqlToSubstrait inherits from.

Copy link
Copy Markdown
Member Author

@nielspardon nielspardon Jul 23, 2025

Choose a reason for hiding this comment

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

I pushed a commit switching to SqlConverterBase.EXTENSION_COLLECTION consistently which should help to avoid having VSCode change this line over and over again

bestbeforetoday
bestbeforetoday approved these changes Jul 23, 2025
View reviewed changes
Copy link
Copy Markdown
Member

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

@nielspardon nielspardon force-pushed the par-runtime-exception branch 3 times, most recently from 3ec87a6 to 39bc8f6 Compare July 24, 2025 18:26
@nielspardon
style: replace RuntimeException with more specific exceptions
a3d8320 
Signed-off-by: Niels Pardon <par@zurich.ibm.com>
@nielspardon
style: use UncheckedIOException
59fdc2a 
Signed-off-by: Niels Pardon <par@zurich.ibm.com>
@nielspardon
build: check PMD rule AvoidThrowingRawExceptionTypes
9f68b16 
Signed-off-by: Niels Pardon <par@zurich.ibm.com>
@nielspardon
chore: fix EXTENSION_COLLECTION import
14d8d91 
Signed-off-by: Niels Pardon <par@zurich.ibm.com>
@nielspardon nielspardon force-pushed the par-runtime-exception branch from 39bc8f6 to 14d8d91 Compare July 24, 2025 19:41
@nielspardon
chore: fix Gradle build dependency
135961c 
Signed-off-by: Niels Pardon <par@zurich.ibm.com>
@bestbeforetoday bestbeforetoday merged commit 441ba4c into substrait-io:main Jul 24, 2025
12 checks passed
@nielspardon nielspardon deleted the par-runtime-exception branch August 5, 2025 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bestbeforetoday bestbeforetoday bestbeforetoday approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@nielspardon @bestbeforetoday