clintropolis commented on code in PR #18366:
URL: https://github.com/apache/druid/pull/18366#discussion_r2258899207
##########
sql/src/main/java/org/apache/druid/sql/SqlQueryPlus.java:
##########
@@ -65,25 +66,32 @@ 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 Set<String> authContextKeys;
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,
+ Set<String> authContextKeys,
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));
+ // stmtContext is used for query planning and execution
+ this.stmtContext = stmtContext == null
+ ? Collections.emptyMap()
+ : Collections.unmodifiableMap(new
HashMap<>(stmtContext));
Review Comment:
not new, but why `Collections.unmodifiableMap(new HashMap<>(stmtContext))`
instead of just `Collections.unmodifiableMap(stmtContext)`? Intellij also seems
to be suggesting just using `Map.copyOf(stmtContext)`
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerFactory.java:
##########
@@ -142,6 +146,7 @@ public DruidPlanner createPlannerForTesting(
engine,
sql,
statementAndSetContext.getMainStatement(),
+ ImmutableSet.copyOf(queryContext.keySet()),
Review Comment:
nit: can use `Set.copyOf`
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerContext.java:
##########
@@ -176,6 +178,9 @@ private PlannerContext(
this.sql = sql;
this.sqlNode = sqlNode;
this.engine = engine;
+ // authContextKeys is used for security checks
Review Comment:
nit: i think these comments are less necessary with the rename to
authContextKeys since the variable names themselves make it much more clear
what is happening 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]
