[GitHub] [incubator-druid] jihoonson commented on a change in pull request #7933: #7858 Throwing UnsupportedOperationException from ImmutableDruidDataSource's equals() and hashCode() methods
jihoonson commented on a change in pull request #7933: #7858 Throwing UnsupportedOperationException from ImmutableDruidDataSource's equals() and hashCode() methods URL: https://github.com/apache/incubator-druid/pull/7933#discussion_r302215099 ## File path: server/src/test/java/org/apache/druid/server/http/DataSourcesResourceTest.java ## @@ -182,9 +182,9 @@ public void testGetFullQueryableDataSources() Set result = (Set) response.getEntity(); Assert.assertEquals(200, response.getStatus()); Assert.assertEquals(2, result.size()); -Assert.assertEquals( - listDataSources.stream().map(DruidDataSource::toImmutableDruidDataSource).collect(Collectors.toSet()), -new HashSet<>(result) +TestUtils.assertEqualsImmutableDruidDataSource( + listDataSources.stream().map(DruidDataSource::toImmutableDruidDataSource).collect(Collectors.toList()), Review comment: The "equality" is checked differently for `ArrayList` and `HashSet`. Set is not an ordered collection, so ordering is not considered when comparing two sets while List is an ordered one. Please check the code of `ArrayList.equals()` and `HashSet.equals()` for details. This is an automated message from the Apache Git Service. To respond to the message, please log on to 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] [incubator-druid] jihoonson commented on a change in pull request #7933: #7858 Throwing UnsupportedOperationException from ImmutableDruidDataSource's equals() and hashCode() methods
jihoonson commented on a change in pull request #7933: #7858 Throwing UnsupportedOperationException from ImmutableDruidDataSource's equals() and hashCode() methods URL: https://github.com/apache/incubator-druid/pull/7933#discussion_r297335072 ## File path: server/src/main/java/org/apache/druid/client/ImmutableDruidDataSource.java ## @@ -120,8 +120,32 @@ public String toString() + "'}"; } + /** + * + * Look at https://github.com/apache/incubator-druid/issues/7858 for explanation of why equals method is not supported. Review comment: Same for `hashCode` This is an automated message from the Apache Git Service. To respond to the message, please log on to 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] [incubator-druid] jihoonson commented on a change in pull request #7933: #7858 Throwing UnsupportedOperationException from ImmutableDruidDataSource's equals() and hashCode() methods
jihoonson commented on a change in pull request #7933: #7858 Throwing
UnsupportedOperationException from ImmutableDruidDataSource's equals() and
hashCode() methods
URL: https://github.com/apache/incubator-druid/pull/7933#discussion_r297341021
##
File path: server/src/test/java/org/apache/druid/test/utils/TestUtils.java
##
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.test.utils;
+
+import org.apache.druid.client.ImmutableDruidDataSource;
+import org.junit.Assert;
+
+import java.util.ArrayList;
+import java.util.List;
+
+public class TestUtils
+{
+
+ /**
+ * This method is to check equality of {@link ImmutableDruidDataSource}
objects to be called from test code.
+ * @param expected expected object
+ * @param actual actual object
+ *
+ */
+ public static void assertEqualsImmutableDruidDataSource(Object expected,
Object actual)
+ {
+if (equalsRegardingNull(expected, actual)) {
+ return;
+} else {
+ throw new AssertionError("Expected and actual objects differ.");
Review comment:
The error should say how they are different.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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] [incubator-druid] jihoonson commented on a change in pull request #7933: #7858 Throwing UnsupportedOperationException from ImmutableDruidDataSource's equals() and hashCode() methods
jihoonson commented on a change in pull request #7933: #7858 Throwing UnsupportedOperationException from ImmutableDruidDataSource's equals() and hashCode() methods URL: https://github.com/apache/incubator-druid/pull/7933#discussion_r297339185 ## File path: server/src/test/java/org/apache/druid/server/http/DataSourcesResourceTest.java ## @@ -268,11 +268,11 @@ public Access authorize(AuthenticationResult authenticationResult1, Resource res Assert.assertEquals(200, response.getStatus()); Assert.assertEquals(1, result.size()); -Assert.assertEquals( +TestUtils.assertEqualsImmutableDruidDataSource( listDataSources.subList(0, 1).stream() .map(DruidDataSource::toImmutableDruidDataSource) - .collect(Collectors.toSet()), -new HashSet<>(result) + .collect(Collectors.toList()), Review comment: Same here. They should be sorted first. This is an automated message from the Apache Git Service. To respond to the message, please log on to 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] [incubator-druid] jihoonson commented on a change in pull request #7933: #7858 Throwing UnsupportedOperationException from ImmutableDruidDataSource's equals() and hashCode() methods
jihoonson commented on a change in pull request #7933: #7858 Throwing
UnsupportedOperationException from ImmutableDruidDataSource's equals() and
hashCode() methods
URL: https://github.com/apache/incubator-druid/pull/7933#discussion_r297342280
##
File path: server/src/test/java/org/apache/druid/test/utils/TestUtils.java
##
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.test.utils;
+
+import org.apache.druid.client.ImmutableDruidDataSource;
+import org.junit.Assert;
+
+import java.util.ArrayList;
+import java.util.List;
+
+public class TestUtils
+{
+
+ /**
+ * This method is to check equality of {@link ImmutableDruidDataSource}
objects to be called from test code.
+ * @param expected expected object
+ * @param actual actual object
+ *
+ */
+ public static void assertEqualsImmutableDruidDataSource(Object expected,
Object actual)
+ {
+if (equalsRegardingNull(expected, actual)) {
+ return;
+} else {
+ throw new AssertionError("Expected and actual objects differ.");
+}
+ }
+
+ private static boolean equalsRegardingNull(Object expected, Object actual)
+ {
+if (expected == null) {
+ return actual == null;
+}
+
+if (expected instanceof ArrayList && actual instanceof ArrayList) {
Review comment:
Why should this method be able to compare `ArrayList` as well as
`ImmutableDruidDataSource`? I suggest to add two methods to compare each type.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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] [incubator-druid] jihoonson commented on a change in pull request #7933: #7858 Throwing UnsupportedOperationException from ImmutableDruidDataSource's equals() and hashCode() methods
jihoonson commented on a change in pull request #7933: #7858 Throwing
UnsupportedOperationException from ImmutableDruidDataSource's equals() and
hashCode() methods
URL: https://github.com/apache/incubator-druid/pull/7933#discussion_r297337965
##
File path:
server/src/test/java/org/apache/druid/client/ImmutableDruidDataSourceTest.java
##
@@ -26,39 +26,76 @@
import com.google.common.collect.ImmutableSortedMap;
import org.apache.druid.jackson.DefaultObjectMapper;
import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.test.utils.TestUtils;
import org.apache.druid.timeline.DataSegment;
import org.apache.druid.timeline.DataSegment.PruneLoadSpecHolder;
-import org.junit.Assert;
import org.junit.Test;
+import javax.annotation.Nonnull;
import java.io.IOException;
public class ImmutableDruidDataSourceTest
{
@Test
public void testSerde() throws IOException
{
-final DataSegment segment = new DataSegment(
-"test",
-Intervals.of("2017/2018"),
-"version",
-null,
-ImmutableList.of("dim1", "dim2"),
-ImmutableList.of("met1", "met2"),
-null,
-1,
-100L,
-PruneLoadSpecHolder.DEFAULT
-);
-final ImmutableDruidDataSource dataSource = new ImmutableDruidDataSource(
-"test",
-ImmutableMap.of("prop1", "val1", "prop2", "val2"),
-ImmutableSortedMap.of(segment.getId(), segment)
-);
+final DataSegment segment = getTestSegment();
+final ImmutableDruidDataSource dataSource =
getImmutableDruidDataSource(segment);
final ObjectMapper objectMapper = new DefaultObjectMapper()
.setInjectableValues(new Std().addValue(PruneLoadSpecHolder.class,
PruneLoadSpecHolder.DEFAULT));
final String json = objectMapper.writeValueAsString(dataSource);
-Assert.assertEquals(dataSource, objectMapper.readValue(json,
ImmutableDruidDataSource.class));
+
+TestUtils.assertEqualsImmutableDruidDataSource(dataSource,
objectMapper.readValue(json, ImmutableDruidDataSource.class));
+ }
+
+ @Test(expected = UnsupportedOperationException.class)
Review comment:
Please use `ExpectedException` instead and check the exception message as
well.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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] [incubator-druid] jihoonson commented on a change in pull request #7933: #7858 Throwing UnsupportedOperationException from ImmutableDruidDataSource's equals() and hashCode() methods
jihoonson commented on a change in pull request #7933: #7858 Throwing UnsupportedOperationException from ImmutableDruidDataSource's equals() and hashCode() methods URL: https://github.com/apache/incubator-druid/pull/7933#discussion_r297340807 ## File path: server/src/test/java/org/apache/druid/test/utils/TestUtils.java ## @@ -0,0 +1,70 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.test.utils; + +import org.apache.druid.client.ImmutableDruidDataSource; +import org.junit.Assert; + +import java.util.ArrayList; +import java.util.List; + +public class TestUtils Review comment: I don't think this class name is very intuitive and easy to search. Only a few ppl would use this class who have seen this class before. Please rename the class to be something more intuitive. Maybe `ImmutableDruidDataSourceUtils`. Also please add a Javadoc link to `ImmutableDruidDataSource.equals()` and `ImmutableDruidDataSource.hashCode()` so that ppl would easily navigate this method. This is an automated message from the Apache Git Service. To respond to the message, please log on to 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] [incubator-druid] jihoonson commented on a change in pull request #7933: #7858 Throwing UnsupportedOperationException from ImmutableDruidDataSource's equals() and hashCode() methods
jihoonson commented on a change in pull request #7933: #7858 Throwing
UnsupportedOperationException from ImmutableDruidDataSource's equals() and
hashCode() methods
URL: https://github.com/apache/incubator-druid/pull/7933#discussion_r297336196
##
File path:
server/src/main/java/org/apache/druid/client/ImmutableDruidDataSource.java
##
@@ -120,8 +120,32 @@ public String toString()
+ "'}";
}
+ /**
+ *
+ * Look at https://github.com/apache/incubator-druid/issues/7858 for
explanation of why equals method is not supported.
+ *
+ */
@Override
public boolean equals(Object o)
+ {
+throw new UnsupportedOperationException("equals:ImmutableDruidDataSource
shouldn't be used as the key in containers");
Review comment:
It looks like `equals:ImmutableDruidDataSource` is not the Java convention.
At least I'm not familiar with it. Would you tell me where this convention came
from?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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] [incubator-druid] jihoonson commented on a change in pull request #7933: #7858 Throwing UnsupportedOperationException from ImmutableDruidDataSource's equals() and hashCode() methods
jihoonson commented on a change in pull request #7933: #7858 Throwing UnsupportedOperationException from ImmutableDruidDataSource's equals() and hashCode() methods URL: https://github.com/apache/incubator-druid/pull/7933#discussion_r297335015 ## File path: server/src/main/java/org/apache/druid/client/ImmutableDruidDataSource.java ## @@ -120,8 +120,32 @@ public String toString() + "'}"; } + /** + * + * Look at https://github.com/apache/incubator-druid/issues/7858 for explanation of why equals method is not supported. Review comment: Please describe the reason properly why this method shouldn't be used in addition to the link to the original issue. This is an automated message from the Apache Git Service. To respond to the message, please log on to 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] [incubator-druid] jihoonson commented on a change in pull request #7933: #7858 Throwing UnsupportedOperationException from ImmutableDruidDataSource's equals() and hashCode() methods
jihoonson commented on a change in pull request #7933: #7858 Throwing
UnsupportedOperationException from ImmutableDruidDataSource's equals() and
hashCode() methods
URL: https://github.com/apache/incubator-druid/pull/7933#discussion_r297338060
##
File path:
server/src/test/java/org/apache/druid/client/ImmutableDruidDataSourceTest.java
##
@@ -26,39 +26,76 @@
import com.google.common.collect.ImmutableSortedMap;
import org.apache.druid.jackson.DefaultObjectMapper;
import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.test.utils.TestUtils;
import org.apache.druid.timeline.DataSegment;
import org.apache.druid.timeline.DataSegment.PruneLoadSpecHolder;
-import org.junit.Assert;
import org.junit.Test;
+import javax.annotation.Nonnull;
import java.io.IOException;
public class ImmutableDruidDataSourceTest
{
@Test
public void testSerde() throws IOException
{
-final DataSegment segment = new DataSegment(
-"test",
-Intervals.of("2017/2018"),
-"version",
-null,
-ImmutableList.of("dim1", "dim2"),
-ImmutableList.of("met1", "met2"),
-null,
-1,
-100L,
-PruneLoadSpecHolder.DEFAULT
-);
-final ImmutableDruidDataSource dataSource = new ImmutableDruidDataSource(
-"test",
-ImmutableMap.of("prop1", "val1", "prop2", "val2"),
-ImmutableSortedMap.of(segment.getId(), segment)
-);
+final DataSegment segment = getTestSegment();
+final ImmutableDruidDataSource dataSource =
getImmutableDruidDataSource(segment);
final ObjectMapper objectMapper = new DefaultObjectMapper()
.setInjectableValues(new Std().addValue(PruneLoadSpecHolder.class,
PruneLoadSpecHolder.DEFAULT));
final String json = objectMapper.writeValueAsString(dataSource);
-Assert.assertEquals(dataSource, objectMapper.readValue(json,
ImmutableDruidDataSource.class));
+
+TestUtils.assertEqualsImmutableDruidDataSource(dataSource,
objectMapper.readValue(json, ImmutableDruidDataSource.class));
+ }
+
+ @Test(expected = UnsupportedOperationException.class)
+ public void testEqualsMethodThrowsUnsupportedOperationException()
+ {
+final DataSegment segment1 = getTestSegment();
+
+final ImmutableDruidDataSource dataSource1 =
getImmutableDruidDataSource(segment1);
+
+final DataSegment segment2 = getTestSegment();
+
+final ImmutableDruidDataSource dataSource2 =
getImmutableDruidDataSource(segment2);
+
+dataSource1.equals(dataSource2);
+ }
+
+ @Nonnull
+ private ImmutableDruidDataSource getImmutableDruidDataSource(DataSegment
segment1)
+ {
+return new ImmutableDruidDataSource(
+ "test",
+ ImmutableMap.of("prop1", "val1", "prop2", "val2"),
+ ImmutableSortedMap.of(segment1.getId(), segment1)
+);
+ }
+
+ @Nonnull
+ private DataSegment getTestSegment()
+ {
+return new DataSegment(
+ "test",
+ Intervals.of("2017/2018"),
+ "version",
+ null,
+ ImmutableList.of("dim1", "dim2"),
+ ImmutableList.of("met1", "met2"),
+ null,
+ 1,
+ 100L,
+ PruneLoadSpecHolder.DEFAULT
+);
+ }
+
+ @Test(expected = UnsupportedOperationException.class)
Review comment:
Same here. Use `ExpectedException` instead.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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] [incubator-druid] jihoonson commented on a change in pull request #7933: #7858 Throwing UnsupportedOperationException from ImmutableDruidDataSource's equals() and hashCode() methods
jihoonson commented on a change in pull request #7933: #7858 Throwing UnsupportedOperationException from ImmutableDruidDataSource's equals() and hashCode() methods URL: https://github.com/apache/incubator-druid/pull/7933#discussion_r297339093 ## File path: server/src/test/java/org/apache/druid/server/http/DataSourcesResourceTest.java ## @@ -182,9 +182,9 @@ public void testGetFullQueryableDataSources() Set result = (Set) response.getEntity(); Assert.assertEquals(200, response.getStatus()); Assert.assertEquals(2, result.size()); -Assert.assertEquals( - listDataSources.stream().map(DruidDataSource::toImmutableDruidDataSource).collect(Collectors.toSet()), -new HashSet<>(result) +TestUtils.assertEqualsImmutableDruidDataSource( + listDataSources.stream().map(DruidDataSource::toImmutableDruidDataSource).collect(Collectors.toList()), Review comment: This test code depends on how `listDataSources` is created and it works only when `listDataSources` is in the same order with `result`. They should be sorted first before you compare them. This is an automated message from the Apache Git Service. To respond to the message, please log on to 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] [incubator-druid] jihoonson commented on a change in pull request #7933: #7858 Throwing UnsupportedOperationException from ImmutableDruidDataSource's equals() and hashCode() methods
jihoonson commented on a change in pull request #7933: #7858 Throwing UnsupportedOperationException from ImmutableDruidDataSource's equals() and hashCode() methods URL: https://github.com/apache/incubator-druid/pull/7933#discussion_r297337106 ## File path: server/src/test/java/org/apache/druid/client/ImmutableDruidDataSourceTest.java ## @@ -26,39 +26,76 @@ import com.google.common.collect.ImmutableSortedMap; import org.apache.druid.jackson.DefaultObjectMapper; import org.apache.druid.java.util.common.Intervals; +import org.apache.druid.test.utils.TestUtils; import org.apache.druid.timeline.DataSegment; import org.apache.druid.timeline.DataSegment.PruneLoadSpecHolder; -import org.junit.Assert; import org.junit.Test; +import javax.annotation.Nonnull; Review comment: You don't have to use this. We assume everything is not null unless they are explicitly annotated as `@Nullable`. This is an automated message from the Apache Git Service. To respond to the message, please log on to 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
