clintropolis commented on code in PR #12636:
URL: https://github.com/apache/druid/pull/12636#discussion_r900669383
##########
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:
>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.
Hmm, currently `DruidStatement` calls `validateAndAuthorize` for both
`prepare` and `execute`, are you saying that `prepare` no longer authorizes or
that it never did? (because looking at the current code it seems to do
>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.
There is a 'VIEW' resource that views are authorized against, not the
resource that the view query uses, which i think is more or less the same thing?
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/SqlParameterizerShuttle.java:
##########
@@ -50,34 +63,36 @@ public SqlParameterizerShuttle(PlannerContext
plannerContext)
public SqlNode visit(SqlDynamicParam param)
{
try {
- if (plannerContext.getParameters().size() > param.getIndex()) {
- TypedValue paramBinding =
plannerContext.getParameters().get(param.getIndex());
- if (paramBinding == null) {
- throw new IAE("Parameter at position[%s] is not bound",
param.getIndex());
- }
- if (paramBinding.value == null) {
- return SqlLiteral.createNull(param.getParserPosition());
- }
- SqlTypeName typeName =
SqlTypeName.getNameForJdbcType(paramBinding.type.typeId);
- if (SqlTypeName.APPROX_TYPES.contains(typeName)) {
- return SqlLiteral.createApproxNumeric(paramBinding.value.toString(),
param.getParserPosition());
- }
- if (SqlTypeName.TIMESTAMP.equals(typeName) && paramBinding.value
instanceof Long) {
- return SqlLiteral.createTimestamp(
- TimestampString.fromMillisSinceEpoch((Long) paramBinding.value),
- 0,
- param.getParserPosition()
- );
- }
-
- return typeName.createLiteral(paramBinding.value,
param.getParserPosition());
- } else {
- throw new IAE("Parameter at position[%s] is not bound",
param.getIndex());
+ if (plannerContext.getParameters().size() <= param.getIndex()) {
+ throw new IAE("Parameter at position [%s] is not bound",
param.getIndex());
+ }
+ TypedValue paramBinding =
plannerContext.getParameters().get(param.getIndex());
+ if (paramBinding == null) {
+ throw new IAE("Parameter at position [%s] is not bound",
param.getIndex());
+ }
+ if (paramBinding.value == null) {
+ return SqlLiteral.createNull(param.getParserPosition());
}
+ SqlTypeName typeName =
SqlTypeName.getNameForJdbcType(paramBinding.type.typeId);
+ if (SqlTypeName.APPROX_TYPES.contains(typeName)) {
+ return SqlLiteral.createApproxNumeric(paramBinding.value.toString(),
param.getParserPosition());
+ }
+ if (SqlTypeName.TIMESTAMP.equals(typeName) && paramBinding.value
instanceof Long) {
+ return SqlLiteral.createTimestamp(
+ TimestampString.fromMillisSinceEpoch((Long) paramBinding.value),
+ 0,
+ param.getParserPosition()
+ );
+ }
+
+ // This throws ClassCastException for a DATE parameter given as
Review Comment:
i'm not personally sure that this is the only place that is expected to
throw this exception, which might be why I had the entire thing wrapped since i
didn't feel confident that I exhaustively tested all possible input types. iirc
my idea was to eat any exceptions we encounter here to let the
`RelParameterizerShuttle` try again later and if it also fails then it we did
all we could.
--
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]
