clintropolis commented on code in PR #18366:
URL: https://github.com/apache/druid/pull/18366#discussion_r2256668461
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/resources/SqlStatementResource.java:
##########
@@ -179,18 +178,23 @@ public Response doPost(@Context final HttpServletRequest
req,
@VisibleForTesting
Response doPost(
- SqlQuery sqlQuery, // Not final: reassigned using createModifiedSqlQuery
+ final SqlQuery sqlQuery,
final HttpServletRequest req
)
{
- final SqlQueryPlus sqlQueryPlus;
+ SqlQueryPlus sqlQueryPlus;
final HttpStatement stmt;
- final QueryContext queryContext;
try {
- sqlQuery = createModifiedSqlQuery(sqlQuery);
- sqlQueryPlus = SqlResource.makeSqlQueryPlus(sqlQuery, req,
defaultQueryConfig);
- queryContext = QueryContext.of(sqlQueryPlus.context());
+ if (sqlQuery.getContext().containsKey(RESULT_FORMAT)) {
+ throw InvalidInput.exception("Query context parameter [%s] is not
allowed", RESULT_FORMAT);
+ }
+ sqlQueryPlus = SqlResource.makeSqlQueryPlus(sqlQuery, req,
DefaultQueryConfig.NIL);
+ // Attach system default query config and result format to the context.
+ sqlQueryPlus = sqlQueryPlus.withContext(
+ ImmutableMap.<String,
Object>builder().putAll(defaultQueryConfig.getContext()).put(RESULT_FORMAT,
sqlQuery.getResultFormat()).build(),
+ sqlQueryPlus.context()
+ );
Review Comment:
any reason not to just pass this into `makeSqlQueryPlus` and do it all there?
##########
sql/src/main/java/org/apache/druid/sql/SqlQueryPlus.java:
##########
@@ -65,25 +65,30 @@ public class SqlQueryPlus
@Nullable
private final SqlNode sqlNode;
private boolean allowSetStatements;
- private final Map<String, Object> queryContext;
+ private final Map<String, Object> stmtContext;
+ private final Map<String, Object> userProvidedContext;
private final List<TypedValue> parameters;
private final AuthenticationResult authResult;
private SqlQueryPlus(
String sql,
SqlNode sqlNode,
boolean allowSetStatements,
- Map<String, Object> queryContext,
+ Map<String, Object> stmtContext,
+ Map<String, Object> userProvidedContext,
List<TypedValue> parameters,
AuthenticationResult authResult
)
{
this.sql = Preconditions.checkNotNull(sql);
this.sqlNode = sqlNode;
this.allowSetStatements = allowSetStatements;
- this.queryContext = queryContext == null
- ? Collections.emptyMap()
- : Collections.unmodifiableMap(new
HashMap<>(queryContext));
+ this.stmtContext = stmtContext == null
Review Comment:
i think we need comments or javadoc, like initially reading this I wonder
"what is `stmtContext`?", Like it isn't obvious completely obvious that this
context is the one for query stuff and the userProvidedContext is just there
for auth stuff. I think we have to be super clear with what all of the
different context stuff is everywhere or this is going to be too confusing.
##########
sql/src/test/java/org/apache/druid/sql/avatica/DruidAvaticaHandlerTest.java:
##########
@@ -278,6 +278,7 @@ public void setUp() throws Exception
binder.bind(AuthorizerMapper.class).toInstance(CalciteTests.TEST_AUTHORIZER_MAPPER);
binder.bind(Escalator.class).toInstance(CalciteTests.TEST_AUTHENTICATOR_ESCALATOR);
binder.install(new PolicyModule());
+
binder.bind(DefaultQueryConfig.class).toInstance(DefaultQueryConfig.NIL);
Review Comment:
would be nice to have a test with system default query context for jdbc
##########
sql/src/main/java/org/apache/druid/sql/SqlQueryPlus.java:
##########
@@ -180,7 +210,13 @@ public Builder sql(String sql)
return this;
}
- public Builder context(Map<String, Object> queryContext)
+ public Builder systemDefaultContext(Map<String, Object>
systemDefaultContext)
+ {
+ this.systemDefaultContext = systemDefaultContext;
+ return this;
+ }
+
+ public Builder queryContext(Map<String, Object> queryContext)
Review Comment:
also needs javadoc to explain in more detail what these different contexts
are
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerContext.java:
##########
@@ -176,6 +178,7 @@ private PlannerContext(
this.sql = sql;
this.sqlNode = sqlNode;
this.engine = engine;
+ this.userProvidedQueryContext = userProvidedQueryContext;
Review Comment:
same though about comments or javadoc to explain the differences between
`queryContext` and `userProvidedQueryContext`
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerFactory.java:
##########
@@ -109,6 +109,7 @@ public DruidPlanner createPlanner(
final SqlEngine engine,
final String sql,
final SqlNode sqlNode,
+ final Map<String, Object> userProvidedContext,
Review Comment:
nit: teriminology seems a bit inconsistent across files, some places is
`userProvidedContext` and others `userProvidedQueryContext` (and again another
place where it would be useful to indicate what is in queryContext and why
userProvidedContext exists)
##########
sql/src/main/java/org/apache/druid/sql/SqlQueryPlus.java:
##########
@@ -65,25 +65,30 @@ public class SqlQueryPlus
@Nullable
private final SqlNode sqlNode;
private boolean allowSetStatements;
- private final Map<String, Object> queryContext;
+ private final Map<String, Object> stmtContext;
+ private final Map<String, Object> userProvidedContext;
private final List<TypedValue> parameters;
private final AuthenticationResult authResult;
private SqlQueryPlus(
String sql,
SqlNode sqlNode,
boolean allowSetStatements,
- Map<String, Object> queryContext,
+ Map<String, Object> stmtContext,
+ Map<String, Object> userProvidedContext,
List<TypedValue> parameters,
AuthenticationResult authResult
)
{
this.sql = Preconditions.checkNotNull(sql);
this.sqlNode = sqlNode;
this.allowSetStatements = allowSetStatements;
- this.queryContext = queryContext == null
- ? Collections.emptyMap()
- : Collections.unmodifiableMap(new
HashMap<>(queryContext));
+ this.stmtContext = stmtContext == null
+ ? Collections.emptyMap()
+ : Collections.unmodifiableMap(new
HashMap<>(stmtContext));
+ this.userProvidedContext = userProvidedContext == null
Review Comment:
i wonder if instead of `userProvided` if this should be something about how
they still need authorization? (since that is what is really going on here)
--
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]
