paul-rogers commented on code in PR #13766:
URL: https://github.com/apache/druid/pull/13766#discussion_r1099279211
##########
docs/configuration/index.md:
##########
@@ -1919,6 +1919,7 @@ The Druid SQL server is configured through the following
properties on the Broke
|`druid.sql.planner.authorizeSystemTablesDirectly`|If true, Druid authorizes
queries against any of the system schema tables (`sys` in SQL) as
`SYSTEM_TABLE` resources which require `READ` access, in addition to
permissions based content filtering.|false|
|`druid.sql.planner.useNativeQueryExplain`|If true, `EXPLAIN PLAN FOR` will
return the explain plan as a JSON representation of equivalent native query(s),
else it will return the original version of explain plan generated by Calcite.
It can be overridden per query with `useNativeQueryExplain` context key.|true|
|`druid.sql.planner.maxNumericInFilters`|Max limit for the amount of numeric
values that can be compared for a string type dimension when the entire SQL
WHERE clause of a query translates to an [OR](../querying/filters.md#or) of
[Bound filter](../querying/filters.md#bound-filter). By default, Druid does not
restrict the amount of numeric Bound Filters on String columns, although this
situation may block other queries from running. Set this property to a smaller
value to prevent Druid from running queries that have prohibitively long
segment processing times. The optimal limit requires some trial and error; we
recommend starting with 100. Users who submit a query that exceeds the limit
of `maxNumericInFilters` should instead rewrite their queries to use strings in
the `WHERE` clause instead of numbers. For example, `WHERE someString IN
(‘123’, ‘456’)`. If this value is disabled, `maxNumericInFilters` set through
query context is ignored.|`-1` (disabled)|
+|`druid.sql.planner.operatorConversion.denyList`| The list of operator
conversions that should be denied.|`[]` (no operator conversions are
disallowed)|
Review Comment:
The `operatorConversion` is an obscure Druid concept. For users, it should
just be `operator`.
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidOperatorTable.java:
##########
@@ -437,6 +442,12 @@ public DruidOperatorTable(
for (SqlOperatorConversion operatorConversion : operatorConversions) {
final OperatorKey operatorKey =
OperatorKey.of(operatorConversion.calciteOperator());
+ if (operationConversionDenySet.contains(operatorKey.name)) {
+ LOG.info(
+ "operatorKey '%s' not being added to available operatorConversions
as it was found in deny list.",
Review Comment:
Perhaps:
```
Operator %s is not available as it is in the deny list.
```
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerOperatorConversionConfig.java:
##########
@@ -0,0 +1,108 @@
+/*
+ * 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.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableList;
+
+import java.util.List;
+import java.util.Objects;
+
+public class PlannerOperatorConversionConfig
+{
+ private static final List<String> DEFAULT_DENY_LIST = ImmutableList.of();
+ @JsonProperty
+ private List<String> denyList = DEFAULT_DENY_LIST;
+
+
+ public List<String> getDenyList()
+ {
+ return denyList;
Review Comment:
I wonder, can this cause an NPE? The code handles the case in which the user
never sets the property. But what if they do this:
```
druid...denyList=null
```
Should we play it safe? Omit the default value and just do:
```java
return denyList == null ? ImmutableList.of() : denyList;
```
Seems OK since the method will only ever be called once, when building the
operator table.
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/CalcitePlannerModule.java:
##########
@@ -40,6 +40,7 @@ public void configure(Binder binder)
// We're actually binding the class to the config prefix, not the other
way around.
JsonConfigProvider.bind(binder, "druid.sql.planner", PlannerConfig.class);
JsonConfigProvider.bind(binder, "druid.sql.planner",
SegmentMetadataCacheConfig.class);
+ JsonConfigProvider.bind(binder, "druid.sql.planner.operatorConversion",
PlannerOperatorConversionConfig.class);
Review Comment:
It is very useful to put the path in the config object:
```java
class PlannerOperatorConversionConfig {
public static final String CONFIG_PATH =
"druid.sql.planner.operatorConversion";
```
And:
```java
JsonConfigProvider.bind(binder, PlannerOperatorConversionConfig.CONFIG_PATH,
PlannerOperatorConversionConfig.class);
```
##########
sql/src/test/java/org/apache/druid/sql/calcite/planner/DruidOperatorTableTest.java:
##########
@@ -0,0 +1,129 @@
+/*
+ * 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.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import org.apache.druid.catalog.model.TableDefnRegistry;
+import org.apache.druid.catalog.model.table.TableFunction;
+import org.apache.druid.sql.calcite.expression.SqlOperatorConversion;
+import org.apache.druid.sql.calcite.external.ExternalOperatorConversion;
+import org.apache.druid.sql.calcite.external.HttpOperatorConversion;
+import org.apache.druid.sql.calcite.external.InlineOperatorConversion;
+import org.apache.druid.sql.calcite.external.LocalOperatorConversion;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Answers;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import static org.mockito.ArgumentMatchers.any;
+
+@RunWith(MockitoJUnitRunner.class)
+public class DruidOperatorTableTest
+{
+ private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
+
+ @Mock (answer = Answers.RETURNS_DEEP_STUBS) TableDefnRegistry
tableDefnRegistry;
+
+ @Mock TableFunction tableFunction;
+ private DruidOperatorTable operatorTable;
+
+ @Test
+ public void
test_operatorTable_with_operatorConversionDenyList_deniedConversionsUnavailable()
+ {
+ Mockito.when(tableDefnRegistry.jsonMapper()).thenReturn(OBJECT_MAPPER);
Review Comment:
This test is overkill. First, there should be no need for mocks. If we want
to use the actual extern operators, just create a real instance.
```
protected final TableDefnRegistry tableRegistry;
...
this.tableRegistry = new TableDefnRegistry(jsonMapper);
```
Mocks are evil: they will cause this test to silently fail if we change the
`TableDefnRegistry` class. Direct use will trigger compile errors if we change
the interface.
Second, there is no need to actually use these functions. The deny list is
generic: you can use _any_ built in function. Just register as done in other
tests, then resolve.
In fact, you can create a `Calcite<Something>Test` so this can be tested via
an actual query, if you wanted. Else, just look how those tests set up the
operator registry.
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerOperatorConversionConfig.java:
##########
@@ -0,0 +1,108 @@
+/*
+ * 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.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableList;
+
+import java.util.List;
+import java.util.Objects;
+
+public class PlannerOperatorConversionConfig
Review Comment:
Please rename to `PlannerOperatorConfig`. Again, the "conversion" bit is
just a bit of internal plumbing.
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerOperatorConversionConfig.java:
##########
@@ -0,0 +1,108 @@
+/*
+ * 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.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableList;
+
+import java.util.List;
+import java.util.Objects;
+
+public class PlannerOperatorConversionConfig
+{
+ private static final List<String> DEFAULT_DENY_LIST = ImmutableList.of();
+ @JsonProperty
+ private List<String> denyList = DEFAULT_DENY_LIST;
+
+
+ public List<String> getDenyList()
+ {
+ return denyList;
+ }
+
+ @Override
+ public boolean equals(final Object o)
+ {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ final PlannerOperatorConversionConfig that =
(PlannerOperatorConversionConfig) o;
+ return denyList.equals(that.denyList);
+ }
+
+ @Override
+ public int hashCode()
+ {
+
+ return Objects.hash(
+ denyList
+ );
+ }
+
+ @Override
+ public String toString()
+ {
+ return "PlannerOperatorConversionConfig{" +
+ "denyList=" + denyList +
+ '}';
+ }
+
+ public static Builder builder()
Review Comment:
This class is so simple it doesn't need a builder. Instead, create a static
method:
```
public static newInstance(List<String> denyList)
{
PlannerOperatorConversionConfig config = new
PlannerOperatorConversionConfig();
config.denyList = denyList;
return config;
}
```
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerOperatorConversionConfig.java:
##########
@@ -0,0 +1,108 @@
+/*
+ * 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.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableList;
+
+import java.util.List;
+import java.util.Objects;
+
+public class PlannerOperatorConversionConfig
+{
+ private static final List<String> DEFAULT_DENY_LIST = ImmutableList.of();
+ @JsonProperty
+ private List<String> denyList = DEFAULT_DENY_LIST;
+
+
+ public List<String> getDenyList()
+ {
+ return denyList;
+ }
+
+ @Override
+ public boolean equals(final Object o)
+ {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ final PlannerOperatorConversionConfig that =
(PlannerOperatorConversionConfig) o;
+ return denyList.equals(that.denyList);
+ }
+
+ @Override
+ public int hashCode()
+ {
+
+ return Objects.hash(
+ denyList
+ );
Review Comment:
Nit: remove blank line. Call to `hash()` can be all on one line.
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerOperatorConversionConfig.java:
##########
@@ -0,0 +1,108 @@
+/*
+ * 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.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableList;
+
+import java.util.List;
+import java.util.Objects;
+
+public class PlannerOperatorConversionConfig
+{
+ private static final List<String> DEFAULT_DENY_LIST = ImmutableList.of();
+ @JsonProperty
+ private List<String> denyList = DEFAULT_DENY_LIST;
Review Comment:
Nit: No real need to define a constant. Only one instance of this class will
ever exist per Druid server. Except in tests.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
