paul-rogers commented on code in PR #12636:
URL: https://github.com/apache/druid/pull/12636#discussion_r900530972
##########
sql/src/main/java/org/apache/druid/sql/calcite/view/DruidViewMacro.java:
##########
@@ -58,6 +58,7 @@ public TranslatableTable apply(final List<Object> arguments)
{
final RelDataType rowType;
try (final DruidPlanner planner = plannerFactory.createPlanner(viewSql,
new QueryContext())) {
+ planner.validate(false);
Review Comment:
So, this turns out to be another messy/tricky area. The
`authorizeContextParams` flag simply adds the context variables to the set of
resource actions. But, nothing in the above code path checks the resource
actions. In fact, it seems that it can't do so: there is no authorization
result available. So, the code is ambiguous: its claiming to do something that
it can't do.
As part of the auth cleanup, I moved the `authorizeContextParams` flag to
the authorization step, and clearly stated that a statement (or view) can be
prepared without authorization. Only execution (AKA "plan") needs authorization.
The standard way to handle views is to assign them an owner. Views run with
the owner's permissions. Queries that use the view must be authorized against
the view, not against the resources which the view uses. Of course, Druid has
no concept of users, so the idea of "owner" is ill-defined. Maybe we stash a
bundle of permissions with the view or some such? That's a question for another
time.
Context variables are a property of the query, not of the view. So, we
should check them as part of the query check, and not confuse ourselves trying
to figure out if we should check them with views.
Anyway, take a look at the revised code to see if it makes sense.
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java:
##########
@@ -333,6 +371,9 @@ private PlannerResult planWithDruidConvention(
.filter(action -> action.getAction() == Action.READ)
.collect(Collectors.toSet());
+ // TODO: This is not really a state check since there is a race
condition.
Review Comment:
That rework suggested that there are several other areas to clean up, and
that `SqlLifecycleTest` should be rewritten. That will come later, to limit the
blast radius of this PR.
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java:
##########
@@ -333,6 +371,9 @@ private PlannerResult planWithDruidConvention(
.filter(action -> action.getAction() == Action.READ)
.collect(Collectors.toSet());
+ // TODO: This is not really a state check since there is a race
condition.
Review Comment:
Thanks for the comment: forced me to look more closely at this. So, the
check itself is OK as it is a sanity check. The broader issue is whether the
auth check was done: did the caller properly make use of the `ResourceActions`?
This bugged me a bit: it is critical, yet is outside of this planner.
To address that, I reworked how we do authorization: it is now done in the
planner, and is guaranteed by a new planner state. Since the work is done in
the planner, the `VerificationResult` became redundant and was removed. Since
`SqlLifecycleTest` verifies implementation, rather than functionality, that
needed adjustment also.
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/CalcitePlanner.java:
##########
@@ -0,0 +1,432 @@
+/*
+ * 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.apache.calcite.adapter.java.JavaTypeFactory;
+import org.apache.calcite.config.CalciteConnectionConfig;
+import org.apache.calcite.config.CalciteConnectionConfigImpl;
+import org.apache.calcite.config.CalciteConnectionProperty;
+import org.apache.calcite.config.CalciteSystemProperty;
+import org.apache.calcite.jdbc.CalciteSchema;
+import org.apache.calcite.jdbc.JavaTypeFactoryImpl;
+import org.apache.calcite.plan.Context;
+import org.apache.calcite.plan.ConventionTraitDef;
+import org.apache.calcite.plan.RelOptCluster;
+import org.apache.calcite.plan.RelOptPlanner;
+import org.apache.calcite.plan.RelOptTable.ViewExpander;
+import org.apache.calcite.plan.RelOptUtil;
+import org.apache.calcite.plan.RelTraitDef;
+import org.apache.calcite.plan.RelTraitSet;
+import org.apache.calcite.plan.volcano.VolcanoPlanner;
+import org.apache.calcite.prepare.CalciteCatalogReader;
+import org.apache.calcite.rel.RelCollationTraitDef;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rel.RelRoot;
+import org.apache.calcite.rel.metadata.CachingRelMetadataProvider;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeSystem;
+import org.apache.calcite.rex.RexBuilder;
+import org.apache.calcite.rex.RexExecutor;
+import org.apache.calcite.runtime.Hook;
+import org.apache.calcite.schema.SchemaPlus;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlOperatorTable;
+import org.apache.calcite.sql.parser.SqlParseException;
+import org.apache.calcite.sql.parser.SqlParser;
+import org.apache.calcite.sql.validate.SqlConformance;
+import org.apache.calcite.sql.validate.SqlValidator;
+import org.apache.calcite.sql2rel.RelDecorrelator;
+import org.apache.calcite.sql2rel.SqlRexConvertletTable;
+import org.apache.calcite.sql2rel.SqlToRelConverter;
+import org.apache.calcite.tools.FrameworkConfig;
+import org.apache.calcite.tools.Planner;
+import org.apache.calcite.tools.Program;
+import org.apache.calcite.tools.RelBuilder;
+import org.apache.calcite.tools.ValidationException;
+import org.apache.calcite.util.Pair;
+
+import javax.annotation.Nullable;
+
+import java.io.Reader;
+import java.util.List;
+import java.util.Properties;
+
+/**
+ * Calcite planner. Clone of Calcite's {@code SqlPlannerImpl},
+ * but with the validator made accessible.
+ */
+public class CalcitePlanner implements Planner, ViewExpander
Review Comment:
I did have to edit the file quite a bit to convert it from Calcite to Druid
style. I refrained from more than the minimal changes to make it easier to
compare the two. If we like, I can reformat the whole file to be closer to the
Druid format. I would probably hold off changing the code structure to be more
Druid-like. Calcite has its quirks, and it might be safer to leave well enough
alone on that front. Thoughts? Any particularly egregious issues you'd like
addressed?
--
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]
