ihuzenko commented on a change in pull request #1608: DRILL-6960: AutoLimit the
size of ResultSet for a WebUI (or REST) client
URL: https://github.com/apache/drill/pull/1608#discussion_r256348110
##########
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java
##########
@@ -183,4 +188,34 @@ private static PhysicalPlan getQueryPlan(QueryContext
context, String sql, Point
return handler.getPlan(sqlNode);
}
+
+ //Wrap with Auto Limit
+ private static SqlNode checkAndApplyAutoLimit(SqlConverter parser,
QueryContext context, String sql) {
+ SqlNode sqlNode = parser.parse(sql);
Review comment:
Hello @kkhatua , I agree with logic implemented within the
```checkAndApplyAutoLimit(SqlConverter parser, QueryContext context, String
sql)``` method. But it would be nice to make it more readable. Please refactor
this method, as an example you can use my snippet below:
```java
private static SqlNode checkAndApplyAutoLimit(SqlConverter parser,
QueryContext context, String sql) {
SqlNode sqlNode = parser.parse(sql);
if (isAutoLimitShouldBeApplied(context, sqlNode)) {
sqlNode = wrapWithAutoLimit(sqlNode, context);
} else {
context.disableAutoLimit();
}
return sqlNode;
}
private static boolean isAutoLimitShouldBeApplied(QueryContext context,
SqlNode sqlNode) {
return context.isAutoLimitEnabled() &&
sqlNode.getKind().belongsTo(SqlKind.QUERY)
&& (sqlNode.getKind() != SqlKind.ORDER_BY ||
isAutoLimitLessThanOrderByFetch((SqlOrderBy) sqlNode, context));
}
private static boolean isAutoLimitLessThanOrderByFetch(SqlOrderBy orderBy,
QueryContext context) {
return orderBy.fetch == null ||
Integer.parseInt(orderBy.fetch.toString()) > context.getAutoLimitRowCount();
}
private static SqlNode wrapWithAutoLimit(SqlNode sqlNode, QueryContext
context) {
SqlNumericLiteral autoLimitLiteral =
SqlLiteral.createExactNumeric(context.getAutoLimitRowCount().toString(),
SqlParserPos.ZERO);
if (sqlNode.getKind() == SqlKind.ORDER_BY) {
SqlOrderBy orderBy = (SqlOrderBy) sqlNode;
return new SqlOrderBy(orderBy.getParserPosition(), orderBy.query,
orderBy.orderList, orderBy.offset, autoLimitLiteral);
}
return new SqlOrderBy(SqlParserPos.ZERO, sqlNode, SqlNodeList.EMPTY,
null, autoLimitLiteral);
}
```
Note that I removed log.debug statements, I consider this one
```logger.debug("AutoLimit was{}applied", appliedLimit ? " " : " not ");```
redundant because you're logging something like this when filling QueryProfile.
And this statement
```logger.debug("Query text to execute: {}",
sqlNode.toSqlString(HiveSqlDialect.DEFAULT));``` I assume is harmful , because
it's serialization of sqlNode tree back to SQL which will be done for each
query, even when debug log level is disabled.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
