findepi commented on PR #6474: URL: https://github.com/apache/iceberg/pull/6474#issuecomment-1396954141
> 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 https://github.com/trinodb/trino/pull/14869#discussion_r1072388756) 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
