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 00:43:12 UTC
[GitHub] [druid] suneet-s opened a new pull request #9279: Guicify druid sql
module
suneet-s opened a new pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279
Break up the SQLModule in to smaller modules and provide a binding that
modules can use to register schemas with druid sql.
TODO: Add more tests for the modules. There is currently one test failing that also needs to be fixed (CalciteQueryTest)
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] gianm commented on a change in pull request #9279: Guicify
druid sql module
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r374400132
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidCalciteSchemaModule.java
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.sql.calcite.schema;
+
+import com.google.inject.Binder;
+import com.google.inject.Module;
+import com.google.inject.Provides;
+import com.google.inject.Scopes;
+import com.google.inject.Singleton;
+import com.google.inject.name.Named;
+import com.google.inject.name.Names;
+import org.apache.calcite.schema.SchemaPlus;
+import org.apache.druid.guice.LifecycleModule;
+import org.apache.druid.sql.guice.SqlBindings;
+
+/**
+ * The module responsible for providing bindings
+ */
+public class DruidCalciteSchemaModule implements Module
+{
+ private static final String DRUID_SCHEMA_NAME = "druid";
+ static final String INCOMPLETE_SCHEMA = "INCOMPLETE_SCHEMA";
+
+ @Override
+ public void configure(Binder binder)
+ {
+ binder.bind(String.class).annotatedWith(DruidSchemaName.class).toInstance(DRUID_SCHEMA_NAME);
+
+ // Should only be used by the information schema
+ binder.bind(SchemaPlus.class)
+ .annotatedWith(Names.named(INCOMPLETE_SCHEMA))
+ .toProvider(RootSchemaProvider.class)
+ .in(Scopes.SINGLETON);
+
+ // DruidSchema needs to listen to changes for incoming segments
+ LifecycleModule.register(binder, DruidSchema.class);
+ binder.bind(SystemSchema.class).in(Scopes.SINGLETON);
+ binder.bind(InformationSchema.class).in(Scopes.SINGLETON);
+ binder.bind(LookupSchema.class).in(Scopes.SINGLETON);
+
+ // Binder to inject different schema to Calcite
+ SqlBindings.addSchema(binder, DruidSqlSchema.class);
+ SqlBindings.addSchema(binder, SystemSqlSchema.class);
+ SqlBindings.addSchema(binder, LookupSqlSchema.class);
+ }
+
+ @Provides
+ @Singleton
+ private SchemaPlus getRootSchema(@Named(INCOMPLETE_SCHEMA) SchemaPlus rootSchema, InformationSchema informationSchema)
+ {
+ String name = "INFORMATION_SCHEMA";
+ rootSchema.add(name, informationSchema);
Review comment:
This is a bit gross. The binding setup and method signatures suggest that the complete root schema is built using an incomplete root schema as input. But this method actually modifies the incomplete schema and they end up being the exact same object. And in fact, that grossness is required for the InformationSchema to work properly, because it needs to be able to see itself in the root schema!
Maybe we could give the InformationSchema a root schema provider instead of the actual root schema? Would that work? It'd be cleaner, if so.
----------------------------------------------------------------
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 #9279:
Guicify druid sql module
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r374816210
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidSqlSchema.java
##########
@@ -0,0 +1,51 @@
+/*
+ * 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.sql.calcite.schema;
+
+import com.google.inject.Inject;
+import org.apache.calcite.schema.Schema;
+
+/**
+ * The schema for druid tables to be accessible via sql.
+ */
+class DruidSqlSchema implements DruidCalciteSchema
Review comment:
lol - alright I like that suggestion. Thanks!
----------------------------------------------------------------
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 #9279: Guicify
druid sql module
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r374850725
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidSqlSchema.java
##########
@@ -0,0 +1,51 @@
+/*
+ * 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.sql.calcite.schema;
+
+import com.google.inject.Inject;
+import org.apache.calcite.schema.Schema;
+
+/**
+ * The schema for druid tables to be accessible via sql.
+ */
+class DruidSqlSchema implements DruidCalciteSchema
Review comment:
:|
----------------------------------------------------------------
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 #9279: Guicify druid sql module
Posted by GitBox <gi...@apache.org>.
gianm merged pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279
----------------------------------------------------------------
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 issue #9279: Guicify druid sql module
Posted by GitBox <gi...@apache.org>.
suneet-s commented on issue #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#issuecomment-581002193
@gianm This is ready for review, but I can't remove the WIP label
----------------------------------------------------------------
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 #9279:
Guicify druid sql module
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r374440751
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidSqlSchema.java
##########
@@ -0,0 +1,51 @@
+/*
+ * 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.sql.calcite.schema;
+
+import com.google.inject.Inject;
+import org.apache.calcite.schema.Schema;
+
+/**
+ * The schema for druid tables to be accessible via sql.
+ */
+class DruidSqlSchema implements DruidCalciteSchema
Review comment:
I don't like `Provider` because that's a guice thing and I'd expect it to have a method that returns an object, but this interface returns 2 things and it doesn't implement Provider<T>
How about
* `<Druid>Schema` which provides the implementation that Calcite needs to get tables
* `CalciteSubSchema` which is an interface containing a name and a `Schema` so that it can be registered to a Root Schema(`SchemaPlus`)
* `<System>SubSchema` the implementation of the `CalciteSubSchema` interface for appropriate `<System>Schema`
----------------------------------------------------------------
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 #9279:
Guicify druid sql module
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r372535816
##########
File path: sql/src/main/java/org/apache/druid/sql/guice/SqlModule.java
##########
@@ -63,25 +55,21 @@ public void configure(Binder binder)
if (isEnabled()) {
Calcites.setSystemProperties();
- JsonConfigProvider.bind(binder, "druid.sql.planner", PlannerConfig.class);
- JsonConfigProvider.bind(binder, "druid.sql.avatica", AvaticaServerConfig.class);
- LifecycleModule.register(binder, DruidSchema.class);
binder.bind(ViewManager.class).to(NoopViewManager.class).in(LazySingleton.class);
- // Add empty SqlAggregator binder.
- Multibinder.newSetBinder(binder, SqlAggregator.class);
+ binder.install(new DruidCalciteSchemaModule());
Review comment:
I usually like smaller modules because it's easier to rationalize what needs to be installed in that module. Every package has a module that describes how implementations in it's package are provided to the rest of the code. The other advantage of this is you can keep all the implementations package private and only expose interfaces to other packages - which means there's less accidental coupling between packages.
This also makes testing each module easier (I realize I haven't written tests for them yet, but they're coming)
----------------------------------------------------------------
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 #9279: Guicify
druid sql module
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r374400877
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidSchemaName.java
##########
@@ -0,0 +1,37 @@
+/*
+ * 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.sql.calcite.schema;
+
+import com.google.inject.BindingAnnotation;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+/**
+ * An annotation to get the name of the schenma to access druid tables in SQL.
Review comment:
"schema" (spelling). And "Druid" should be capitalized.
----------------------------------------------------------------
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 #9279:
Guicify druid sql module
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r372208486
##########
File path: benchmarks/src/main/java/org/apache/druid/benchmark/query/SqlVsNativeBenchmark.java
##########
@@ -116,12 +115,10 @@ public void setup()
final PlannerConfig plannerConfig = new PlannerConfig();
this.walker = closer.register(new SpecificSegmentsQuerySegmentWalker(conglomerate).add(dataSegment, index));
- final DruidSchema druidSchema = CalciteTests.createMockSchema(conglomerate, walker, plannerConfig);
- final SystemSchema systemSchema = CalciteTests.createMockSystemSchema(druidSchema, walker, plannerConfig);
-
+ final SchemaPlus rootSchema =
Review comment:
I don't usually mark variables in a function as final, but I was trying to follow the pattern I saw in the class. That's why it's inconsistent :\ I'll clean it 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 #9279:
Guicify druid sql module
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r374433550
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidCalciteSchemaModule.java
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.sql.calcite.schema;
+
+import com.google.inject.Binder;
+import com.google.inject.Module;
+import com.google.inject.Provides;
+import com.google.inject.Scopes;
+import com.google.inject.Singleton;
+import com.google.inject.name.Named;
+import com.google.inject.name.Names;
+import org.apache.calcite.schema.SchemaPlus;
+import org.apache.druid.guice.LifecycleModule;
+import org.apache.druid.sql.guice.SqlBindings;
+
+/**
+ * The module responsible for providing bindings
+ */
+public class DruidCalciteSchemaModule implements Module
+{
+ private static final String DRUID_SCHEMA_NAME = "druid";
+ static final String INCOMPLETE_SCHEMA = "INCOMPLETE_SCHEMA";
+
+ @Override
+ public void configure(Binder binder)
+ {
+ binder.bind(String.class).annotatedWith(DruidSchemaName.class).toInstance(DRUID_SCHEMA_NAME);
+
+ // Should only be used by the information schema
+ binder.bind(SchemaPlus.class)
+ .annotatedWith(Names.named(INCOMPLETE_SCHEMA))
+ .toProvider(RootSchemaProvider.class)
+ .in(Scopes.SINGLETON);
+
+ // DruidSchema needs to listen to changes for incoming segments
+ LifecycleModule.register(binder, DruidSchema.class);
+ binder.bind(SystemSchema.class).in(Scopes.SINGLETON);
+ binder.bind(InformationSchema.class).in(Scopes.SINGLETON);
+ binder.bind(LookupSchema.class).in(Scopes.SINGLETON);
+
+ // Binder to inject different schema to Calcite
+ SqlBindings.addSchema(binder, DruidSqlSchema.class);
+ SqlBindings.addSchema(binder, SystemSqlSchema.class);
+ SqlBindings.addSchema(binder, LookupSqlSchema.class);
+ }
+
+ @Provides
+ @Singleton
+ private SchemaPlus getRootSchema(@Named(INCOMPLETE_SCHEMA) SchemaPlus rootSchema, InformationSchema informationSchema)
+ {
+ String name = "INFORMATION_SCHEMA";
Review comment:
Done
----------------------------------------------------------------
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 #9279: Guicify
druid sql module
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r374404659
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/view/DruidViewMacroFactory.java
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.sql.calcite.view;
+
+import org.apache.druid.server.security.Escalator;
+import org.apache.druid.sql.calcite.planner.PlannerFactory;
+
+/**
+ * A factory to create a {@link DruidViewMacro} that is used by Guice for Assisted injection.
+ */
+public interface DruidViewMacroFactory
+{
+ /**
+ * Creates an instance of {@link DruidViewMacro}
+ */
+ DruidViewMacro create(PlannerFactory plannerFactory, Escalator escalator, String viewSql);
Review comment:
Is it really worth all this indirection and complexity to avoid having stuff refer to the static string `"druid"`?
----------------------------------------------------------------
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 #9279: Guicify
druid sql module
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r372165871
##########
File path: sql/src/main/java/org/apache/druid/sql/avatica/AvaticaModule.java
##########
@@ -0,0 +1,42 @@
+/*
+ * 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.sql.avatica;
+
+import com.google.inject.Binder;
+import com.google.inject.Module;
+import org.apache.druid.guice.JsonConfigProvider;
+import org.apache.druid.guice.LazySingleton;
+import org.apache.druid.server.initialization.jetty.JettyBindings;
+import org.apache.druid.server.metrics.MetricsModule;
+
+/**
+ * The module responsible for providing bindings to Avatica
Review comment:
I'll only mention this once to avoid clutter in the review, but IMO Javadoc lines are nicer looking when they are punctuated like sentences (end in periods). Even if they're not really sentences.
----------------------------------------------------------------
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 #9279: Guicify
druid sql module
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r374360878
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/aggregation/SqlAggregationModule.java
##########
@@ -0,0 +1,38 @@
+/*
+ * 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.sql.calcite.aggregation;
+
+import com.google.inject.Binder;
+import com.google.inject.Module;
+import com.google.inject.multibindings.Multibinder;
+
+/**
+ * Module that provides SQL aggregations.
+ * To add an aggregation use {@link org.apache.druid.sql.guice.SqlBindings#addAggregator(Binder, Class)}
+ */
+public class SqlAggregationModule implements Module
+{
+ @Override
+ public void configure(Binder binder)
+ {
+ // Add empty SqlAggregator binder.
+ Multibinder.newSetBinder(binder, SqlAggregator.class);
Review comment:
This is (AFAIK) only used by DruidOperatorTable, which is provided by CalcitePlannerModule. I don't think anyone would ever want just one or the other. Would it make sense to combine 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:
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 #9279:
Guicify druid sql module
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r372533247
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidCalciteSchema.java
##########
@@ -0,0 +1,33 @@
+/*
+ * 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.sql.calcite.schema;
+
+import org.apache.calcite.schema.Schema;
+
+/**
+ * A calcite schema that has a name which it should be registered to.
+ */
+public interface DruidCalciteSchema extends Schema
Review comment:
Yeah I was being lazy and extended a libraries' interface. I've changed it to use composition instead since I need the name and the Schema .
----------------------------------------------------------------
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 #9279: Guicify
druid sql module
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r372165333
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlCalcitePlannerModule.java
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.sql.calcite.planner;
+
+import com.google.inject.Binder;
+import com.google.inject.Module;
+import com.google.inject.Scopes;
+import org.apache.calcite.schema.SchemaPlus;
+import org.apache.druid.guice.JsonConfigProvider;
+
+/**
+ * The module responsible for provide bindings for the Calcite Planner.
+ */
+public class DruidSqlCalcitePlannerModule implements Module
Review comment:
IMO CalcitePlannerModule is fine / better. We don't use Calcite for anything other than Druid SQL so it's not ambiguous.
----------------------------------------------------------------
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 #9279:
Guicify druid sql module
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r374410500
##########
File path: sql/src/main/java/org/apache/druid/sql/guice/SqlModule.java
##########
@@ -63,25 +55,21 @@ public void configure(Binder binder)
if (isEnabled()) {
Calcites.setSystemProperties();
- JsonConfigProvider.bind(binder, "druid.sql.planner", PlannerConfig.class);
- JsonConfigProvider.bind(binder, "druid.sql.avatica", AvaticaServerConfig.class);
- LifecycleModule.register(binder, DruidSchema.class);
binder.bind(ViewManager.class).to(NoopViewManager.class).in(LazySingleton.class);
- // Add empty SqlAggregator binder.
- Multibinder.newSetBinder(binder, SqlAggregator.class);
+ binder.install(new DruidCalciteSchemaModule());
Review comment:
A pattern I've used in the past is 1 module per sub package. The source directory structure should already be broken up so that related classes are under the same package.
----------------------------------------------------------------
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 #9279:
Guicify druid sql module
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r372208685
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlCalcitePlannerModule.java
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.sql.calcite.planner;
+
+import com.google.inject.Binder;
+import com.google.inject.Module;
+import com.google.inject.Scopes;
+import org.apache.calcite.schema.SchemaPlus;
+import org.apache.druid.guice.JsonConfigProvider;
+
+/**
+ * The module responsible for provide bindings for the Calcite Planner.
+ */
+public class DruidSqlCalcitePlannerModule implements Module
Review comment:
👍
----------------------------------------------------------------
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 #9279:
Guicify druid sql module
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r374426610
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/schema/RootSchemaProvider.java
##########
@@ -0,0 +1,53 @@
+/*
+ * 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.sql.calcite.schema;
+
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import org.apache.calcite.jdbc.CalciteSchema;
+import org.apache.calcite.schema.SchemaPlus;
+
+import java.util.Set;
+
+/**
+ * Provides the RootSchema for calcite with
+ * - metadata schema disabled because it's not needed
+ * - caching disabled because druid's caching is better.
+ *
+ * All the provided schema are added to the rootSchema.
+ */
+public class RootSchemaProvider implements Provider<SchemaPlus>
+{
+ private final Set<DruidCalciteSchema> calciteSchemas;
+
+ @Inject
+ RootSchemaProvider(Set<DruidCalciteSchema> calciteSchemas)
+ {
+ this.calciteSchemas = calciteSchemas;
+ }
+
+ @Override
+ public SchemaPlus get()
+ {
+ SchemaPlus rootSchema = CalciteSchema.createRootSchema(false, false).plus();
+ calciteSchemas.forEach(schema -> rootSchema.add(schema.getSchemaName(), schema.getSchema()));
Review comment:
Done.
----------------------------------------------------------------
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 #9279: Guicify
druid sql module
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r372167534
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidCalciteSchema.java
##########
@@ -0,0 +1,33 @@
+/*
+ * 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.sql.calcite.schema;
+
+import org.apache.calcite.schema.Schema;
+
+/**
+ * A calcite schema that has a name which it should be registered to.
+ */
+public interface DruidCalciteSchema extends Schema
Review comment:
Do you think it makes sense to make this a SchemaProvider and have it return a Schema instead of being one? It always seems weird to me to extend other libraries' interfaces and add methods to 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:
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 #9279: Guicify
druid sql module
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r372166678
##########
File path: sql/src/main/java/org/apache/druid/sql/guice/SqlBindings.java
##########
@@ -41,4 +45,12 @@ public static void addOperatorConversion(
{
Multibinder.newSetBinder(binder, SqlOperatorConversion.class).addBinding().to(clazz);
}
+
+ /**
+ * Returns a multiBinder that can modules can use to bind {@link DruidCalciteSchema} to be used by the SqlModule
+ */
+ public static Multibinder<DruidCalciteSchema> calciteSchemaBinder(final Binder binder)
Review comment:
Maybe do `addSchema` to be consistent with the others.
I recognize this isn't consistent with how DruidBindings works, but you can't win 'em all. (Well, I guess you could win them all, if you changed DruidBindings too.)
----------------------------------------------------------------
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 #9279:
Guicify druid sql module
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r374433786
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidCalciteSchemaModule.java
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.sql.calcite.schema;
+
+import com.google.inject.Binder;
+import com.google.inject.Module;
+import com.google.inject.Provides;
+import com.google.inject.Scopes;
+import com.google.inject.Singleton;
+import com.google.inject.name.Named;
+import com.google.inject.name.Names;
+import org.apache.calcite.schema.SchemaPlus;
+import org.apache.druid.guice.LifecycleModule;
+import org.apache.druid.sql.guice.SqlBindings;
+
+/**
+ * The module responsible for providing bindings
+ */
+public class DruidCalciteSchemaModule implements Module
+{
+ private static final String DRUID_SCHEMA_NAME = "druid";
+ static final String INCOMPLETE_SCHEMA = "INCOMPLETE_SCHEMA";
+
+ @Override
+ public void configure(Binder binder)
+ {
+ binder.bind(String.class).annotatedWith(DruidSchemaName.class).toInstance(DRUID_SCHEMA_NAME);
+
+ // Should only be used by the information schema
+ binder.bind(SchemaPlus.class)
+ .annotatedWith(Names.named(INCOMPLETE_SCHEMA))
+ .toProvider(RootSchemaProvider.class)
+ .in(Scopes.SINGLETON);
+
+ // DruidSchema needs to listen to changes for incoming segments
+ LifecycleModule.register(binder, DruidSchema.class);
+ binder.bind(SystemSchema.class).in(Scopes.SINGLETON);
+ binder.bind(InformationSchema.class).in(Scopes.SINGLETON);
+ binder.bind(LookupSchema.class).in(Scopes.SINGLETON);
+
+ // Binder to inject different schema to Calcite
+ SqlBindings.addSchema(binder, DruidSqlSchema.class);
+ SqlBindings.addSchema(binder, SystemSqlSchema.class);
+ SqlBindings.addSchema(binder, LookupSqlSchema.class);
+ }
+
+ @Provides
+ @Singleton
+ private SchemaPlus getRootSchema(@Named(INCOMPLETE_SCHEMA) SchemaPlus rootSchema, InformationSchema informationSchema)
+ {
+ String name = "INFORMATION_SCHEMA";
+ rootSchema.add(name, informationSchema);
Review comment:
Agreed this is gross :( I tried for a while to get this set up correctly. Here's where I landed
Unfortunately I'd still need to inject 2 `SchemaPlus`es - a Provider to an "incomplete" one that is needed by InformationSchema, and a complete one that's used by the PlannerFactory. It's unclear to me if any other classes would ever need to use the "complete" schema. So even though this is gross, I think this will prevent someone from accidentally injecting the wrong schema in another class.
The grossness is also contained in this one method. I'll add more comments explaining this. Maybe someone else can make this better in a future patch. 🤞
----------------------------------------------------------------
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 #9279: Guicify
druid sql module
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r374815572
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidSqlSchema.java
##########
@@ -0,0 +1,51 @@
+/*
+ * 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.sql.calcite.schema;
+
+import com.google.inject.Inject;
+import org.apache.calcite.schema.Schema;
+
+/**
+ * The schema for druid tables to be accessible via sql.
+ */
+class DruidSqlSchema implements DruidCalciteSchema
Review comment:
So there'd be both a SystemSchema and SystemSubSchema (and the SystemSubSchema would be how you get a SystemSchema)?
Who would have thought this would be the hard part.
How about DruidSchema, NamedDruidSchema, and NamedSchema?
----------------------------------------------------------------
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 #9279: Guicify
druid sql module
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r374401184
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidSqlSchema.java
##########
@@ -0,0 +1,51 @@
+/*
+ * 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.sql.calcite.schema;
+
+import com.google.inject.Inject;
+import org.apache.calcite.schema.Schema;
+
+/**
+ * The schema for druid tables to be accessible via sql.
Review comment:
"Druid", "SQL" (capitalization)
----------------------------------------------------------------
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 #9279: Guicify
druid sql module
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r372531831
##########
File path: benchmarks/src/main/java/org/apache/druid/benchmark/query/SqlVsNativeBenchmark.java
##########
@@ -116,12 +115,10 @@ public void setup()
final PlannerConfig plannerConfig = new PlannerConfig();
this.walker = closer.register(new SpecificSegmentsQuerySegmentWalker(conglomerate).add(dataSegment, index));
- final DruidSchema druidSchema = CalciteTests.createMockSchema(conglomerate, walker, plannerConfig);
- final SystemSchema systemSchema = CalciteTests.createMockSystemSchema(druidSchema, walker, plannerConfig);
-
+ final SchemaPlus rootSchema =
Review comment:
In that case, keeping to the style of the surrounding code is probably best. I withdraw my request for consistency across files. (Unless we can reach consensus on whether vars should be marked final whenever possible.)
----------------------------------------------------------------
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 #9279: Guicify
druid sql module
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r374403101
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/schema/RootSchemaProvider.java
##########
@@ -0,0 +1,53 @@
+/*
+ * 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.sql.calcite.schema;
+
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import org.apache.calcite.jdbc.CalciteSchema;
+import org.apache.calcite.schema.SchemaPlus;
+
+import java.util.Set;
+
+/**
+ * Provides the RootSchema for calcite with
+ * - metadata schema disabled because it's not needed
+ * - caching disabled because druid's caching is better.
+ *
+ * All the provided schema are added to the rootSchema.
+ */
+public class RootSchemaProvider implements Provider<SchemaPlus>
+{
+ private final Set<DruidCalciteSchema> calciteSchemas;
+
+ @Inject
+ RootSchemaProvider(Set<DruidCalciteSchema> calciteSchemas)
+ {
+ this.calciteSchemas = calciteSchemas;
+ }
+
+ @Override
+ public SchemaPlus get()
+ {
+ SchemaPlus rootSchema = CalciteSchema.createRootSchema(false, false).plus();
+ calciteSchemas.forEach(schema -> rootSchema.add(schema.getSchemaName(), schema.getSchema()));
Review comment:
Would be nice to have something here to check for duplicate schema names. I don't think the base Calcite class does.
----------------------------------------------------------------
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 #9279: Guicify
druid sql module
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r372166465
##########
File path: sql/src/main/java/org/apache/druid/sql/guice/SqlModule.java
##########
@@ -63,25 +55,21 @@ public void configure(Binder binder)
if (isEnabled()) {
Calcites.setSystemProperties();
- JsonConfigProvider.bind(binder, "druid.sql.planner", PlannerConfig.class);
- JsonConfigProvider.bind(binder, "druid.sql.avatica", AvaticaServerConfig.class);
- LifecycleModule.register(binder, DruidSchema.class);
binder.bind(ViewManager.class).to(NoopViewManager.class).in(LazySingleton.class);
- // Add empty SqlAggregator binder.
- Multibinder.newSetBinder(binder, SqlAggregator.class);
+ binder.install(new DruidCalciteSchemaModule());
Review comment:
I'm wondering why you wanted to split these up? It seemed nice to me to have all the core SQL-related bindings in one file.
----------------------------------------------------------------
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 #9279: Guicify
druid sql module
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r372164143
##########
File path: benchmarks/src/main/java/org/apache/druid/benchmark/query/SqlVsNativeBenchmark.java
##########
@@ -116,12 +115,10 @@ public void setup()
final PlannerConfig plannerConfig = new PlannerConfig();
this.walker = closer.register(new SpecificSegmentsQuerySegmentWalker(conglomerate).add(dataSegment, index));
- final DruidSchema druidSchema = CalciteTests.createMockSchema(conglomerate, walker, plannerConfig);
- final SystemSchema systemSchema = CalciteTests.createMockSystemSchema(druidSchema, walker, plannerConfig);
-
+ final SchemaPlus rootSchema =
Review comment:
This one is `final` but the one in SqlBenchmark isn't, any reason?
(Fwiw I usually like labeling things final whenever possible but I don't insist on it, except for class fields)
----------------------------------------------------------------
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 #9279: Guicify
druid sql module
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r374361494
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidCalciteSchemaModule.java
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.sql.calcite.schema;
+
+import com.google.inject.Binder;
+import com.google.inject.Module;
+import com.google.inject.Provides;
+import com.google.inject.Scopes;
+import com.google.inject.Singleton;
+import com.google.inject.name.Named;
+import com.google.inject.name.Names;
+import org.apache.calcite.schema.SchemaPlus;
+import org.apache.druid.guice.LifecycleModule;
+import org.apache.druid.sql.guice.SqlBindings;
+
+/**
+ * The module responsible for providing bindings
Review comment:
But which bindings?
----------------------------------------------------------------
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 #9279:
Guicify druid sql module
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r374424527
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/view/DruidViewMacroFactory.java
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.sql.calcite.view;
+
+import org.apache.druid.server.security.Escalator;
+import org.apache.druid.sql.calcite.planner.PlannerFactory;
+
+/**
+ * A factory to create a {@link DruidViewMacro} that is used by Guice for Assisted injection.
+ */
+public interface DruidViewMacroFactory
+{
+ /**
+ * Creates an instance of {@link DruidViewMacro}
+ */
+ DruidViewMacro create(PlannerFactory plannerFactory, Escalator escalator, String viewSql);
Review comment:
If I was doing this again, I probably wouldn't have done it this way just to get rid of a static constant being used across a few packages in the sql sub-module.
I got carried away with all the guice magic. But it's a bit of a pain to revert this change now.
----------------------------------------------------------------
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 #9279: Guicify
druid sql module
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r374394978
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidCalciteSchemaModule.java
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.sql.calcite.schema;
+
+import com.google.inject.Binder;
+import com.google.inject.Module;
+import com.google.inject.Provides;
+import com.google.inject.Scopes;
+import com.google.inject.Singleton;
+import com.google.inject.name.Named;
+import com.google.inject.name.Names;
+import org.apache.calcite.schema.SchemaPlus;
+import org.apache.druid.guice.LifecycleModule;
+import org.apache.druid.sql.guice.SqlBindings;
+
+/**
+ * The module responsible for providing bindings
+ */
+public class DruidCalciteSchemaModule implements Module
+{
+ private static final String DRUID_SCHEMA_NAME = "druid";
+ static final String INCOMPLETE_SCHEMA = "INCOMPLETE_SCHEMA";
+
+ @Override
+ public void configure(Binder binder)
+ {
+ binder.bind(String.class).annotatedWith(DruidSchemaName.class).toInstance(DRUID_SCHEMA_NAME);
+
+ // Should only be used by the information schema
+ binder.bind(SchemaPlus.class)
+ .annotatedWith(Names.named(INCOMPLETE_SCHEMA))
+ .toProvider(RootSchemaProvider.class)
+ .in(Scopes.SINGLETON);
+
+ // DruidSchema needs to listen to changes for incoming segments
+ LifecycleModule.register(binder, DruidSchema.class);
+ binder.bind(SystemSchema.class).in(Scopes.SINGLETON);
+ binder.bind(InformationSchema.class).in(Scopes.SINGLETON);
+ binder.bind(LookupSchema.class).in(Scopes.SINGLETON);
+
+ // Binder to inject different schema to Calcite
+ SqlBindings.addSchema(binder, DruidSqlSchema.class);
+ SqlBindings.addSchema(binder, SystemSqlSchema.class);
+ SqlBindings.addSchema(binder, LookupSqlSchema.class);
+ }
+
+ @Provides
+ @Singleton
+ private SchemaPlus getRootSchema(@Named(INCOMPLETE_SCHEMA) SchemaPlus rootSchema, InformationSchema informationSchema)
+ {
+ String name = "INFORMATION_SCHEMA";
Review comment:
Extract to a constant?
----------------------------------------------------------------
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 #9279: Guicify
druid sql module
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r374821017
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/aggregation/SqlAggregationModule.java
##########
@@ -0,0 +1,38 @@
+/*
+ * 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.sql.calcite.aggregation;
+
+import com.google.inject.Binder;
+import com.google.inject.Module;
+import com.google.inject.multibindings.Multibinder;
+
+/**
+ * Module that provides SQL aggregations.
+ * To add an aggregation use {@link org.apache.druid.sql.guice.SqlBindings#addAggregator(Binder, Class)}
+ */
+public class SqlAggregationModule implements Module
+{
+ @Override
+ public void configure(Binder binder)
+ {
+ // Add empty SqlAggregator binder.
+ Multibinder.newSetBinder(binder, SqlAggregator.class);
Review comment:
Hmm, it's not what I would have done but I don't think it's a big deal either way. Please proceed with keeping it this way then.
----------------------------------------------------------------
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 #9279:
Guicify druid sql module
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r372208628
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/aggregation/AggregationModule.java
##########
@@ -0,0 +1,38 @@
+/*
+ * 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.sql.calcite.aggregation;
+
+import com.google.inject.Binder;
+import com.google.inject.Module;
+import com.google.inject.multibindings.Multibinder;
+
+/**
+ * Module that provides SQL aggregations.
+ * To add an aggregation use {@link org.apache.druid.sql.guice.SqlBindings#addAggregator(Binder, Class)}
+ */
+public class AggregationModule implements Module
Review comment:
👍
----------------------------------------------------------------
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 #9279:
Guicify druid sql module
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r374419452
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/aggregation/SqlAggregationModule.java
##########
@@ -0,0 +1,38 @@
+/*
+ * 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.sql.calcite.aggregation;
+
+import com.google.inject.Binder;
+import com.google.inject.Module;
+import com.google.inject.multibindings.Multibinder;
+
+/**
+ * Module that provides SQL aggregations.
+ * To add an aggregation use {@link org.apache.druid.sql.guice.SqlBindings#addAggregator(Binder, Class)}
+ */
+public class SqlAggregationModule implements Module
+{
+ @Override
+ public void configure(Binder binder)
+ {
+ // Add empty SqlAggregator binder.
+ Multibinder.newSetBinder(binder, SqlAggregator.class);
Review comment:
I'd like to leave it in this package because of it's proximity to the SqlAggregator 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 #9279: Guicify
druid sql module
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r374401687
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidSqlSchema.java
##########
@@ -0,0 +1,51 @@
+/*
+ * 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.sql.calcite.schema;
+
+import com.google.inject.Inject;
+import org.apache.calcite.schema.Schema;
+
+/**
+ * The schema for druid tables to be accessible via sql.
+ */
+class DruidSqlSchema implements DruidCalciteSchema
Review comment:
We now have classes DruidSchema, DruidSqlSchema, and DruidCalciteSchema. Their relationships aren't very clear from the names. How about naming them DruidSchema, DruidSchemaProvider, and SchemaProvider respectively?
----------------------------------------------------------------
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 #9279: Guicify
druid sql module
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r374819636
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/view/DruidViewMacroFactory.java
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.sql.calcite.view;
+
+import org.apache.druid.server.security.Escalator;
+import org.apache.druid.sql.calcite.planner.PlannerFactory;
+
+/**
+ * A factory to create a {@link DruidViewMacro} that is used by Guice for Assisted injection.
+ */
+public interface DruidViewMacroFactory
+{
+ /**
+ * Creates an instance of {@link DruidViewMacro}
+ */
+ DruidViewMacro create(PlannerFactory plannerFactory, Escalator escalator, String viewSql);
Review comment:
Well, okay, I'm fine with keeping it this way. It seems a little convoluted but at least I learned something about Guice while reviewing it. Maybe consider adding some comments about what the purpose of all the indirection is, so future code-archaeologists can understand the intent behind it.
----------------------------------------------------------------
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 #9279:
Guicify druid sql module
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r374419870
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidCalciteSchemaModule.java
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.sql.calcite.schema;
+
+import com.google.inject.Binder;
+import com.google.inject.Module;
+import com.google.inject.Provides;
+import com.google.inject.Scopes;
+import com.google.inject.Singleton;
+import com.google.inject.name.Named;
+import com.google.inject.name.Names;
+import org.apache.calcite.schema.SchemaPlus;
+import org.apache.druid.guice.LifecycleModule;
+import org.apache.druid.sql.guice.SqlBindings;
+
+/**
+ * The module responsible for providing bindings
Review comment:
updated
----------------------------------------------------------------
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 #9279: Guicify
druid sql module
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r374407723
##########
File path: sql/src/main/java/org/apache/druid/sql/guice/SqlModule.java
##########
@@ -63,25 +55,21 @@ public void configure(Binder binder)
if (isEnabled()) {
Calcites.setSystemProperties();
- JsonConfigProvider.bind(binder, "druid.sql.planner", PlannerConfig.class);
- JsonConfigProvider.bind(binder, "druid.sql.avatica", AvaticaServerConfig.class);
- LifecycleModule.register(binder, DruidSchema.class);
binder.bind(ViewManager.class).to(NoopViewManager.class).in(LazySingleton.class);
- // Add empty SqlAggregator binder.
- Multibinder.newSetBinder(binder, SqlAggregator.class);
+ binder.install(new DruidCalciteSchemaModule());
Review comment:
The only reason I asked was because I was concerned that having things be too split up would make it hard for people to guess what functionality should be in which file (at least with something named SqlModule it's clear that all the SQL stuff goes in there).
The easier testability is nice though, so you convinced me it's worth it. Let's just make sure it's as clear as possible from the namings what stuff goes in what module (or at least from the javadocs).
----------------------------------------------------------------
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 #9279: Guicify
druid sql module
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9279: Guicify druid sql module
URL: https://github.com/apache/druid/pull/9279#discussion_r372165148
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/aggregation/AggregationModule.java
##########
@@ -0,0 +1,38 @@
+/*
+ * 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.sql.calcite.aggregation;
+
+import com.google.inject.Binder;
+import com.google.inject.Module;
+import com.google.inject.multibindings.Multibinder;
+
+/**
+ * Module that provides SQL aggregations.
+ * To add an aggregation use {@link org.apache.druid.sql.guice.SqlBindings#addAggregator(Binder, Class)}
+ */
+public class AggregationModule implements Module
Review comment:
Even though this is in the SQL module, I think it'd still be better to call it SqlAggregationModule. There's another AggregatorsModule and it would be confusing to see them side by side in an IDE or code search.
----------------------------------------------------------------
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