Make it explicit that metrics reporter is required

[findepi]

No description provided.

[nastra]

Suggested change

	Preconditions.checkNotNull(reporter, "reporter cannot be null");
	Preconditions.checkArgument(null != reporter, Invalid metrics reporter: null");

[findepi]

it seems the codebase is generally OK using checkNotNull for null checks

$ git grep 'Preconditions.checkNotNull' | wc -l
392

are you saying this is passé?

[nastra]

IMO we should prefer Preconditions.checkArgument here, because it indicates that a wrong argument has been passed and is therefore more expressive. Also we generally use Preconditions.checkArgument more than Preconditions.checkNotNull:

git grep 'Preconditions.checkArgument' | wc -l                                                                                                                                                                                                
1542

[findepi]

Also we generally use Preconditions.checkArgument more than Preconditions.checkNotNull:

do we use it commonly to check for nullness?

i think in the code i've contributed so far to iceberg i used cNN and don't remember being turned around to use cA

[jackye1995]

yeah as Piotr suggested, it seems like there are multiple places we already use cNN for this, so I am fine with continuing to use that. @nastra what do you think?

[nastra]

the reason why I made the suggestion is mainly because the error msg should be updated to Invalid metrics reporter: null. Additionally I checked the code around table scans and scan metrics. Since those places all use Preconditions.checkArgument it seems more appropriate to use the same here IMO.

[rdblue]

+1 for @nastra's comment about the error message.

Whether we use checkNotNull or checkArgument doesn't really matter because both are ArgumentException. The problem with this is that it makes an unnecessary change to have an error message that doesn't fit with the conventions used elsewhere.

[jackye1995]

looks good to me, would be good if we reach some consensus about the way for null check, and change that to be consistent across codebase and add corresponding checkstyle rules.

[findepi]

looks good to me

thanks for your review @jackye1995 !

would be good if we reach some consensus about the way for null check

i prefer checkNotNull, but it's up for the project to decide
i just hope this future decision isn't required for the merge here

BTW, I am the library's user more frequently than its contributor. As a user, I would benefit from explicit indication which API methods accept nulls, and even more -- which can return nulls.
We keep asking "can this be null" question in PR reviews (eg here trinodb/trino#14869 (comment))
but more often than not, we forget to ask that question, potentially setting us to NPE failures.

This too, I would prefer to be a follow up, not a prerequisite for a merge here.

[nastra]

BTW, I am the library's user more frequently than its contributor. As a user, I would benefit from explicit indication which API methods accept nulls, and even more -- which can return nulls. We keep asking "can this be null" question in PR reviews (eg here trinodb/trino#14869 (comment)) but more often than not, we forget to ask that question, potentially setting us to NPE failures.

This too, I would prefer to be a follow up, not a prerequisite for a merge here.

I agree that it would improve things when it's more obvious that certain things can/cannot be null, e.g. through @Nullable. Maybe this is something we should mention on the ML to raise awareness?

Re: cNN vs cA I think it's fine to go ahead and get this merged with what you prefer (assuming the error message is updated as mentioned above) as we don't want to block things from getting in while this is discussed.

[jackye1995]

We keep asking "can this be null" question in PR reviews

Yes this is the consequence of different styles of the projects. I like Trino's approach of checking null at every constructor and also use Optional as much as possible. For Iceberg, it would be ideal if nullability is at least indicated in documentation.

[jackye1995]

I will merge this PR as suggested, and will create a Github issue for further discussion.

[findepi]

Yes this is the consequence of different styles of the projects.

That's a good point.
I accept the inherent friction being result of that, but I do hope some of that friction is avoidable.

I like Trino's approach of checking null at every constructor and also use Optional as much as possible.

I like it too. That's why I fully supported @kokosing's push for to leverage Optional whenever possible.
There are still some rare places that would benefit from improvement, but we're generally in quite a good shape with null-unfriendliness.

For Iceberg, it would be ideal if nullability is at least indicated in documentation.

100% agreed, especially if you mean API-level documentation (javadoc, annotations), as that's what library users will see in their IDEs.

[findepi]

thanks for the merge!