[GitHub] [druid] paul-rogers commented on a diff in pull request #13766: Operator conversion deny list

["paul-rogers (via GitHub)" <gi...@apache.org>]

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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


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