[GitHub] [druid] jon-wei commented on a change in pull request #10224: Segment backed broadcast join IndexedTable

[GitBox <gi...@apache.org>]

jon-wei commented on a change in pull request #10224:
URL: https://github.com/apache/druid/pull/10224#discussion_r466619530



##########
File path: processing/src/main/java/org/apache/druid/segment/join/table/RowBasedIndexedTable.java
##########
@@ -69,10 +68,6 @@ public RowBasedIndexedTable(
     this.keyColumns = keyColumns;
     this.version = version;
 
-    if (new HashSet<>(keyColumns).size() != keyColumns.size()) {

Review comment:
       Should this sanity check be retained?

##########
File path: processing/src/main/java/org/apache/druid/segment/join/MapJoinableFactory.java
##########
@@ -19,49 +19,64 @@
 
 package org.apache.druid.segment.join;
 
+import com.google.common.collect.HashMultimap;
+import com.google.common.collect.SetMultimap;
 import com.google.inject.Inject;
+import org.apache.druid.java.util.common.ISE;
 import org.apache.druid.query.DataSource;
 
-import java.util.IdentityHashMap;
 import java.util.Map;
 import java.util.Optional;
+import java.util.Set;
 
 /**
  * A {@link JoinableFactory} that delegates to the appropriate factory based on the type of the datasource.
  *
- * Datasources can register a factory via a DruidBinder
+ * Datasources can register a factory via a DruidBinder. Any number of factories can be bound to a datasource, the
+ * 'first' that matches will be returned to the caller, or none if no matches.
  */
 public class MapJoinableFactory implements JoinableFactory
 {
-  private final Map<Class<? extends DataSource>, JoinableFactory> joinableFactories;
+  private final SetMultimap<Class<? extends DataSource>, JoinableFactory> joinableFactories;
 
   @Inject
-  public MapJoinableFactory(Map<Class<? extends DataSource>, JoinableFactory> joinableFactories)
+  public MapJoinableFactory(
+      Set<JoinableFactory> factories,
+      Map<Class<? extends JoinableFactory>, Class<? extends DataSource>> factoryToDataSource
+  )
   {
-    // Accesses to IdentityHashMap should be faster than to HashMap or ImmutableMap.
-    // Class doesn't override Object.equals().
-    this.joinableFactories = new IdentityHashMap<>(joinableFactories);
+    this.joinableFactories = HashMultimap.create();
+    factories.forEach(joinableFactory -> {
+      joinableFactories.put(factoryToDataSource.get(joinableFactory.getClass()), joinableFactory);
+    });
   }
 
   @Override
   public boolean isDirectlyJoinable(DataSource dataSource)
   {
-    JoinableFactory factory = joinableFactories.get(dataSource.getClass());
-    if (factory == null) {
-      return false;
-    } else {
-      return factory.isDirectlyJoinable(dataSource);
+    Set<JoinableFactory> factories = joinableFactories.get(dataSource.getClass());
+    for (JoinableFactory factory : factories) {
+      if (factory.isDirectlyJoinable(dataSource)) {
+        return true;
+      }
     }
+    return false;
   }
 
   @Override
   public Optional<Joinable> build(DataSource dataSource, JoinConditionAnalysis condition)
   {
-    JoinableFactory factory = joinableFactories.get(dataSource.getClass());
-    if (factory == null) {
-      return Optional.empty();
-    } else {
-      return factory.build(dataSource, condition);
+    Set<JoinableFactory> factories = joinableFactories.get(dataSource.getClass());
+    Optional<Joinable> maybeJoinable = Optional.empty();
+    for (JoinableFactory factory : factories) {
+      Optional<Joinable> candidate = factory.build(dataSource, condition);
+      if (candidate.isPresent()) {
+        if (maybeJoinable.isPresent()) {
+          throw new ISE("Multiple joinable factories are valid for table[%s]", dataSource);

Review comment:
       Can you explain why this is an error condition? From the comment at https://github.com/apache/druid/pull/10224/files#diff-51b09b44a8330d19728379390eef60a0R35 it sounds like multiple valid should be okay




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org