You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "zachjsh (via GitHub)" <gi...@apache.org> on 2023/02/07 21:07:21 UTC
[GitHub] [druid] zachjsh opened a new pull request, #13766: Operator conversion deny list
zachjsh opened a new pull request, #13766:
URL: https://github.com/apache/druid/pull/13766
### Description
This change adds a new config property `druid.sql.planner.operatorConversion.denyList`, which allows a user to specify
any operator conversions that they wish to disallow. A user may want to do this for a number of reasons, including security concerns. The default value of this property is the empty list `[]`, which does not disallow any operator conversions.
An example usage of this property is `druid.sql.planner.operatorConversion.denyList=["extern"]`, which disallows the usage of the `extern` operator conversion. If the property is configured this way, and a user of the Druid cluster tries to submit a query that uses the `extern` function, such as the example given [here](https://druid.apache.org/docs/latest/multi-stage-query/examples.html#insert-with-no-rollup), a response with http response code `400` is returned with en error body similar to the following:
```
{
"taskId": "4ec5b0b6-fa9b-4c3a-827d-2308294e9985",
"state": "FAILED",
"error": {
"error": "Plan validation failed",
"errorMessage": "org.apache.calcite.runtime.CalciteContextException: From line 28, column 5 to line 32, column 5: No match found for function signature EXTERN(<CHARACTER>, <CHARACTER>, <CHARACTER>)",
"errorClass": "org.apache.calcite.tools.ValidationException",
"host": null
}
}
```
This PR has:
- [x] 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.)
- [x] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] 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/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [x] 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.
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
[GitHub] [druid] zachjsh commented on a diff in pull request #13766: Operator conversion deny list
Posted by "zachjsh (via GitHub)" <gi...@apache.org>.
zachjsh commented on code in PR #13766:
URL: https://github.com/apache/druid/pull/13766#discussion_r1100563040
##########
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()
+ {
+ return new PlannerOperatorConversionConfig().toBuilder();
+ }
+
+ public Builder toBuilder()
+ {
+ return new Builder(this);
+ }
+
+ /**
+ * Builder for {@link PlannerConfig}, primarily for use in tests to
Review Comment:
fixed
##########
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:
fixed
--
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
[GitHub] [druid] zachjsh commented on a diff in pull request #13766: Operator conversion deny list
Posted by "zachjsh (via GitHub)" <gi...@apache.org>.
zachjsh commented on code in PR #13766:
URL: https://github.com/apache/druid/pull/13766#discussion_r1100565324
##########
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.",
+ operatorKey.name);
Review Comment:
This could lead to information leak, as we may not want to expose which functions were explicitly put on deny list, but rather have it fail in generic way. Let's revisit this at later time, if necessary.
##########
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:
fixed
--
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
[GitHub] [druid] zachjsh commented on a diff in pull request #13766: Operator conversion deny list
Posted by "zachjsh (via GitHub)" <gi...@apache.org>.
zachjsh commented on code in PR #13766:
URL: https://github.com/apache/druid/pull/13766#discussion_r1100563578
##########
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:
fixed
##########
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:
removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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
[GitHub] [druid] abhishekagarwal87 commented on pull request #13766: Operator conversion deny list
Posted by "abhishekagarwal87 (via GitHub)" <gi...@apache.org>.
abhishekagarwal87 commented on PR #13766:
URL: https://github.com/apache/druid/pull/13766#issuecomment-1425265622
if it's for security concerns, can we tie this to role-based access control? Like how we did for query context params. Also, would you want to disable access to an operator or would you like to disable access to an underlying resource that the said operator accesses? So if you're going to disallow access to external resources, wouldn't it be better for an admin to remove "External read" permission, no matter which operator needs that access?
--
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
[GitHub] [druid] zachjsh commented on a diff in pull request #13766: Operator conversion deny list
Posted by "zachjsh (via GitHub)" <gi...@apache.org>.
zachjsh commented on code in PR #13766:
URL: https://github.com/apache/druid/pull/13766#discussion_r1100564226
##########
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:
Removed the evil mocks. Decided to keep this test otherwise, if ok with you.
--
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
[GitHub] [druid] zachjsh commented on a diff in pull request #13766: Operator conversion deny list
Posted by "zachjsh (via GitHub)" <gi...@apache.org>.
zachjsh commented on code in PR #13766:
URL: https://github.com/apache/druid/pull/13766#discussion_r1101964864
##########
sql/src/test/java/org/apache/druid/sql/calcite/planner/PlannerOperatorConfigTest.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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.common.collect.ImmutableList;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class PlannerOperatorConfigTest
+{
+ @Test
Review Comment:
Thanks! Simplified the config class unit tests with suggestion.
--
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
[GitHub] [druid] abhishekrb19 commented on a diff in pull request #13766: Operator conversion deny list
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #13766:
URL: https://github.com/apache/druid/pull/13766#discussion_r1099264267
##########
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()
+ {
+ return new PlannerOperatorConversionConfig().toBuilder();
+ }
+
+ public Builder toBuilder()
+ {
+ return new Builder(this);
+ }
+
+ /**
+ * Builder for {@link PlannerConfig}, primarily for use in tests to
Review Comment:
Copy-paste issue?
`PlannerConfig` -> `PlannerOperatorConversionConfig`
##########
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.",
+ operatorKey.name);
Review Comment:
Instead of relying on Calcite validation, should we throw an exception with a user-friendly message that would guide the user not to use blocked operators without exposing the internals?
--
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
[GitHub] [druid] paul-rogers commented on a diff in pull request #13766: Operator conversion deny list
Posted by "paul-rogers (via GitHub)" <gi...@apache.org>.
paul-rogers commented on code in PR #13766:
URL: https://github.com/apache/druid/pull/13766#discussion_r1101911957
##########
sql/src/test/java/org/apache/druid/sql/calcite/planner/PlannerOperatorConfigTest.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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.common.collect.ImmutableList;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class PlannerOperatorConfigTest
+{
+ @Test
Review Comment:
Got nailed by code coverage? `EqualsVerifier` is what you want here. See `DatasourceTableTest.java` for an example, or just search for the class name.
--
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
[GitHub] [druid] paul-rogers commented on a diff in pull request #13766: Operator conversion deny list
Posted by "paul-rogers (via GitHub)" <gi...@apache.org>.
paul-rogers commented on code in PR #13766:
URL: https://github.com/apache/druid/pull/13766#discussion_r1100666897
##########
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.",
+ operatorKey.name);
Review Comment:
This is a log message that would not normally be visible to the user. This is not a message given to the end user. The end user will get Calcite's usual message that "function foo with arguments VARCHAR, BIGINT, is not found."
--
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
[GitHub] [druid] zachjsh commented on a diff in pull request #13766: Operator conversion deny list
Posted by "zachjsh (via GitHub)" <gi...@apache.org>.
zachjsh commented on code in PR #13766:
URL: https://github.com/apache/druid/pull/13766#discussion_r1100563357
##########
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:
good suggestion, fixed
##########
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:
Fixed
--
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
[GitHub] [druid] zachjsh commented on a diff in pull request #13766: Operator conversion deny list
Posted by "zachjsh (via GitHub)" <gi...@apache.org>.
zachjsh commented on code in PR #13766:
URL: https://github.com/apache/druid/pull/13766#discussion_r1100563840
##########
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:
fixed
--
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
[GitHub] [druid] paul-rogers commented on pull request #13766: Operator conversion deny list
Posted by "paul-rogers (via GitHub)" <gi...@apache.org>.
paul-rogers commented on PR #13766:
URL: https://github.com/apache/druid/pull/13766#issuecomment-1426105902
Just for the record, the goal of this PR is to block input sources not supported in a given cluster. The _right_ way to do this is to create a deny list for input sources, and block them in all the places we use them. Blocking SQL leaves a hole for native ingest. It leaves a hole if an extension provides a table function that uses a disallowed input source, etc. Blocking at the site of use allows a better error message: "the Druid input source foo is not supported in this installation."
--
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
[GitHub] [druid] paul-rogers commented on pull request #13766: Operator conversion deny list
Posted by "paul-rogers (via GitHub)" <gi...@apache.org>.
paul-rogers commented on PR #13766:
URL: https://github.com/apache/druid/pull/13766#issuecomment-1426106463
The build here is failing due to ARM. However, ARM is failing on all builds, so we can treat this build as green.
--
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
[GitHub] [druid] zachjsh merged pull request #13766: Operator conversion deny list
Posted by "zachjsh (via GitHub)" <gi...@apache.org>.
zachjsh merged PR #13766:
URL: https://github.com/apache/druid/pull/13766
--
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
[GitHub] [druid] zachjsh commented on a diff in pull request #13766: Operator conversion deny list
Posted by "zachjsh (via GitHub)" <gi...@apache.org>.
zachjsh commented on code in PR #13766:
URL: https://github.com/apache/druid/pull/13766#discussion_r1100565821
##########
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:
fixed
--
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
[GitHub] [druid] paul-rogers commented on a diff in pull request #13766: Operator conversion deny list
Posted by "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