paul-rogers commented on code in PR #12845:
URL: https://github.com/apache/druid/pull/12845#discussion_r939799217
##########
processing/src/main/java/org/apache/druid/query/QueryContext.java:
##########
@@ -68,6 +68,19 @@ public QueryContext(@Nullable Map<String, Object> userParams)
invalidateMergedParams();
}
+ public QueryContext(QueryContext from, @Nullable Map<String, Object>
userParams)
Review Comment:
This bit of code was added as part of trying to use the new `SqlQueryPlus`
in the Calcite tests. Some experiments work, others don't. This is one that
added more complexity than it removed, so the latest commit backs out this
change, among others in the Calcite tests.
##########
sql/src/main/resources/saffron.properties:
##########
@@ -0,0 +1,28 @@
+# 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.
+#-------------------------------------------------------------
+#
+# Properties for Calcite (formerly known as "Saffron").
Review Comment:
I think it had several names. I see lots of references in Drill code to
"Optiq" which seems to have been the name after "Saffron" and before "Calcite."
I always thought that "Calcite" used a library called "Saffron", but reading
the initialization code revealed that is not actually the case.
Oh, the useless facts with which we clutter our brains...
##########
sql/src/main/java/org/apache/druid/sql/AbstractStatement.java:
##########
@@ -0,0 +1,203 @@
+/*
+ * 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;
+
+import org.apache.calcite.sql.parser.SqlParseException;
+import org.apache.calcite.tools.ValidationException;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.query.QueryContext;
+import org.apache.druid.query.QueryContexts;
+import org.apache.druid.server.security.Access;
+import org.apache.druid.server.security.AuthorizationUtils;
+import org.apache.druid.server.security.ForbiddenException;
+import org.apache.druid.server.security.ResourceAction;
+import org.apache.druid.sql.calcite.planner.DruidPlanner;
+import org.apache.druid.sql.calcite.planner.PlannerContext;
+import org.apache.druid.sql.calcite.planner.PlannerResult;
+
+import java.io.Closeable;
+import java.util.Set;
+import java.util.UUID;
+import java.util.function.Function;
+
+/**
+ * Represents a SQL statement either for preparation or execution.
+ * A statement is given by a lifecycle context and the statement
+ * to execute. See derived classes for actions. Closing the statement
+ * emits logs and metrics for the statement.
+ */
+public abstract class AbstractStatement implements Closeable
+{
+ private static final Logger log = new Logger(AbstractStatement.class);
+
+ protected final SqlToolbox sqlToolbox;
+ protected final SqlQueryPlus queryPlus;
+ protected final SqlExecutionReporter reporter;
+ protected PlannerContext plannerContext;
+ protected Set<ResourceAction> queryResource;
+ protected Set<ResourceAction> resourceActions;
+
+ public AbstractStatement(
+ final SqlToolbox sqlToolbox,
+ final SqlQueryPlus sqlRequest,
+ final String remoteAddress
+ )
+ {
+ this.sqlToolbox = sqlToolbox;
+ this.queryPlus = sqlRequest;
+ this.reporter = new SqlExecutionReporter(this, remoteAddress);
+
+ // Context is modified, not copied.
+ contextWithSqlId(sqlRequest.context())
+ .addDefaultParams(sqlToolbox.defaultQueryConfig.getContext());
+ }
+
+ private QueryContext contextWithSqlId(QueryContext queryContext)
+ {
+ // "bySegment" results are never valid to use with SQL because the result
format is incompatible
+ // so, overwrite any user specified context to avoid exceptions down the
line
+
+ if (queryContext.removeUserParam(QueryContexts.BY_SEGMENT_KEY) != null) {
+ log.warn("'bySegment' results are not supported for SQL queries,
ignoring query context parameter");
+ }
+ queryContext.addDefaultParam(PlannerContext.CTX_SQL_QUERY_ID,
UUID.randomUUID().toString());
+ return queryContext;
+ }
+
+ public String sqlQueryId()
+ {
+ return queryPlus.context().getAsString(PlannerContext.CTX_SQL_QUERY_ID);
+ }
+
+ /**
+ * Validate SQL query and authorize against any datasources or views which
+ * will take part in the query. Must be called by the API methods, not
+ * directly.
+ */
+ protected void validate(DruidPlanner planner)
+ {
+ plannerContext = planner.getPlannerContext();
+ plannerContext.setAuthenticationResult(queryPlus.authResult());
+ plannerContext.setParameters(queryPlus.parameters());
+ try {
+ planner.validate();
+ }
+ // We can't collapse catch clauses since SqlPlanningException has
+ // type-sensitive constructors.
+ catch (SqlParseException e) {
+ throw new SqlPlanningException(e);
+ }
+ catch (ValidationException e) {
+ throw new SqlPlanningException(e);
+ }
+ }
+
+ /**
+ * Authorize the query using the authorizer provided, and an option to
authorize
+ * context variables as well as query resources.
+ */
+ protected void authorize(
+ DruidPlanner planner,
+ Function<Set<ResourceAction>, Access> authorizer
+ )
+ {
+ boolean authorizeContextParams =
sqlToolbox.authConfig.authorizeQueryContextParams();
+
+ // Authentication is done by the planner using the function provided
+ // here. The planner ensures that this step is done before planning.
+ Access authorizationResult = planner.authorize(authorizer,
authorizeContextParams);
+ if (!authorizationResult.isAllowed()) {
+ throw new ForbiddenException(authorizationResult.toString());
+ }
+
+ queryResource = planner.resourceActions(false);
Review Comment:
This one is subtle and fixes a bug in the current code. Please see the
revised comments to see if they explain the issue clearly.
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidTypeSystem.java:
##########
@@ -29,7 +29,8 @@ public class DruidTypeSystem implements RelDataTypeSystem
public static final DruidTypeSystem INSTANCE = new DruidTypeSystem();
/**
- * Druid uses millisecond precision for timestamps internally. This is also
the default at the SQL layer.
+ * Druid uses millisecond precision for timestamps internally. This is also
+ * the default at the SQL layer.
Review Comment:
Interesting. While the code can sprawl to 120 characters, it is actually
quite hard to read comments at that width. Books, magazines and newspapers have
limited width for this reason. The old 120-column green-bar paper has long
since given way to 80 column (approx.) letter-sized pages.
I wonder, should we allow our code to be 120 characters, but encourage 80
for comments? Here's [a random
reference](https://spin.atomicobject.com/2016/09/24/improving-web-typography/#:~:text=Takeaway%3A%20Set%20text%20column%20widths,around%2075%20characters%20helps%20readability.)
which suggests I'm not alone in this opinion.
If we want to compress the code, loosening our rules for `{` (put on same
line as method declaration) and ')' (append to the last parameter) is a better
way to do so.
In any case, reverted the change here.
##########
sql/src/main/java/org/apache/druid/sql/AbstractStatement.java:
##########
@@ -0,0 +1,203 @@
+/*
+ * 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;
+
+import org.apache.calcite.sql.parser.SqlParseException;
+import org.apache.calcite.tools.ValidationException;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.query.QueryContext;
+import org.apache.druid.query.QueryContexts;
+import org.apache.druid.server.security.Access;
+import org.apache.druid.server.security.AuthorizationUtils;
+import org.apache.druid.server.security.ForbiddenException;
+import org.apache.druid.server.security.ResourceAction;
+import org.apache.druid.sql.calcite.planner.DruidPlanner;
+import org.apache.druid.sql.calcite.planner.PlannerContext;
+import org.apache.druid.sql.calcite.planner.PlannerResult;
+
+import java.io.Closeable;
+import java.util.Set;
+import java.util.UUID;
+import java.util.function.Function;
+
+/**
+ * Represents a SQL statement either for preparation or execution.
+ * A statement is given by a lifecycle context and the statement
+ * to execute. See derived classes for actions. Closing the statement
+ * emits logs and metrics for the statement.
+ */
+public abstract class AbstractStatement implements Closeable
+{
+ private static final Logger log = new Logger(AbstractStatement.class);
+
+ protected final SqlToolbox sqlToolbox;
+ protected final SqlQueryPlus queryPlus;
+ protected final SqlExecutionReporter reporter;
+ protected PlannerContext plannerContext;
+ protected Set<ResourceAction> queryResource;
+ protected Set<ResourceAction> resourceActions;
+
+ public AbstractStatement(
+ final SqlToolbox sqlToolbox,
+ final SqlQueryPlus sqlRequest,
+ final String remoteAddress
+ )
+ {
+ this.sqlToolbox = sqlToolbox;
+ this.queryPlus = sqlRequest;
+ this.reporter = new SqlExecutionReporter(this, remoteAddress);
+
+ // Context is modified, not copied.
+ contextWithSqlId(sqlRequest.context())
+ .addDefaultParams(sqlToolbox.defaultQueryConfig.getContext());
+ }
+
+ private QueryContext contextWithSqlId(QueryContext queryContext)
+ {
+ // "bySegment" results are never valid to use with SQL because the result
format is incompatible
+ // so, overwrite any user specified context to avoid exceptions down the
line
+
+ if (queryContext.removeUserParam(QueryContexts.BY_SEGMENT_KEY) != null) {
+ log.warn("'bySegment' results are not supported for SQL queries,
ignoring query context parameter");
+ }
+ queryContext.addDefaultParam(PlannerContext.CTX_SQL_QUERY_ID,
UUID.randomUUID().toString());
+ return queryContext;
+ }
+
+ public String sqlQueryId()
+ {
+ return queryPlus.context().getAsString(PlannerContext.CTX_SQL_QUERY_ID);
+ }
+
+ /**
+ * Validate SQL query and authorize against any datasources or views which
+ * will take part in the query. Must be called by the API methods, not
+ * directly.
+ */
+ protected void validate(DruidPlanner planner)
+ {
+ plannerContext = planner.getPlannerContext();
+ plannerContext.setAuthenticationResult(queryPlus.authResult());
+ plannerContext.setParameters(queryPlus.parameters());
+ try {
+ planner.validate();
+ }
+ // We can't collapse catch clauses since SqlPlanningException has
+ // type-sensitive constructors.
+ catch (SqlParseException e) {
+ throw new SqlPlanningException(e);
+ }
+ catch (ValidationException e) {
+ throw new SqlPlanningException(e);
+ }
+ }
+
+ /**
+ * Authorize the query using the authorizer provided, and an option to
authorize
+ * context variables as well as query resources.
+ */
+ protected void authorize(
+ DruidPlanner planner,
+ Function<Set<ResourceAction>, Access> authorizer
+ )
+ {
+ boolean authorizeContextParams =
sqlToolbox.authConfig.authorizeQueryContextParams();
+
+ // Authentication is done by the planner using the function provided
+ // here. The planner ensures that this step is done before planning.
+ Access authorizationResult = planner.authorize(authorizer,
authorizeContextParams);
+ if (!authorizationResult.isAllowed()) {
+ throw new ForbiddenException(authorizationResult.toString());
+ }
+
+ queryResource = planner.resourceActions(false);
+ resourceActions = planner.resourceActions(authorizeContextParams);
+ }
+
+ /**
+ * Resource authorizer based on the authentication result
+ * provided earlier.
+ */
+ protected Function<Set<ResourceAction>, Access> authorizer()
+ {
+ return resourceActions ->
+ AuthorizationUtils.authorizeAllResourceActions(
+ queryPlus.authResult(),
+ resourceActions,
+ sqlToolbox.plannerFactory.getAuthorizerMapper()
+ );
+ }
+
+ /**
+ * Plan the query, which also produces the sequence that runs
+ * the query.
+ */
+ protected PlannerResult plan(DruidPlanner planner)
+ {
+ try {
+ return planner.plan();
+ }
+ catch (ValidationException e) {
+ throw new SqlPlanningException(e);
+ }
+ }
+
+ /**
+ * Return the datasource and table resources for this
+ * statement.
+ */
+ public Set<ResourceAction> resources()
+ {
+ return queryResource;
+ }
+
+ public Set<ResourceAction> allResources()
+ {
+ return resourceActions;
+ }
+
+ public SqlQueryPlus sqlRequest()
+ {
+ return queryPlus;
+ }
+
+ public SqlExecutionReporter reporter()
+ {
+ return reporter;
+ }
+
+ /**
+ * Releases resources and emits logs and metrics as defined the
+ * associated reporter.
+ */
+ @Override
+ public void close()
+ {
+ closeQuietly();
+ reporter.emit();
Review Comment:
I don't think that it can, but added the try/catch anyway, since there is no
harm and it may prevent future issues. We should still call `emit`, but with
the any error registered. The error reporter handles both the success and error
cases.
##########
processing/src/main/java/org/apache/druid/query/QueryContext.java:
##########
@@ -68,6 +68,19 @@ public QueryContext(@Nullable Map<String, Object> userParams)
invalidateMergedParams();
}
+ public QueryContext(QueryContext from, @Nullable Map<String, Object>
userParams)
+ {
+ this.defaultParams = new TreeMap<>(from.defaultParams);
+ this.userParams = userParams == null ? new TreeMap<>() : new
TreeMap<>(userParams);
+ this.systemParams = new TreeMap<>(from.systemParams);
+ invalidateMergedParams();
+ }
+
+ public QueryContext withOverrides(Map<String, Object> overrides)
Review Comment:
Also backed out as part of undoing the unsuccessful Calcite test changes.
##########
sql/src/main/java/org/apache/druid/sql/SqlLifecycleManager.java:
##########
@@ -22,40 +22,48 @@
import com.google.common.collect.ImmutableList;
import com.google.errorprone.annotations.concurrent.GuardedBy;
import org.apache.druid.guice.LazySingleton;
-import org.apache.druid.sql.SqlLifecycle.State;
+import org.apache.druid.server.security.ResourceAction;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.Set;
/**
- * This class manages only _authorized_ {@link SqlLifecycle}s submitted via
HTTP,
- * such as {@link org.apache.druid.sql.http.SqlResource}. The main use case of
this class is
- * tracking running queries so that the cancel API can identify the lifecycles
to cancel.
+ * This class manages only <i>authorized</i> {@link DirectStatement}s
submitted via
+ * HTTP, such as {@link org.apache.druid.sql.http.SqlResource}. The main use
case of
+ * this class is tracking running queries so that the cancel API can identify
+ * the statements to cancel.
*
- * This class is thread-safe as there are 2 or more threads that can access
lifecycles at the same time
- * for query running or query canceling.
+ * This class is thread-safe as there are 2 or more threads that can access
+ * statements at the same time for query running or query canceling.
*
- * For managing and canceling native queries, see {@link
org.apache.druid.server.QueryScheduler}.
- * As its name indicates, it also performs resource scheduling for native
queries based on query lanes
+ * For managing and canceling native queries, see
+ * {@link org.apache.druid.server.QueryScheduler}. As its name indicates, it
+ * also performs resource scheduling for native queries based on query lanes
* {@link org.apache.druid.server.QueryLaningStrategy}.
*
* @see org.apache.druid.server.QueryScheduler#cancelQuery(String)
*/
@LazySingleton
public class SqlLifecycleManager
{
+ public interface Cancellable
Review Comment:
Little did I know I was affecting an English accent all these years. I agree
with your analysis. Looks like "cancellation" is the more common form, in both
dialects, so kept that.
##########
sql/src/main/java/org/apache/druid/sql/AbstractStatement.java:
##########
@@ -0,0 +1,203 @@
+/*
+ * 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;
+
+import org.apache.calcite.sql.parser.SqlParseException;
+import org.apache.calcite.tools.ValidationException;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.query.QueryContext;
+import org.apache.druid.query.QueryContexts;
+import org.apache.druid.server.security.Access;
+import org.apache.druid.server.security.AuthorizationUtils;
+import org.apache.druid.server.security.ForbiddenException;
+import org.apache.druid.server.security.ResourceAction;
+import org.apache.druid.sql.calcite.planner.DruidPlanner;
+import org.apache.druid.sql.calcite.planner.PlannerContext;
+import org.apache.druid.sql.calcite.planner.PlannerResult;
+
+import java.io.Closeable;
+import java.util.Set;
+import java.util.UUID;
+import java.util.function.Function;
+
+/**
+ * Represents a SQL statement either for preparation or execution.
+ * A statement is given by a lifecycle context and the statement
+ * to execute. See derived classes for actions. Closing the statement
+ * emits logs and metrics for the statement.
+ */
+public abstract class AbstractStatement implements Closeable
+{
+ private static final Logger log = new Logger(AbstractStatement.class);
+
+ protected final SqlToolbox sqlToolbox;
+ protected final SqlQueryPlus queryPlus;
+ protected final SqlExecutionReporter reporter;
+ protected PlannerContext plannerContext;
+ protected Set<ResourceAction> queryResource;
+ protected Set<ResourceAction> resourceActions;
+
+ public AbstractStatement(
+ final SqlToolbox sqlToolbox,
+ final SqlQueryPlus sqlRequest,
+ final String remoteAddress
+ )
+ {
+ this.sqlToolbox = sqlToolbox;
+ this.queryPlus = sqlRequest;
Review Comment:
Legacy of the old name for `SqlQueryPlus`. 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: [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]
