imply-cheddar commented on code in PR #14232:
URL: https://github.com/apache/druid/pull/14232#discussion_r1233505654
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerFactory.java:
##########
@@ -163,6 +164,7 @@ private FrameworkConfig buildFrameworkConfig(PlannerContext
plannerContext)
.typeSystem(DruidTypeSystem.INSTANCE)
.defaultSchema(rootSchema.getSubSchema(druidSchemaName))
.sqlToRelConverterConfig(sqlToRelConverterConfig)
+ .costFactory(new DruidVolcanoCost.Factory())
Review Comment:
You have the if statement below to add this if it's in DECOUPLED mode, but
it would appear as though you are adding it in all instances as well. It's
interesting that all tests continue to pass, which should give us confidence to
just attach this in all cases, but let's be conservative and minimize impact to
other modes of planning for now, so I think we can remove this.
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerFactory.java:
##########
@@ -191,7 +193,15 @@ public SqlConformance conformance()
return null;
}
}
- })
- .build();
+ });
+
+ if (plannerConfig().getNativeQuerySqlPlanningMode()
+
.equals(PlannerConfig.NATIVE_QUERY_SQL_PLANNING_MODE_DECOUPLED)
Review Comment:
`getNativeQuerySqlPlanningMode()` could maybe be null and generate an NPE.
There is perhaps a `@NotNull` annotation to try to protect from it, but you can
just write your equals better and not have to rely on such things:
```
PlannerConfig.NATIVE_QUERY_SQL_PLANNING_MODE_DECOUPLED.equals(plannerConfig().getNativeSqlPlanningMode())
```
--
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]
