You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/01/28 01:19:34 UTC
[GitHub] [druid] suneet-s opened a new pull request #9271: Make
JoinableFactory an extension point
suneet-s opened a new pull request #9271: Make JoinableFactory an extension point
URL: https://github.com/apache/druid/pull/9271
This change makes it so that extensions can register a JoinableFactory that
should be used for a DataSource.
Extensions can provide the factories via DruidBinders#joinableFactoryBinder
Known DataSources - like InlineDataSource are provided in the
JoinableFactoryModule. This module installs a FactoryWarehouse that is
used to decide which factory should be used to generate the Joinable for
the provided DataSource.
The ExtensionPoint is marked as Beta since it is not yet clear if this
needs to remain available to other extensions or if the best way to
register a factory is by using the datasource class.
<hr>
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/licenses.yaml)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #9271: Make
JoinableFactory an extension point
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9271: Make JoinableFactory an extension point
URL: https://github.com/apache/druid/pull/9271#discussion_r371972823
##########
File path: processing/src/main/java/org/apache/druid/segment/join/JoinableFactory.java
##########
@@ -19,13 +19,17 @@
package org.apache.druid.segment.join;
+import com.google.common.annotations.Beta;
+import org.apache.druid.guice.annotations.ExtensionPoint;
import org.apache.druid.query.DataSource;
import java.util.Optional;
/**
* Utility for creating {@link Joinable} objects.
*/
+@Beta
Review comment:
> It may also be that this ExtensionPoint goes away completely in a near future version, so core extensions should not rely on this.
Would you please add this to the javadoc?
> so core extensions should not rely on this.
Hmm, would you elaborate more on this? Are you referring to the Druid core extensions here? Per my understanding, the extension point and public api interfaces are more for third party extensions which are neither core nor contrib extensions. I think it doesnβt matter much whether the core or contrib extensions rely on some beta extension points or not because we will fix the code for them by ourselves. But the third party extensions are not in the Druid repository but in their own and so changed in extension points and apis should be noticed.
How about making this interface as an extension point once all dusts settle down?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #9271: Make
JoinableFactory an extension point
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9271: Make JoinableFactory an extension point
URL: https://github.com/apache/druid/pull/9271#discussion_r371592053
##########
File path: server/src/main/java/org/apache/druid/guice/JoinableFactoryModule.java
##########
@@ -0,0 +1,60 @@
+/*
+ * 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.guice;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.inject.Binder;
+import com.google.inject.Module;
+import com.google.inject.Scopes;
+import com.google.inject.multibindings.MapBinder;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.InlineDataSource;
+import org.apache.druid.segment.join.InlineJoinableFactory;
+import org.apache.druid.segment.join.JoinableFactory;
+import org.apache.druid.segment.join.MapDataSourceJoinableFactoryWarehouse;
+
+import java.util.Map;
+
+/**
+ * Module that installs {@link JoinableFactory} for the appropriate DataSource.
+ */
+public class JoinableFactoryModule implements Module
Review comment:
π I have them written in the next patch. Going to wait for any other reviews to come in before pushing that back up
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #9271: Make
JoinableFactory an extension point
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9271: Make JoinableFactory an extension point
URL: https://github.com/apache/druid/pull/9271#discussion_r371771802
##########
File path: processing/src/main/java/org/apache/druid/segment/join/JoinableFactory.java
##########
@@ -19,13 +19,17 @@
package org.apache.druid.segment.join;
+import com.google.common.annotations.Beta;
+import org.apache.druid.guice.annotations.ExtensionPoint;
import org.apache.druid.query.DataSource;
import java.util.Optional;
/**
* Utility for creating {@link Joinable} objects.
*/
+@Beta
Review comment:
Yes. That's what I wanted to convey. It may also be that this ExtensionPoint goes away completely in a near future version, so core extensions should not rely on this.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #9271: Make
JoinableFactory an extension point
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9271: Make JoinableFactory an extension point
URL: https://github.com/apache/druid/pull/9271#discussion_r371976682
##########
File path: processing/src/main/java/org/apache/druid/segment/join/JoinableFactory.java
##########
@@ -19,13 +19,17 @@
package org.apache.druid.segment.join;
+import com.google.common.annotations.Beta;
+import org.apache.druid.guice.annotations.ExtensionPoint;
import org.apache.druid.query.DataSource;
import java.util.Optional;
/**
* Utility for creating {@link Joinable} objects.
*/
+@Beta
Review comment:
Good suggestion. I thought it had to be an extension point for the factory to be bound from an extension. I've removed the annotations.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #9271:
Expose JoinableFactory through Guice Bindings
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9271: Expose JoinableFactory through Guice Bindings
URL: https://github.com/apache/druid/pull/9271#discussion_r372088497
##########
File path: processing/src/main/java/org/apache/druid/segment/join/MapDataSourceJoinableFactoryWarehouse.java
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.segment.join;
+
+import com.google.inject.Inject;
+import org.apache.druid.query.DataSource;
+
+import java.util.IdentityHashMap;
+import java.util.Map;
+import java.util.Optional;
+
+/**
+ * A {@link JoinableFactory} that delegates to the appropriate factory based on the type of the datasource.
+ *
+ * Datasources can register a factory via a DruidBinder
+ */
+public class MapDataSourceJoinableFactoryWarehouse implements JoinableFactory
Review comment:
Changed my mind - decided to make a separate PR to keep the deltas small - #9275
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #9271: Make
JoinableFactory an extension point
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9271: Make JoinableFactory an extension point
URL: https://github.com/apache/druid/pull/9271#discussion_r371667976
##########
File path: server/src/test/java/org/apache/druid/guice/JoinableFactoryModuleTest.java
##########
@@ -0,0 +1,104 @@
+/*
+ * 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.guice;
+
+import com.google.common.collect.ImmutableList;
+import com.google.inject.Guice;
+import com.google.inject.Injector;
+import com.google.inject.Key;
+import com.google.inject.Module;
+import com.google.inject.Scopes;
+import com.google.inject.TypeLiteral;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.InlineDataSource;
+import org.apache.druid.segment.join.InlineJoinableFactory;
+import org.apache.druid.segment.join.JoinableFactory;
+import org.apache.druid.segment.join.MapDataSourceJoinableFactoryWarehouse;
+import org.apache.druid.segment.join.NoopDataSource;
+import org.apache.druid.segment.join.NoopJoinableFactory;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import javax.validation.Validation;
+import javax.validation.Validator;
+import java.util.Map;
+import java.util.Properties;
+
+public class JoinableFactoryModuleTest
+{
+ private Injector injector;
+
+ @Before
+ public void setUp()
+ {
+ Properties props = new Properties();
+ injector = makeInjectorWithProperties(props);
+ }
+
+ @Test
+ public void testInjectJoinableFactoryIsSingleton()
+ {
+ JoinableFactory factory = injector.getInstance(JoinableFactory.class);
+ Assert.assertEquals(MapDataSourceJoinableFactoryWarehouse.class, factory.getClass());
+ JoinableFactory otherFactory = injector.getInstance(JoinableFactory.class);
+ Assert.assertSame(factory, otherFactory);
+ }
+
+ @Test
+ public void testInjectDefaultBindingsShouldBeInjected()
+ {
+ Map<Class<? extends DataSource>, JoinableFactory> joinableFactories =
+ injector.getInstance(Key.get(new TypeLiteral<Map<Class<? extends DataSource>, JoinableFactory>>() {}));
+ Assert.assertEquals(1, joinableFactories.size());
+ Assert.assertEquals(InlineJoinableFactory.class, joinableFactories.get(InlineDataSource.class).getClass());
+ }
+
+ @Test
+ public void testJoinableFactoryCanBind()
+ {
+ injector = makeInjectorWithProperties(
+ new Properties(),
+ binder -> DruidBinders.joinableFactoryBinder(binder).addBinding(NoopDataSource.class).toInstance(NoopJoinableFactory.INSTANCE));
+ Map<Class<? extends DataSource>, JoinableFactory> joinableFactories =
+ injector.getInstance(Key.get(new TypeLiteral<Map<Class<? extends DataSource>, JoinableFactory>>() {}));
+ Assert.assertEquals(2, joinableFactories.size());
+ Assert.assertEquals(NoopJoinableFactory.INSTANCE, joinableFactories.get(NoopDataSource.class));
+ }
+
+ private Injector makeInjectorWithProperties(final Properties props, Module... otherModules)
Review comment:
What is `props` for?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #9271: Expose
JoinableFactory through Guice Bindings
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9271: Expose JoinableFactory through Guice Bindings
URL: https://github.com/apache/druid/pull/9271#discussion_r372081312
##########
File path: processing/src/main/java/org/apache/druid/segment/join/MapDataSourceJoinableFactoryWarehouse.java
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.segment.join;
+
+import com.google.inject.Inject;
+import org.apache.druid.query.DataSource;
+
+import java.util.IdentityHashMap;
+import java.util.Map;
+import java.util.Optional;
+
+/**
+ * A {@link JoinableFactory} that delegates to the appropriate factory based on the type of the datasource.
+ *
+ * Datasources can register a factory via a DruidBinder
+ */
+public class MapDataSourceJoinableFactoryWarehouse implements JoinableFactory
Review comment:
1) MapJoinableFactory sounds like a nicer name to me since the "DataSource" isn't adding much, and this is actually a JoinableFactory, not a JoinableFactoryWarehouse.
2) Factories come from warehouses? π
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #9271: Make
JoinableFactory an extension point
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9271: Make JoinableFactory an extension point
URL: https://github.com/apache/druid/pull/9271#discussion_r371772946
##########
File path: processing/src/main/java/org/apache/druid/segment/join/MapDataSourceJoinableFactoryWarehouse.java
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.segment.join;
+
+import com.google.inject.Inject;
+import org.apache.druid.query.DataSource;
+
+import java.util.IdentityHashMap;
+import java.util.Map;
+import java.util.Optional;
+
+/**
+ * A {@link JoinableFactory} that delegates to the appropriate factory based on the type of the datasource.
+ *
+ * Datasources can register a factory via a DruidBinder
Review comment:
`DruidBinders` is available in the `server` sub-module, and the warehouse is in the `processing` sub-module so it's not available from in here
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] ccaominh commented on a change in pull request #9271: Make
JoinableFactory an extension point
Posted by GitBox <gi...@apache.org>.
ccaominh commented on a change in pull request #9271: Make JoinableFactory an extension point
URL: https://github.com/apache/druid/pull/9271#discussion_r371576188
##########
File path: server/src/main/java/org/apache/druid/guice/JoinableFactoryModule.java
##########
@@ -0,0 +1,60 @@
+/*
+ * 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.guice;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.inject.Binder;
+import com.google.inject.Module;
+import com.google.inject.Scopes;
+import com.google.inject.multibindings.MapBinder;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.InlineDataSource;
+import org.apache.druid.segment.join.InlineJoinableFactory;
+import org.apache.druid.segment.join.JoinableFactory;
+import org.apache.druid.segment.join.MapDataSourceJoinableFactoryWarehouse;
+
+import java.util.Map;
+
+/**
+ * Module that installs {@link JoinableFactory} for the appropriate DataSource.
+ */
+public class JoinableFactoryModule implements Module
Review comment:
Do you want to add a unit test for this class?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #9271: Make
JoinableFactory an extension point
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9271: Make JoinableFactory an extension point
URL: https://github.com/apache/druid/pull/9271#discussion_r371665415
##########
File path: processing/src/main/java/org/apache/druid/segment/join/MapDataSourceJoinableFactoryWarehouse.java
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.segment.join;
+
+import com.google.inject.Inject;
+import org.apache.druid.query.DataSource;
+
+import java.util.IdentityHashMap;
+import java.util.Map;
+import java.util.Optional;
+
+/**
+ * A {@link JoinableFactory} that delegates to the appropriate factory based on the type of the datasource.
+ *
+ * Datasources can register a factory via a DruidBinder
Review comment:
`{@link DruidBinders}`?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] ccaominh merged pull request #9271: Expose JoinableFactory
through Guice Bindings
Posted by GitBox <gi...@apache.org>.
ccaominh merged pull request #9271: Expose JoinableFactory through Guice Bindings
URL: https://github.com/apache/druid/pull/9271
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #9271:
Expose JoinableFactory through Guice Bindings
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9271: Expose JoinableFactory through Guice Bindings
URL: https://github.com/apache/druid/pull/9271#discussion_r372086349
##########
File path: processing/src/main/java/org/apache/druid/segment/join/MapDataSourceJoinableFactoryWarehouse.java
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.segment.join;
+
+import com.google.inject.Inject;
+import org.apache.druid.query.DataSource;
+
+import java.util.IdentityHashMap;
+import java.util.Map;
+import java.util.Optional;
+
+/**
+ * A {@link JoinableFactory} that delegates to the appropriate factory based on the type of the datasource.
+ *
+ * Datasources can register a factory via a DruidBinder
+ */
+public class MapDataSourceJoinableFactoryWarehouse implements JoinableFactory
Review comment:
many factories in a warehouse? π
I'll change the name in my next patch - I put `DataSource` in there because I wanted to indicate that it was getting the factory from the datasource. I'll use `MapJoinableFactory`
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #9271: Make
JoinableFactory an extension point
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9271: Make JoinableFactory an extension point
URL: https://github.com/apache/druid/pull/9271#discussion_r371663752
##########
File path: processing/src/main/java/org/apache/druid/segment/join/JoinableFactory.java
##########
@@ -19,13 +19,17 @@
package org.apache.druid.segment.join;
+import com.google.common.annotations.Beta;
+import org.apache.druid.guice.annotations.ExtensionPoint;
import org.apache.druid.query.DataSource;
import java.util.Optional;
/**
* Utility for creating {@link Joinable} objects.
*/
+@Beta
Review comment:
How does it make different from other extension points without `@Beta`? Does it mean this extension point can change in breaking ways even between minor releases?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #9271: Make
JoinableFactory an extension point
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9271: Make JoinableFactory an extension point
URL: https://github.com/apache/druid/pull/9271#discussion_r371775816
##########
File path: server/src/test/java/org/apache/druid/guice/JoinableFactoryModuleTest.java
##########
@@ -0,0 +1,104 @@
+/*
+ * 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.guice;
+
+import com.google.common.collect.ImmutableList;
+import com.google.inject.Guice;
+import com.google.inject.Injector;
+import com.google.inject.Key;
+import com.google.inject.Module;
+import com.google.inject.Scopes;
+import com.google.inject.TypeLiteral;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.InlineDataSource;
+import org.apache.druid.segment.join.InlineJoinableFactory;
+import org.apache.druid.segment.join.JoinableFactory;
+import org.apache.druid.segment.join.MapDataSourceJoinableFactoryWarehouse;
+import org.apache.druid.segment.join.NoopDataSource;
+import org.apache.druid.segment.join.NoopJoinableFactory;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import javax.validation.Validation;
+import javax.validation.Validator;
+import java.util.Map;
+import java.util.Properties;
+
+public class JoinableFactoryModuleTest
+{
+ private Injector injector;
+
+ @Before
+ public void setUp()
+ {
+ Properties props = new Properties();
+ injector = makeInjectorWithProperties(props);
+ }
+
+ @Test
+ public void testInjectJoinableFactoryIsSingleton()
+ {
+ JoinableFactory factory = injector.getInstance(JoinableFactory.class);
+ Assert.assertEquals(MapDataSourceJoinableFactoryWarehouse.class, factory.getClass());
+ JoinableFactory otherFactory = injector.getInstance(JoinableFactory.class);
+ Assert.assertSame(factory, otherFactory);
+ }
+
+ @Test
+ public void testInjectDefaultBindingsShouldBeInjected()
+ {
+ Map<Class<? extends DataSource>, JoinableFactory> joinableFactories =
+ injector.getInstance(Key.get(new TypeLiteral<Map<Class<? extends DataSource>, JoinableFactory>>() {}));
+ Assert.assertEquals(1, joinableFactories.size());
+ Assert.assertEquals(InlineJoinableFactory.class, joinableFactories.get(InlineDataSource.class).getClass());
+ }
+
+ @Test
+ public void testJoinableFactoryCanBind()
+ {
+ injector = makeInjectorWithProperties(
+ new Properties(),
+ binder -> DruidBinders.joinableFactoryBinder(binder).addBinding(NoopDataSource.class).toInstance(NoopJoinableFactory.INSTANCE));
+ Map<Class<? extends DataSource>, JoinableFactory> joinableFactories =
+ injector.getInstance(Key.get(new TypeLiteral<Map<Class<? extends DataSource>, JoinableFactory>>() {}));
+ Assert.assertEquals(2, joinableFactories.size());
+ Assert.assertEquals(NoopJoinableFactory.INSTANCE, joinableFactories.get(NoopDataSource.class));
+ }
+
+ private Injector makeInjectorWithProperties(final Properties props, Module... otherModules)
Review comment:
oops - I actually don't need a bunch of the bindings here. Removed.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org