abhishekagarwal87 commented on code in PR #17101:
URL: https://github.com/apache/druid/pull/17101#discussion_r1766266808
##########
sql/src/test/java/org/apache/druid/quidem/DruidQuidemCommandHandler.java:
##########
@@ -141,25 +156,21 @@ protected void executeExplain(Context x) throws Exception
{
DruidConnectionExtras connectionExtras = (DruidConnectionExtras)
x.connection();
ObjectMapper objectMapper = connectionExtras.getObjectMapper();
- QueryLogHook qlh = new QueryLogHook(objectMapper);
- qlh.logQueriesForGlobal(
- () -> {
- executeQuery(x);
- }
- );
-
- List<Query<?>> queries = qlh.getRecordedQueries();
-
- queries = queries
- .stream()
- .map(q -> BaseCalciteQueryTest.recursivelyClearContext(q,
objectMapper))
- .collect(Collectors.toList());
+ DruidHookDispatcher dhp = unwrapDruidHookDispatcher(x);
+ List<Query<?>> logged = new ArrayList<>();
+ try (Closeable unhook = dhp.withHook(DruidHook.NATIVE_PLAN, (key,
relNode) -> {
Review Comment:
```suggestion
try (Closeable unhook = dhp.withHook(DruidHook.NATIVE_PLAN, (key,
query) -> {
```
##########
sql/src/test/java/org/apache/druid/quidem/DruidQuidemCommandHandler.java:
##########
@@ -107,9 +104,27 @@ public final void execute(Context x, boolean execute)
protected final void executeQuery(Context x)
{
final SqlCommand sqlCommand = x.previousSqlCommand();
+ executeQuery(x, sqlCommand.sql);
+ }
+
+ protected final void executeExplainQuery(Context x)
+ {
+ boolean isExplainSupported =
DruidConnectionExtras.unwrapOrThrow(x.connection()).isExplainSupported();
+
+ final SqlCommand sqlCommand = x.previousSqlCommand();
+
+ if (isExplainSupported) {
+ executeQuery(x, "explain plan for " + sqlCommand.sql);
+ } else {
+ executeQuery(x, sqlCommand.sql);
+ }
+ }
+
+ protected final void executeQuery(Context x, String sql)
Review Comment:
can you rename `x` to something more descriptive like `context`? would have
made easier to follow the code.
##########
sql/src/test/java/org/apache/druid/quidem/DruidQuidemCommandHandler.java:
##########
@@ -141,25 +156,21 @@ protected void executeExplain(Context x) throws Exception
{
DruidConnectionExtras connectionExtras = (DruidConnectionExtras)
x.connection();
ObjectMapper objectMapper = connectionExtras.getObjectMapper();
- QueryLogHook qlh = new QueryLogHook(objectMapper);
- qlh.logQueriesForGlobal(
- () -> {
- executeQuery(x);
- }
- );
-
- List<Query<?>> queries = qlh.getRecordedQueries();
-
- queries = queries
- .stream()
- .map(q -> BaseCalciteQueryTest.recursivelyClearContext(q,
objectMapper))
- .collect(Collectors.toList());
+ DruidHookDispatcher dhp = unwrapDruidHookDispatcher(x);
Review Comment:
while I can follow whats happening here, would be nice to add some comments
alongside the code.
##########
sql/src/test/java/org/apache/druid/sql/calcite/util/SqlTestFramework.java:
##########
@@ -197,6 +197,8 @@ default void configureGuice(CoreInjectorBuilder
injectorBuilder, List<Module> ov
{
configureGuice(injectorBuilder);
}
+
+ Boolean isExplainSupported();
Review Comment:
Please add some javadocs 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]
