[CALCITE-2780] Replace UnmodifiableArrayList with JDK Functions#997
[CALCITE-2780] Replace UnmodifiableArrayList with JDK Functions#997belugabehr wants to merge 1 commit intoapache:masterfrom belugabehr:CALCITE-2780
Conversation
| public List<SqlNode> getOperandList() { | ||
| return UnmodifiableArrayList.of(operands); // not immutable, but quick | ||
| // not immutable, but quick | ||
| return Collections.unmodifiableList(Arrays.asList(operands)); |
There was a problem hiding this comment.
Maybe worth adding the pattern to Util?
public static <T> List<T> Util.unmodifiableList(T[] elems)
There was a problem hiding this comment.
I wouldn't recommend it. You then expect others to have to know that such a utility exists and it will end up with some mixture of both.
There was a problem hiding this comment.
Util is used all over the place and provides lots of common functions regarding Collections.
If the concern is code consistency and preventing style mixture, maybe we should look at how to prevent "bad patterns" using checkstyle and similar checker tools...
There was a problem hiding this comment.
Thanks for the feedback. If that is what is required to get this change acceptable, then I will oblige. Just let me know.
If you want this change, what level of unit tests would you like?
There was a problem hiding this comment.
I don't see extra use of Collections.unmodifiableList() to wrap arrays in code base, and the pattern was already to use UnmodifiableArrayList, so I don't think the codebase have a consistency issue. Also Util usage is fairly common in Calcite code base, and it is kind of expected for people to look at that class for any utility method.
As for unit test, just reuse the same as previously? I think it should work as-is
|
@laurentgo I had to take a bit of a different tact to implement it as you requested. Take a look. Thanks. |
qinghui-xu
left a comment
qinghui-xu
left a comment
There was a problem hiding this comment.
It seems there is some code style issue.
| }; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Code style: replace tab by using spaces.
|
Closing since JIRA closed |
4 participants
Less is more.