[GitHub] kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit Wrapping should not apply to non-select query
kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit
Wrapping should not apply to non-select query
URL: https://github.com/apache/drill/pull/1608#discussion_r254118821
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java
##
@@ -84,9 +85,14 @@
private boolean closed = false;
private DrillOperatorTable table;
- public QueryContext(final UserSession session, final DrillbitContext
drillbitContext, QueryId queryId) {
+ public QueryContext(UserSession session, DrillbitContext drillbitContext,
QueryId queryId) {
+this(session, drillbitContext, queryId, null);
+ }
+
+ public QueryContext(UserSession session, DrillbitContext drillbitContext,
QueryId queryId, Integer autoLimit) {
Review comment:
I'm effectively creating an additional constructor with a modified
signature, so as to not change anything else. We cant go the "query option"
solution, so let me see if I can do without it.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit Wrapping should not apply to non-select query
kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit
Wrapping should not apply to non-select query
URL: https://github.com/apache/drill/pull/1608#discussion_r254115477
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java
##
@@ -183,4 +183,20 @@ private static PhysicalPlan getQueryPlan(QueryContext
context, String sql, Point
return handler.getPlan(sqlNode);
}
+
+ //Wrap with Auto Limit
+ private static SqlNode wrapWithAutoLimit(SqlConverter parser, QueryContext
context, String sql) {
+SqlNode sqlNode = parser.parse(sql);
Review comment:
This helps a lot! I've no knowledge of the Calcite SqlNodes, so this snippet
helps.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit Wrapping should not apply to non-select query
kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit
Wrapping should not apply to non-select query
URL: https://github.com/apache/drill/pull/1608#discussion_r254106628
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileWrapper.java
##
@@ -150,6 +150,14 @@ private long
tallyMajorFragmentCost(List majorFragments) {
return globalProcessNanos;
}
+ public boolean hasAutoLimit() {
+return profile.hasAutoLimit();
+ }
+
+ public int getAutoLimit() {
+return profile.getAutoLimit();
Review comment:
My statement above... I don't think the query option is the right thing to
do, since this is a WebUI interaction feature.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit Wrapping should not apply to non-select query
kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit
Wrapping should not apply to non-select query
URL: https://github.com/apache/drill/pull/1608#discussion_r254106698
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/work/user/UserWorker.java
##
@@ -73,9 +73,13 @@ private static QueryId queryIdGenerator() {
}
public QueryId submitWork(UserClientConnection connection, RunQuery query) {
+return submitWork(connection, query, null);
+ }
+
+ public QueryId submitWork(UserClientConnection connection, RunQuery query,
Integer autoLimitRowCount) {
Review comment:
Agreed.. let me take a look at modifying RunQuery
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit Wrapping should not apply to non-select query
kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit
Wrapping should not apply to non-select query
URL: https://github.com/apache/drill/pull/1608#discussion_r254106312
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java
##
@@ -136,15 +136,30 @@
*/
public Foreman(final WorkerBee bee, final DrillbitContext drillbitContext,
final UserClientConnection connection, final QueryId queryId, final
RunQuery queryRequest) {
+this(bee, drillbitContext, connection, queryId, queryRequest, null);
+ }
+
+ /**
+ * Constructor. Sets up the Foreman, but does not initiate any execution.
+ *
+ * @param bee work manager (runs fragments)
+ * @param drillbitContext drillbit context
+ * @param connection connection
+ * @param queryId the id for the query
+ * @param queryRequest the query to execute
+ * @param autoLimitRowCount the number of rows to limit in execution (used
for WebRequests)
+ */
+ public Foreman(final WorkerBee bee, final DrillbitContext drillbitContext,
Review comment:
I was reluctant to make too many changes to the protobuf, so I avoided
modifying RunQuery. I can try looking at the work in changing the protobuf for
this.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit Wrapping should not apply to non-select query
kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit
Wrapping should not apply to non-select query
URL: https://github.com/apache/drill/pull/1608#discussion_r254106312
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java
##
@@ -136,15 +136,30 @@
*/
public Foreman(final WorkerBee bee, final DrillbitContext drillbitContext,
final UserClientConnection connection, final QueryId queryId, final
RunQuery queryRequest) {
+this(bee, drillbitContext, connection, queryId, queryRequest, null);
+ }
+
+ /**
+ * Constructor. Sets up the Foreman, but does not initiate any execution.
+ *
+ * @param bee work manager (runs fragments)
+ * @param drillbitContext drillbit context
+ * @param connection connection
+ * @param queryId the id for the query
+ * @param queryRequest the query to execute
+ * @param autoLimitRowCount the number of rows to limit in execution (used
for WebRequests)
+ */
+ public Foreman(final WorkerBee bee, final DrillbitContext drillbitContext,
Review comment:
I was reluctant to make too many changes to the protobuf, so I avoided
modifying RunQuery.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit Wrapping should not apply to non-select query
kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit
Wrapping should not apply to non-select query
URL: https://github.com/apache/drill/pull/1608#discussion_r254105360
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java
##
@@ -273,6 +279,29 @@ public RemoteFunctionRegistry getRemoteFunctionRegistry()
{
return drillbitContext.getRemoteFunctionRegistry();
}
+ /**
+ * Check if auto-limiting of resultset is enabled
+ * @return True if auto-limit is enabled
+ */
+ public boolean isAutoLimitEnabled() {
+return autoLimitRowCount != null;
+ }
+
+ /**
+ * Returns the maximum size of auto-limited resultset
+ * @return Maximum size of auto-limited resultSet
+ */
+ public Integer getAutoLimitRowCount() {
Review comment:
The feature is specifically for WebUI submitted queries. A query option
doesn't make sense because WebUI queries are "session-less" and the only reason
we want to wrap with a limit is to prevent the Drillbit from being overloaded
with holding an entire result set that does not get completely consumed by the
HTTP client.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit Wrapping should not apply to non-select query
kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit
Wrapping should not apply to non-select query
URL: https://github.com/apache/drill/pull/1608#discussion_r253686311
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java
##
@@ -47,6 +47,7 @@
public class DrillSqlWorker {
private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(DrillSqlWorker.class);
private static final ControlsInjector injector =
ControlsInjectorFactory.getInjector(DrillSqlWorker.class);
+ private static final String NEW_LINE = System.getProperty("line.separator");
Review comment:
Done
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit Wrapping should not apply to non-select query
kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit
Wrapping should not apply to non-select query
URL: https://github.com/apache/drill/pull/1608#discussion_r253686299
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryResources.java
##
@@ -83,10 +83,12 @@ public QueryResult submitQueryJSON(QueryWrapper query)
throws Exception {
@Consumes(MediaType.APPLICATION_FORM_URLENCODED)
@Produces(MediaType.TEXT_HTML)
public Viewable submitQuery(@FormParam("query") String query,
- @FormParam("queryType") String queryType) throws
Exception {
+ @FormParam("queryType") String queryType,
+ @FormParam("autoLimit") String autoLimit
+ ) throws Exception {
try {
final String trimmedQueryString =
CharMatcher.is(';').trimTrailingFrom(query.trim());
- final QueryResult result = submitQueryJSON(new
QueryWrapper(trimmedQueryString, queryType));
+ final QueryResult result = submitQueryJSON(new
QueryWrapper(trimmedQueryString, queryType, autoLimit)); //QueryWrapper will
validate autoLimit
Review comment:
Ok
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit Wrapping should not apply to non-select query
kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit
Wrapping should not apply to non-select query
URL: https://github.com/apache/drill/pull/1608#discussion_r253686115
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java
##
@@ -183,4 +184,23 @@ private static PhysicalPlan getQueryPlan(QueryContext
context, String sql, Point
return handler.getPlan(sqlNode);
}
+
+ //Wrap with Auto Limit
+ private static SqlNode wrapWithAutoLimit(SqlConverter parser, QueryContext
context, String sql) {
+SqlNode sqlNode = parser.parse(sql);
+if (context.isAutoLimitEnabled() &&
sqlNode.getKind().belongsTo(SqlKind.QUERY)) {
+ int autoLimitRowCount = context.getAutoLimitRowCount();
Review comment:
+1
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit Wrapping should not apply to non-select query
kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit
Wrapping should not apply to non-select query
URL: https://github.com/apache/drill/pull/1608#discussion_r253685826
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java
##
@@ -273,6 +279,29 @@ public RemoteFunctionRegistry getRemoteFunctionRegistry()
{
return drillbitContext.getRemoteFunctionRegistry();
}
+ /**
+ * Check if auto-limiting of resultset is enabled
+ * @return
Review comment:
Done
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit Wrapping should not apply to non-select query
kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit
Wrapping should not apply to non-select query
URL: https://github.com/apache/drill/pull/1608#discussion_r253685784
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java
##
@@ -183,4 +184,23 @@ private static PhysicalPlan getQueryPlan(QueryContext
context, String sql, Point
return handler.getPlan(sqlNode);
}
+
+ //Wrap with Auto Limit
+ private static SqlNode wrapWithAutoLimit(SqlConverter parser, QueryContext
context, String sql) {
+SqlNode sqlNode = parser.parse(sql);
+if (context.isAutoLimitEnabled() &&
sqlNode.getKind().belongsTo(SqlKind.QUERY)) {
Review comment:
Done
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit Wrapping should not apply to non-select query
kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit
Wrapping should not apply to non-select query
URL: https://github.com/apache/drill/pull/1608#discussion_r253685842
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java
##
@@ -273,6 +279,29 @@ public RemoteFunctionRegistry getRemoteFunctionRegistry()
{
return drillbitContext.getRemoteFunctionRegistry();
}
+ /**
+ * Check if auto-limiting of resultset is enabled
+ * @return
+ */
+ public boolean isAutoLimitEnabled() {
+return autoLimitRowCount != null;
+ }
+
+ /**
+ * Returns the maximum size of auto-limited resultset
+ * @return
Review comment:
Done.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit Wrapping should not apply to non-select query
kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit Wrapping should not apply to non-select query URL: https://github.com/apache/drill/pull/1608#discussion_r253163039 ## File path: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java ## @@ -183,4 +183,24 @@ private static PhysicalPlan getQueryPlan(QueryContext context, String sql, Point return handler.getPlan(sqlNode); } + Review comment: I am not familiar with how Calcite parsing works, but I would have that that wrapping the entire query, including `WITH` select statements, should work too. I'll test this and incorporate if nothing breaks. Thanks. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit Wrapping should not apply to non-select query
kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit
Wrapping should not apply to non-select query
URL: https://github.com/apache/drill/pull/1608#discussion_r253162524
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryWrapper.java
##
@@ -69,14 +75,24 @@ public QueryType getType() {
return QueryType.valueOf(queryType);
}
+ public Integer getAutoLimitRowCount() {
+return autoLimitRowCount;
+ }
+
+ public boolean isAutoLimitEnabled() {
Review comment:
Used locally, but only in one place. Will in-line the function instead.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit Wrapping should not apply to non-select query
kkhatua commented on a change in pull request #1608: DRILL-6960: Auto Limit
Wrapping should not apply to non-select query
URL: https://github.com/apache/drill/pull/1608#discussion_r252798223
##
File path: exec/java-exec/src/main/resources/rest/static/js/querySubmission.js
##
@@ -53,7 +53,18 @@ function doSubmitQueryWithUserName() {
//Wrap & Submit Query (invoked directly if impersonation is not enabled)
function wrapAndSubmitQuery() {
//Wrap if required
-wrapQuery();
+var mustWrapWithLimit = $('input[name="forceLimit"]:checked').length > 0;
+//Clear field when submitting if not mustWrapWithLimit
+if (!mustWrapWithLimit) {
+ //Wipe out any numeric entry in the field before
+ $('#autoLimit').attr('value', '');
+} else {
+ let autoLimitValue=document.getElementById("autoLimit").value;
+ if (isNaN(autoLimitValue)) {
+alert(autoLimitValue+ " is not a number. Please fill in a valid
number");
Review comment:
We're already using the alert mechanism for empty username (when
impersonation is enabled). In both cases, we want to indicate that the user
cannot progress because of a missing username or invalid number. An error
message on the query page might get missed.
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:
us...@infra.apache.org
With regards,
Apache Git Services
