Code cleanups #15860

findepi · 2023-01-26T13:03:32Z

Instead of pointed changes in individual PRs, let's update the codebase at once.
Fewer commits => faster review overall.

raunaqmorarka · 2023-01-26T14:23:24Z

plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHiveFileFormats.java

@@ -750,10 +750,9 @@ protected void checkCursor(RecordCursor cursor, List<TestColumn> testColumns, in
                Type type = types.get(i);
                Object fieldFromCursor = getFieldFromCursor(cursor, type, i);
                if (fieldFromCursor == null) {
-                    assertEquals(null, testColumn.getExpectedValue(), "Expected null for column " + testColumn.getName());
+                    assertNull(testColumn.getExpectedValue(), "Expected null for column " + testColumn.getName());


seems unrelated to instanceof

my mistake, reverted

raunaqmorarka · 2023-01-26T14:23:35Z

plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHiveFileFormats.java

@@ -902,7 +901,7 @@ public TestColumn(
            this.writeValue = writeValue;
            this.expectedValue = expectedValue;
            this.partitionKey = partitionKey;
-            checkArgument(dereferenceNames.size() == 0 || partitionKey == false, "partial column cannot be a partition key");
+            checkArgument(dereferenceNames.size() == 0 || !partitionKey, "partial column cannot be a partition key");


seems unrelated to instanceof

raunaqmorarka · 2023-01-26T14:23:45Z

plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHiveFileFormats.java

-            sb.append(", partitionKey=").append(partitionKey);
-            sb.append('}');
-            return sb.toString();
+            String sb = "TestColumn{" + "baseName='" + baseName + "'" +


seems unrelated to instanceof

raunaqmorarka · 2023-01-26T14:27:16Z

core/trino-main/src/main/java/io/trino/sql/analyzer/Analysis.java

@@ -1152,8 +1152,8 @@ public List<TableInfo> getReferencedTables()



typo in commit message

thanks, fixed

There currently is no truly viable alternative, so it is ok to use the method in current and new code.

martint · 2023-01-26T18:42:07Z

core/trino-main/src/main/java/io/trino/sql/planner/iterative/Lookup.java

@@ -31,7 +31,6 @@
     * If the node is not a GroupReference, it returns the
     * argument as is.
     */
-    @Deprecated


The purpose of this is to discourage people from using this method or at least pause and think whether what they are trying to do can be done in a different way. Let’s keep it and add an explanation of why it’s marked this way.

Thanks for explaining this. However, the result is that we have warnings in the code, we become used to them, and that makes responding to legitimate IDE warnings less likely. Should we sprinkle the codebase with many @SuppressWarnings("deprecation"), wherever we use this method?

Any suggestions @martint how to mark this method in a way that doesn't trigger code-level warnings in IDEs?
(otherwise I am leaning towards merging as-is, I hope that's fine)

Btw, i've seen this having detrimental effect on new contributors. Old timers know they can use the method, but some of the newcomers simply don't want to use a deprecated method in new code. However, there is no existing code they can follow to do that. In retrospect, we deprecated the method a few years too early, too optimistically.
(2ef99f8, Aug 2017, cc @sopel39)

The issue with this is that the more rules we have using it, the harder it will be reverse in the future.

It seems something is broken with my Intellij code navigation. It shows me 51 code places using this method, and no code places using the alternative.

Old timers know they can use the method, but some of the newcomers simply don't want to use a deprecated method in new code.

That is precisely why we marked it as deprecated. Not because we were going to remove it soon, but to discourage using it for the common case where a pattern matcher can be used instead. Newcomers should pause, reach out and ask someone who has the context when they see no apparent alternative.

If there were another alternative to tag a method to say there are gotchas or warnings associated with it, we would use they instead. Sadly, there isn’t, so we’re left with using deprecation to signal this.

Newcomers should pause, reach out and ask someone who has the context when they see no apparent alternative.

This method was already used in many places, and the code comment here doesn't suggest any alternatives like a deprecated method is supposed to. So to someone new, this "deprecation" always looked like either a mistake or an incomplete work that somebody forgot to follow up on.
There might have been lesser confusion if the actual intent behind this was clarified in the code comment in simple language like it is in this thread.

Indeed. That’s why I suggested adding such explanation above.

findepi · 2023-01-27T08:12:44Z

CI #12535

findepi · 2023-01-27T08:16:00Z

CI some product tests failed with apparently no logs left behind

sopel39 · 2023-01-27T11:22:55Z

core/trino-main/src/main/java/io/trino/sql/planner/iterative/Lookup.java

@@ -31,7 +31,6 @@
     * If the node is not a GroupReference, it returns the
     * argument as is.
     */
-    @Deprecated


The issue with this is that the more rules we have using it, the harder it will be reverse in the future.

findepi · 2023-01-27T12:19:57Z

CI #11344

findepi · 2023-01-27T13:37:22Z

CI #5427 (-> #15879)

Remove unused method

0f16e4c

findepi added the no-release-notes This pull request does not require release notes entry label Jan 26, 2023

findepi requested review from raunaqmorarka and ebyhr January 26, 2023 13:03

cla-bot bot added the cla-signed label Jan 26, 2023

findepi added maintenance Project maintenance task dx Issues or pull requests related to developer experience labels Jan 26, 2023

raunaqmorarka approved these changes Jan 26, 2023

View reviewed changes

github-actions bot added docs tests:hive labels Jan 26, 2023

findepi added 7 commits January 26, 2023 17:17

Use enhanced instanceof

faf1d2a

Annotate QueryAssert fluent methods

b0fd345

Allow calls to resolve

34d6ca9

There currently is no truly viable alternative, so it is ok to use the method in current and new code.

Replace lambda with method reference

f5bc78e

Remove unused CaseAggregation.projectionSymbol field

c6073bf

Simplify Stream.forEach where stream redundant

8dea140

Simplify Map values iteration

acdba18

findepi force-pushed the findepi/instance_tab_tab-of branch from cdbad05 to acdba18 Compare January 26, 2023 16:17

martint reviewed Jan 26, 2023

View reviewed changes

empty

eac2256

findepi changed the title ~~Use enhanced instanceof~~ Code cleanups Jan 27, 2023

sopel39 approved these changes Jan 27, 2023

View reviewed changes

findepi merged commit 208b380 into trinodb:master Jan 27, 2023

findepi deleted the findepi/instance_tab_tab-of branch January 27, 2023 13:37

github-actions bot added this to the 407 milestone Jan 27, 2023

colebow mentioned this pull request Jan 31, 2023

Add Trino 407 release notes #15919

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code cleanups #15860

Code cleanups #15860

findepi commented Jan 26, 2023

raunaqmorarka Jan 26, 2023

findepi Jan 26, 2023

raunaqmorarka Jan 26, 2023

raunaqmorarka Jan 26, 2023

raunaqmorarka Jan 26, 2023

findepi Jan 26, 2023

martint Jan 26, 2023

findepi Jan 26, 2023

findepi Jan 26, 2023

findepi Jan 27, 2023

sopel39 Jan 27, 2023

findepi Jan 27, 2023

martint Jan 27, 2023

raunaqmorarka Jan 27, 2023

martint Jan 27, 2023

findepi commented Jan 27, 2023 •

edited

Loading

findepi commented Jan 27, 2023

sopel39 Jan 27, 2023

findepi commented Jan 27, 2023

findepi commented Jan 27, 2023 •

edited

Loading

		@@ -1152,8 +1152,8 @@ public List<TableInfo> getReferencedTables()

Code cleanups #15860

Code cleanups #15860

Conversation

findepi commented Jan 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findepi commented Jan 27, 2023 • edited Loading

findepi commented Jan 27, 2023

Choose a reason for hiding this comment

findepi commented Jan 27, 2023

findepi commented Jan 27, 2023 • edited Loading

findepi commented Jan 27, 2023 •

edited

Loading

findepi commented Jan 27, 2023 •

edited

Loading