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/29 02:06:16 UTC
[GitHub] [druid] gianm opened a new pull request #9281: Add
LookupJoinableFactory.
gianm opened a new pull request #9281: Add LookupJoinableFactory.
URL: https://github.com/apache/druid/pull/9281
Enables joins where the right-hand side is a lookup. Includes an
integration test.
Also, includes changes to LookupExtractorFactoryContainerProvider:
1) Add "getAllLookupNames", which will be needed to eventually connect
lookups to Druid's SQL catalog.
2) Convert "get" from nullable to Optional return.
3) Swap out most usages of LookupReferencesManager in favor of the
simpler LookupExtractorFactoryContainerProvider interface.
----------------------------------------------------------------
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 #9281: Add
LookupJoinableFactory.
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9281: Add LookupJoinableFactory.
URL: https://github.com/apache/druid/pull/9281#discussion_r373234927
##########
File path: server/src/test/java/org/apache/druid/guice/JoinableFactoryModuleTest.java
##########
@@ -75,23 +78,25 @@ public void testJoinableFactoryCanBind()
.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(3, joinableFactories.size());
Review comment:
Sounds reasonable, I'll add that in the same followup.
----------------------------------------------------------------
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 #9281: Add
LookupJoinableFactory.
Posted by GitBox <gi...@apache.org>.
ccaominh commented on a change in pull request #9281: Add LookupJoinableFactory.
URL: https://github.com/apache/druid/pull/9281#discussion_r373171796
##########
File path: server/src/test/java/org/apache/druid/guice/JoinableFactoryModuleTest.java
##########
@@ -75,23 +78,25 @@ public void testJoinableFactoryCanBind()
.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(3, joinableFactories.size());
Review comment:
Maybe `JoinableFactoryModule.FACTORY_MAPPINGS` can be made visible for testing so that this can be changed to `FACTORY_MAPPINGS.size() + 1` and the test does not need to updated again if another default mapping is added later.
----------------------------------------------------------------
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 merged pull request #9281: Add LookupJoinableFactory.
Posted by GitBox <gi...@apache.org>.
gianm merged pull request #9281: Add LookupJoinableFactory.
URL: https://github.com/apache/druid/pull/9281
----------------------------------------------------------------
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 #9281: Add
LookupJoinableFactory.
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9281: Add LookupJoinableFactory.
URL: https://github.com/apache/druid/pull/9281#discussion_r373234827
##########
File path: server/src/test/java/org/apache/druid/guice/JoinableFactoryModuleTest.java
##########
@@ -63,7 +66,7 @@ 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(2, joinableFactories.size());
Review comment:
Sure, that sounds good, I'll add one in a followup.
----------------------------------------------------------------
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 #9281: Add
LookupJoinableFactory.
Posted by GitBox <gi...@apache.org>.
ccaominh commented on a change in pull request #9281: Add LookupJoinableFactory.
URL: https://github.com/apache/druid/pull/9281#discussion_r373171138
##########
File path: server/src/test/java/org/apache/druid/guice/JoinableFactoryModuleTest.java
##########
@@ -63,7 +66,7 @@ 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(2, joinableFactories.size());
Review comment:
Do you want to add a similar assert below for `LookupJoinableFactory`?
----------------------------------------------------------------
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