Upgrade Error Prone to 2.18.0 #14597

ksobolew · 2022-10-12T16:32:11Z

Description

In 2.16.0 there's a new checker, LenientFormatStringValidation, ERROR by default, which verifies some more format strings in non-standard libraries (like Guava), and it triggers in several places.

In 2.17.0 there's a new checker, ImpossibleNullComparison, which triggers on some null checks of the return value from Multimap#get, which cannot be null.

In 2.18.0 the UnusedVariable checker reports unused parameters on Guava methods (@Inject or @Provides).

Non-technical explanation

Improved code quality. (Possibly preventing crashes resulting from invalid format strings in some error conditions.)

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

lib/trino-record-decoder/src/main/java/io/trino/decoder/raw/RawColumnDecoder.java

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveType.java

core/trino-main/src/main/java/io/trino/operator/output/PagePartitioner.java

core/trino-main/src/test/java/io/trino/sql/planner/assertions/ExpressionMatcher.java

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveType.java

ksobolew · 2022-10-13T12:05:15Z

Thanks @findepi, addressed comments

findepi · 2022-10-13T15:05:51Z

@ksobolew why rebase?
what changed?

ksobolew · 2022-10-13T15:42:58Z

Oh, I always forget to not rebase reflexively. Sorry. I just removed one import obsoleted by the second commit.

ksobolew · 2022-10-14T07:53:16Z

Just rebased on top of latest master to pull in fixes to the build scripts

findepi · 2022-10-14T10:38:49Z

the build is not green

ksobolew · 2022-10-14T15:46:30Z

ci / test (core/trino-main):

Error:  Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.0.0-M5:test (default-test) on project trino-main: Execution default-test of goal org.apache.maven.plugins:maven-surefire-plugin:3.0.0-M5:test failed: Cannot invoke "java.io.File.getAbsolutePath()" because the return value of "org.apache.maven.artifact.Artifact.getFile()" is null -> [Help 1]

Some kind of Maven issue - I've seen it before, but I have no idea what it is.

ci / test (plugin/trino-hive) is #14631, already fixed (needs rebase).

ci / pt (default, suite-2, ) (weird task name, BTW) is a timeout? (Failed after almost exactly 2 hours)

ksobolew · 2022-10-19T14:27:27Z

Can we merge it now?

ksobolew · 2023-01-05T11:26:05Z

Error Prone 2.17.0 was released a couple of days ago; I merged this into this PR.

pom.xml

There's a new checker in Error Prone 2.16.0, `LenientFormatStringValidation`, ERROR by default, which verifies some more format strings in non-standard libraries (like Guava), and it triggers in several places.

This whole expression was evaluated eagerly for each check, even though it was basically never used. Calling `toString()` on the raw list does the same thing anyway.

ksobolew · 2023-01-10T17:50:16Z

Pulled the last commit from #15654

wendigo · 2023-01-11T14:12:55Z

core/trino-main/src/main/java/io/trino/execution/scheduler/StageTaskSourceFactory.java

@@ -905,7 +905,8 @@ private static ListMultimap<PlanNodeId, ExchangeSourceHandle> getInputsForRemote
        ImmutableListMultimap.Builder<PlanNodeId, ExchangeSourceHandle> result = ImmutableListMultimap.builder();
        for (RemoteSourceNode remoteSource : remoteSources) {
            for (PlanFragmentId fragmentId : remoteSource.getSourceFragmentIds()) {
-                Collection<ExchangeSourceHandle> handles = requireNonNull(exchangeSourceHandles.get(fragmentId), () -> "exchange source handle is missing for fragment: " + fragmentId);
+                Collection<ExchangeSourceHandle> handles = exchangeSourceHandles.get(fragmentId);
+                checkArgument(!handles.isEmpty(), "exchange source handle is missing for fragment: %s", fragmentId);


I'd actually drop that. Previous version wouldn't throw on empty collection - this does. It's a change in the behavior.

I've asked @losipiuk and according to his understanding dropping an RNN is a correct one. We could ask @arhimondr for an opinion though

Sure, I can drop that. It just looked like it's the intent of the original code.

I do not see a reason why we should expect non-empty handles here. I may be wrong though. @arhimondr ?

I would probably just drop this check

`Multimap#get` cannot return null; replace the null check with emptiness check instead. Error Prone 2.17.0 check: `ImpossibleNullComparison`.

This new version triggers on a bunch of unused parameters of Guava methods (`@Inject` or `@Provides`).

cla-bot bot added the cla-signed label Oct 12, 2022

ksobolew requested review from findepi and nineinchnick October 12, 2022 16:32

github-actions bot added the tests:hive label Oct 12, 2022

ksobolew force-pushed the kudi/error-prone-2.16 branch from cc82942 to 6380421 Compare October 13, 2022 09:17

nineinchnick reviewed Oct 13, 2022

View reviewed changes

lib/trino-record-decoder/src/main/java/io/trino/decoder/raw/RawColumnDecoder.java Show resolved Hide resolved

nineinchnick approved these changes Oct 13, 2022

View reviewed changes

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveType.java Outdated Show resolved Hide resolved

ksobolew force-pushed the kudi/error-prone-2.16 branch from 6380421 to c13addb Compare October 13, 2022 09:31

findepi reviewed Oct 13, 2022

View reviewed changes

core/trino-main/src/main/java/io/trino/operator/output/PagePartitioner.java Show resolved Hide resolved

findepi reviewed Oct 13, 2022

View reviewed changes

core/trino-main/src/test/java/io/trino/sql/planner/assertions/ExpressionMatcher.java Outdated Show resolved Hide resolved

findepi reviewed Oct 13, 2022

View reviewed changes

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveType.java Outdated Show resolved Hide resolved

findepi reviewed Oct 13, 2022

View reviewed changes

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveType.java Outdated Show resolved Hide resolved

ksobolew force-pushed the kudi/error-prone-2.16 branch from c13addb to 40f127b Compare October 13, 2022 12:05

findepi approved these changes Oct 13, 2022

View reviewed changes

ksobolew force-pushed the kudi/error-prone-2.16 branch from 40f127b to 69a6e1e Compare October 13, 2022 15:02

ksobolew force-pushed the kudi/error-prone-2.16 branch from 69a6e1e to 5b37621 Compare October 14, 2022 07:52

ksobolew force-pushed the kudi/error-prone-2.16 branch from 5b37621 to 98a6798 Compare October 14, 2022 15:47

findepi added the no-release-notes This pull request does not require release notes entry label Oct 17, 2022

ksobolew force-pushed the kudi/error-prone-2.16 branch 2 times, most recently from eaa07b2 to 8efcd6a Compare December 15, 2022 10:38

martint force-pushed the master branch from 40dbb4f to 0d73d10 Compare December 19, 2022 20:15

ksobolew force-pushed the kudi/error-prone-2.16 branch from 8efcd6a to 0743aad Compare January 5, 2023 11:21

ksobolew changed the title ~~Upgrade Error Prone to 2.16.0~~ Upgrade Error Prone to 2.17.0 Jan 5, 2023

ksobolew force-pushed the kudi/error-prone-2.16 branch from 0743aad to 3b2e0bf Compare January 5, 2023 11:23

ksobolew requested review from findepi and nineinchnick January 5, 2023 11:25

nineinchnick approved these changes Jan 5, 2023

View reviewed changes

findepi reviewed Jan 5, 2023

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

ksobolew force-pushed the kudi/error-prone-2.16 branch from 3b2e0bf to 6a4e38a Compare January 5, 2023 13:04

ksobolew mentioned this pull request Jan 10, 2023

Update error-prone to 2.18.0 #15654

Closed

ksobolew added 2 commits January 10, 2023 18:25

Fix malformed format strings in Guava's methods like verify()

ae25494

There's a new checker in Error Prone 2.16.0, `LenientFormatStringValidation`, ERROR by default, which verifies some more format strings in non-standard libraries (like Guava), and it triggers in several places.

Simplify one invariant check

fe8bb5e

This whole expression was evaluated eagerly for each check, even though it was basically never used. Calling `toString()` on the raw list does the same thing anyway.

ksobolew force-pushed the kudi/error-prone-2.16 branch from 6a4e38a to 9a95f49 Compare January 10, 2023 17:49

ksobolew requested review from findepi, nineinchnick and wendigo January 10, 2023 17:50

ksobolew changed the title ~~Upgrade Error Prone to 2.17.0~~ Upgrade Error Prone to 2.18.0 Jan 10, 2023

wendigo requested a review from kokosing January 11, 2023 14:10

wendigo reviewed Jan 11, 2023

View reviewed changes

wendigo approved these changes Jan 11, 2023

View reviewed changes

wendigo requested a review from losipiuk January 12, 2023 15:11

ksobolew and others added 2 commits January 12, 2023 16:29

Fix ineffective null check

789c976

`Multimap#get` cannot return null; replace the null check with emptiness check instead. Error Prone 2.17.0 check: `ImpossibleNullComparison`.

Update error-prone to 2.18.0

2e13d5b

This new version triggers on a bunch of unused parameters of Guava methods (`@Inject` or `@Provides`).

losipiuk force-pushed the kudi/error-prone-2.16 branch from 9a95f49 to 2e13d5b Compare January 12, 2023 15:29

losipiuk approved these changes Jan 12, 2023

View reviewed changes

findepi approved these changes Jan 12, 2023

View reviewed changes

losipiuk merged commit 462ced0 into trinodb:master Jan 12, 2023

github-actions bot added this to the 406 milestone Jan 12, 2023

colebow mentioned this pull request Jan 12, 2023

Add Trino 406 release notes #15625

Merged

ksobolew deleted the kudi/error-prone-2.16 branch January 13, 2023 09:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade Error Prone to 2.18.0 #14597

Upgrade Error Prone to 2.18.0 #14597

ksobolew commented Oct 12, 2022 •

edited

Loading

ksobolew commented Oct 13, 2022

findepi commented Oct 13, 2022

ksobolew commented Oct 13, 2022

ksobolew commented Oct 14, 2022

findepi commented Oct 14, 2022

ksobolew commented Oct 14, 2022

ksobolew commented Oct 19, 2022

ksobolew commented Jan 5, 2023

ksobolew commented Jan 10, 2023

wendigo Jan 11, 2023

ksobolew Jan 11, 2023

losipiuk Jan 12, 2023

arhimondr Jan 12, 2023

losipiuk Jan 12, 2023

Upgrade Error Prone to 2.18.0 #14597

Upgrade Error Prone to 2.18.0 #14597

Conversation

ksobolew commented Oct 12, 2022 • edited Loading

Description

Non-technical explanation

Release notes

ksobolew commented Oct 13, 2022

findepi commented Oct 13, 2022

ksobolew commented Oct 13, 2022

ksobolew commented Oct 14, 2022

findepi commented Oct 14, 2022

ksobolew commented Oct 14, 2022

ksobolew commented Oct 19, 2022

ksobolew commented Jan 5, 2023

ksobolew commented Jan 10, 2023

wendigo Jan 11, 2023

Choose a reason for hiding this comment

ksobolew Jan 11, 2023

Choose a reason for hiding this comment

losipiuk Jan 12, 2023

Choose a reason for hiding this comment

arhimondr Jan 12, 2023

Choose a reason for hiding this comment

losipiuk Jan 12, 2023

Choose a reason for hiding this comment

ksobolew commented Oct 12, 2022 •

edited

Loading