Turn ImmutableDruidDataSource into a data container

[leventov]

ImmutableDruidDataSource's equals() and hashCode() should throw UnsupportedOperationException as well as DruidDataSource's methods do. ImmutableDruidDataSource should be considered a container, not a data class.

The idea is the same as behind prohibiting/limiting equals() (and therefore usage as HashSet/HashMap keys) of DataSegment: see #6358. When somebody wants to deduplicate ImmutableDruidDataSource objects, they would need to put them into a Map<String, ImmutableDruidDataSource> and resolve conflicts by name manually.

[sashidhar]

@leventov What should be the detail message to be passed to the constructor args of UnsupportedOperationException ? To check my understanding, ImmutableDruidDataSource's equals() and hashCode() methods have to throw UnsupportedOperationException. Is there anything else that needs to be done.

[leventov]

What can also be done is adding static analysis checks to catching calling ImmutableDruidDataSource.equals() calls statically rather than in runtime: see #6358 (comment). You can prohibit $x$.equals() when the type of $x$ is ImmutableDruidDataSource, as well as $MapType$<ImmutableDruidDataSource, $ValueType$> where $MapType$ is .*Map (i. e. HashMap, TreeMap, etc.), and also similar for $SetType$<ImmutableDruidDataSource>.

The error message can say e. g. "ImmutableDruidDataSource shouldn't be used as the key in containers". On the source code level, there should be a link to this issue for the explanation.

[sashidhar]

The side effect of adding a static analysis check catching ImmutableDruidDataSource.equals() is that assert statements in tests like Assert.assertEquals(dataSource, objectMapper.readValue(json, ImmutableDruidDataSource.class)); are flagged as errors.

[sashidhar]

ImmutableDruidDataSourceTest.testSerde() is failing because of this. How can this be rewritten to test serde ?

Raised #7933.

1. ImmutableDruidDataSourceTest.testSerde() is yet to be fixed.
2. Static analysis checks haven't been added yet due to the same reason.

[leventov]

You can move the former equals() implementation to a method like "equalsForTest()" with the comment that it should be used only in tests.

[sashidhar]

1. Introduced equalsForTesting() method and wrote a utils method assertEquals() for equality check of ImmutableDruidDataSource objects in tests (which makes use of equalsForTesting()).

2. When it comes to hashCode() things get interesting. Below is a case where hashCode() is not invoked explicitly from tests. collect() operation invokes hashCode() somewhere downstream.

listDataSources.stream().map(DruidDataSource::toImmutableDruidDataSource).collect(Collectors.toSet()) Line 186.

java.lang.UnsupportedOperationException: hashCode:ImmutableDruidDataSource shouldn't be used as the key in containers

        at org.apache.druid.client.ImmutableDruidDataSource.hashCode(ImmutableDruidDataSource.java:168)
        at java.util.HashMap.hash(HashMap.java:339)
        at java.util.HashMap.put(HashMap.java:612)
        at java.util.HashSet.add(HashSet.java:220)
        at java.util.stream.ReduceOps$3ReducingSink.accept(ReduceOps.java:169)
        at java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:193)
        at java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1382)
        at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:481)
        at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:471)
        at java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:708)
        at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
        at java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:499)
        at org.apache.druid.server.http.DataSourcesResourceTest.testSecuredGetFullQueryableDataSources(DataSourcesResourceTest.java:275)
        ...

How to handle this ?

[leventov]

collect(Collectors.toMap((ImmutableDruidDataSource ds) -> ds.getName(), Function.identity()))

[sashidhar]

Earlier,

Assert.assertEquals( listDataSources.stream().map(DruidDataSource::toImmutableDruidDataSource).collect(Collectors.toSet()), new HashSet<>(result) );

Test: https://github.com/apache/incubator-druid/blob/master/server/src/test/java/org/apache/druid/server/http/DataSourcesResourceTest.java

Collecting the stream as a list instead of a set.

TestUtils.assertEqualsImmutableDruidDataSource( listDataSources.stream().map(DruidDataSource::toImmutableDruidDataSource).collect(Collectors.toList()), new ArrayList<>(result) );

Let me know if this is right.

[leventov]

It's OK