uros-b commented on PR #17484: URL: https://github.com/apache/iceberg/pull/17484#issuecomment-5507347785
Thank you for the comments folks! As @szehon-ho noted above - replaceAll takes a regex, so using it for these literals isn't just the question of micro perf, but it's semantically the wrong method here and a bit misleading at the call site. The change proposed in this PR preserves intended behavior and keeps the PR pretty scoped. Going one step further, I'd be happy to also do what @RussellSpitzer suggested and add a checkstyle rule so we lock this out permanently rather than leaving it as a one-off. This will be good for the codebase going forward. We already have 25+ RegexpSinglelineJava rules of this shape, so this would fit the existing pattern. I'll go ahead and resolve the conflicts and introduce the rule, and then I think this PR will be in a good state, without too much churn. Hope this makes sense, please LMK what you think -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
