[GitHub] jihoonson commented on a change in pull request #6068: Prohibit Lists.newArrayList() with a single argument
jihoonson commented on a change in pull request #6068: Prohibit
Lists.newArrayList() with a single argument
URL: https://github.com/apache/incubator-druid/pull/6068#discussion_r206277090
##
File path:
indexing-service/src/main/java/io/druid/indexing/common/task/Tasks.java
##
@@ -74,7 +73,8 @@
toBeAccumulated.add(interval);
} else {
compactIntervals.add(JodaUtils.umbrellaInterval(toBeAccumulated));
- toBeAccumulated = Lists.newArrayList(interval);
+ toBeAccumulated.clear();
Review comment:
Oh, you're correct. This looks fine.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] jihoonson commented on a change in pull request #6068: Prohibit Lists.newArrayList() with a single argument
jihoonson commented on a change in pull request #6068: Prohibit Lists.newArrayList() with a single argument URL: https://github.com/apache/incubator-druid/pull/6068#discussion_r206274914 ## File path: processing/src/main/java/io/druid/collections/spatial/Node.java ## @@ -56,7 +57,7 @@ public Node(float[] minCoordinates, float[] maxCoordinates, boolean isLeaf, Bitm public Node( float[] minCoordinates, float[] maxCoordinates, - List children, + @Nullable Node child, Review comment: I don't have a strong opinion here, but I think anyone would wonder why this `Node` accepts only a single child in its constructor if he is not familiar with this part because they would know what the tree is and usually a node can have multiple children. I think that BDUF and YAGNI are the concepts to give some basic guidance and we should be able to apply it flexibly. Anyway, if you want to keep this, please add a comment why it currently accepts a single child. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] jihoonson commented on a change in pull request #6068: Prohibit Lists.newArrayList() with a single argument
jihoonson commented on a change in pull request #6068: Prohibit
Lists.newArrayList() with a single argument
URL: https://github.com/apache/incubator-druid/pull/6068#discussion_r206272902
##
File path:
indexing-service/src/main/java/io/druid/indexing/common/task/Tasks.java
##
@@ -74,7 +73,8 @@
toBeAccumulated.add(interval);
} else {
compactIntervals.add(JodaUtils.umbrellaInterval(toBeAccumulated));
- toBeAccumulated = Lists.newArrayList(interval);
+ toBeAccumulated.clear();
Review comment:
Here is a simple example of `umbrellaInterval()` to break this method. Note
that every caller currently passes a list. This method can be changed to accept
a list without further change.
```java
public static Interval umbrellaInterval(List intervals)
{
if (intervals.size() == 1) {
return intervals.get(0);
}
...
```
> Also, as per "Effective Java", the responsibility for making defensive
copies is on the method (API) implementation side, not on the caller side.
I basically agree on this. If you want to keep as it is, please add a
javadoc to `umbrellaInterval()` to make sure that it always returns a new
instance.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] jihoonson commented on a change in pull request #6068: Prohibit Lists.newArrayList() with a single argument
jihoonson commented on a change in pull request #6068: Prohibit
Lists.newArrayList() with a single argument
URL: https://github.com/apache/incubator-druid/pull/6068#discussion_r205986835
##
File path: processing/src/main/java/io/druid/collections/spatial/Node.java
##
@@ -56,7 +57,7 @@ public Node(float[] minCoordinates, float[] maxCoordinates,
boolean isLeaf, Bitm
public Node(
float[] minCoordinates,
float[] maxCoordinates,
- List children,
+ @Nullable Node child,
Review comment:
This adds a new limitation to `Node` to accept only one child when it's
created. It looks to be fine for now, but maybe not in the future. I suggest to
keep this as a list. If you want, we can add a new constructor like below:
```java
public Node(
float[] minCoordinates,
float[] maxCoordinates,
@Nullable Node child,
boolean isLeaf,
Node parent,
MutableBitmap bitmap
)
{
this(
minCoordinates,
maxCoordinates,
//noinspection SSBasedInspection
child == null ? new ArrayList<>() : Lists.newArrayList(child),
isLeaf,
parent,
bitmap
);
}
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
