[ 
https://issues.apache.org/jira/browse/DRILL-7583?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17037043#comment-17037043
 ] 

ASF GitHub Bot commented on DRILL-7583:
---------------------------------------

ihuzenko commented on pull request #1981: DRILL-7583: Remove STOP status from 
operator outcome
URL: https://github.com/apache/drill/pull/1981#discussion_r379412442
 
 

 ##########
 File path: 
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/aggregate/HashAggBatch.java
 ##########
 @@ -297,62 +294,64 @@ public IterOutcome innerNext() {
       }
     }
 
-    if (wasKilled) { // if kill() was called before, then finish up
+    if (wasKilled) { // if cancel() was called before, then finish up
       aggregator.cleanup();
-      incoming.kill(false);
       return IterOutcome.NONE;
     }
 
     // Read and aggregate records
-    // ( may need to run again if the spilled partition that was read
-    //   generated new partitions that were all spilled )
+    // (may need to run again if the spilled partition that was read
+    //  generated new partitions that were all spilled)
     AggOutcome out;
     do {
-      //
       //  Read incoming batches and process their records
-      //
       out = aggregator.doWork();
     } while (out == AggOutcome.CALL_WORK_AGAIN);
 
     switch (out) {
-    case CLEANUP_AND_RETURN:
-      container.zeroVectors();
-      aggregator.cleanup();
-      state = BatchState.DONE;
-      // fall through
-    case RETURN_OUTCOME:
-      // rebuilds the schema in the case of complex writer expressions,
-      // since vectors would be added to batch run-time
-      IterOutcome outcome = aggregator.getOutcome();
-      switch (outcome) {
-        case OK:
-        case OK_NEW_SCHEMA:
-          if (firstBatch) {
-            if (CollectionUtils.isNotEmpty(complexWriters)) {
-              container.buildSchema(SelectionVectorMode.NONE);
-              outcome = IterOutcome.OK_NEW_SCHEMA;
+      case CLEANUP_AND_RETURN:
+        container.zeroVectors();
+        aggregator.cleanup();
+        state = BatchState.DONE;
+        // fall through
+      case RETURN_OUTCOME:
+        // rebuilds the schema in the case of complex writer expressions,
+        // since vectors would be added to batch run-time
+        IterOutcome outcome = aggregator.getOutcome();
+        switch (outcome) {
+          case OK:
+          case OK_NEW_SCHEMA:
+            if (firstBatch) {
+              if (CollectionUtils.isNotEmpty(complexWriters)) {
+                container.buildSchema(SelectionVectorMode.NONE);
+                // You'd be forgiven for thinking we should always return
+                // OK_NEW_SCHEMA for the first batch. It turns out, when
+                // two hash aggs are stacked, we get an error if the
+                // upstream one returns OK_NEW_SCHEMA first. Not sure the
+                // details, only know several tests fail.
+                outcome = IterOutcome.OK_NEW_SCHEMA;
+              }
+              firstBatch = false;
             }
-            firstBatch = false;
-          }
-          // fall thru
-        default:
-          return outcome;
-      }
-
-    case UPDATE_AGGREGATOR:
-      throw UserException.unsupportedError()
-          .message(SchemaChangeException.schemaChanged(
-              "Hash aggregate does not support schema change",
-              incomingSchema,
-              incoming.getSchema()).getMessage())
-          .build(logger);
-    default:
-      throw new IllegalStateException(String.format("Unknown state %s.", out));
+            break;
+          default:
+        }
+        return outcome;
+
+      case UPDATE_AGGREGATOR:
+        throw UserException.unsupportedError()
+            .message(SchemaChangeException.schemaChanged(
+                "Hash aggregate does not support schema change",
+                incomingSchema,
+                incoming.getSchema()).getMessage())
+            .build(logger);
+      default:
+        throw new IllegalStateException(String.format("Unknown state %s.", 
out));
     }
   }
 
   /**
-   * Creates a new Aggregator based on the current schema. If setup fails, this
+   * Creates a new aggregator based on the current schema. If setup fails, this
 
 Review comment:
   ```suggestion
      * Creates a new {@link #aggregator} based on the current schema. If setup 
fails, this
   ```
 
----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


> Remove STOP status in favor of fail-fast
> ----------------------------------------
>
>                 Key: DRILL-7583
>                 URL: https://issues.apache.org/jira/browse/DRILL-7583
>             Project: Apache Drill
>          Issue Type: Improvement
>    Affects Versions: 1.17.0
>            Reporter: Paul Rogers
>            Assignee: Paul Rogers
>            Priority: Major
>             Fix For: 1.18.0
>
>
> The original error solution was a complex process of a) setting a failed 
> flag, b) telling all upstream operators they have failed, c) returning a 
> {{STOP}} status.  Drill has long supported a "fail-fast" error path based on 
> throwing an exception; relying on the fragment executor to clean up the 
> operator stack. Recent revisions have converted most operators to use the 
> simpler fail-fast strategy based on throwing an exception instead of using 
> the older {{STOP}} approach. This change simply removes the old, now-unused 
> {{STOP}} based path.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

Reply via email to