Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code cleanups #15860

Merged
merged 9 commits into from
Jan 27, 2023
Merged

Code cleanups #15860

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0f16e4c
Remove unused method
findepi Jan 26, 2023
faf1d2a
Use enhanced instanceof
findepi Jan 26, 2023
b0fd345
Annotate QueryAssert fluent methods
findepi Jan 26, 2023
34d6ca9
Allow calls to resolve
findepi Jan 26, 2023
f5bc78e
Replace lambda with method reference
findepi Jan 26, 2023
c6073bf
Remove unused CaseAggregation.projectionSymbol field
findepi Jan 26, 2023
8dea140
Simplify Stream.forEach where stream redundant
findepi Jan 26, 2023
acdba18
Simplify Map values iteration
findepi Jan 26, 2023
eac2256
empty
findepi Jan 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion core/trino-main/src/main/java/io/trino/sql/planner/iterative/Lookup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ public interface Lookup
* If the node is not a GroupReference, it returns the
* argument as is.
*/
@Deprecated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

default PlanNode resolve(PlanNode node)
{
if (node instanceof GroupReference) {
Expand Down