[GitHub] [drill] ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for
operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r378086624
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/BaseRootExec.java
##
@@ -184,4 +179,38 @@ public void close() throws Exception {
}
}
}
+
+ /**
+ * Given a list of operators and a stack trace, walks the stack trace and
+ * the operator list to find the leaf-most operator, which is the one
+ * that was active when the exception was thrown. Handle the cases in
+ * which no operator was active, each operator had multiple methods on
+ * the stack, or the exception was thrown in some class called by
+ * the operator.
+ *
+ * Not all operators leave a mark in the trace. In particular if a the
+ * call stack is only through base-class methods, then we have no way to
+ * know the actual class during the call. This is OK because the leaf
+ * methods are just pass-through operations, they are unlikely to fail.
+ *
+ * @param the type of the operator. Parameterized to allow easier
+ * testing
+ * @param dag the list of operators from root-most to leaf-most
+ * @param e the exception thrown somewhere in the operator tree
+ * @return the leaf-most operator, if any
+ */
+ public static T findLeaf(List dag, Throwable e) {
+StackTraceElement[] trace = e.getStackTrace();
Review comment:
I see the order is important for the outer loop
```java
for (int i = dag.size() - 1; i >= 0; i--) {
...
}
```
but in an inner loop, you're iterating for each stack trace element so the
suggested ```Set``` won't break your algorithm. Though I don't force
the change since we know that operators and stack trace collections are small.
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for
operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376942618
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/WriterRecordBatch.java
##
@@ -152,11 +154,18 @@ private void addOutputContainerData() {
container.setRecordCount(1);
}
- protected void setupNewSchema() throws IOException {
+ protected void setupNewSchema() {
try {
// update the schema in RecordWriter
stats.startSetup();
- recordWriter.updateSchema(incoming);
+ try {
+recordWriter.updateSchema(incoming);
+ } catch (IOException e) {
+// TODO: This is better handled inside updateSchema()
Review comment:
same question as above
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors URL: https://github.com/apache/drill/pull/1975#discussion_r376999100 ## File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/OrderedPartitionRecordBatch.java ## @@ -87,6 +87,8 @@ * value is determined by where each record falls in the partition table. This * column is used by PartitionSenderRootExec to determine which bucket to assign * each record to. + * + * This code is not used. Review comment: if the class is never used why don't just delete it? 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 With regards, Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for
operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r377003326
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/FrameSupportTemplate.java
##
@@ -175,7 +177,7 @@ private int processROWS(int row) throws DrillException {
return row;
}
- private int processRANGE(int row) throws DrillException {
+ private int processRANGE(int row) throws SchemaChangeException {
Review comment:
```suggestion
private int processRange(int row) throws SchemaChangeException {
```
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for
operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376908070
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/RootExec.java
##
@@ -20,34 +20,38 @@
import org.apache.drill.exec.proto.ExecProtos.FragmentHandle;
/**
- * Functionality
- *
- * A FragmentRoot is a node which is the last processing node in a query
plan. FragmentTerminals include Exchange
- * output nodes and storage nodes. They are there driving force behind the
completion of a query.
- *
- * Assumptions
- *
- * All implementations of {@link RootExec} assume that all their methods are
called by the same thread.
+ * Node which is the last processing node in a query plan. FragmentTerminals
+ * include Exchange output nodes and storage nodes. They are there driving
force
+ * behind the completion of a query.
*
+ * Assumes that all implementations of {@link RootExec} assume that all their
+ * methods are called by the same thread.
*/
public interface RootExec extends AutoCloseable {
+
/**
* Do the next batch of work.
- * @return Whether or not additional batches of work are necessary. False
means that this fragment is done.
+ *
+ * @return Whether or not additional batches of work are necessary. False
+ * means that this fragment is done.
*/
boolean next();
/**
- * Inform sender that receiving fragment is finished and doesn't need any
more data. This can be called multiple
- * times (once for each downstream receiver). If all receivers are finished
then a subsequent call to {@link #next()}
- * will return false.
- * @param handle The handle pointing to the downstream receiver that does
not need anymore data.
+ * Inform sender that receiving fragment is finished and doesn't need any
more
+ * data. This can be called multiple times (once for each downstream
+ * receiver). If all receivers are finished then a subsequent call to
+ * {@link #next()} will return false.
+ *
+ * @param handle
+ * The handle pointing to the downstream receiver that does not need
+ * anymore data.
*/
void receivingFragmentFinished(FragmentHandle handle);
/**
- * Dump failed batches' state preceded by its parent's state to logs.
Invoked when there is a
- * failure during fragment execution.
+ * Dump failed batches' state preceded by its parent's state to logs. Invoked
+ * when there is a failure during fragment execution.
Review comment:
Please add ```@param``` section to the JavaDoc describing where the trowable
came from.
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for
operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376903426
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/BaseRootExec.java
##
@@ -184,4 +179,38 @@ public void close() throws Exception {
}
}
}
+
+ /**
+ * Given a list of operators and a stack trace, walks the stack trace and
+ * the operator list to find the leaf-most operator, which is the one
+ * that was active when the exception was thrown. Handle the cases in
+ * which no operator was active, each operator had multiple methods on
+ * the stack, or the exception was thrown in some class called by
+ * the operator.
+ *
+ * Not all operators leave a mark in the trace. In particular if a the
+ * call stack is only through base-class methods, then we have no way to
+ * know the actual class during the call. This is OK because the leaf
+ * methods are just pass-through operations, they are unlikely to fail.
+ *
+ * @param the type of the operator. Parameterized to allow easier
+ * testing
+ * @param dag the list of operators from root-most to leaf-most
+ * @param e the exception thrown somewhere in the operator tree
+ * @return the leaf-most operator, if any
+ */
+ public static T findLeaf(List dag, Throwable e) {
+StackTraceElement[] trace = e.getStackTrace();
Review comment:
Minor comment: here we could collect a ```Set``` of class name strings
ignoring those names which don't start with ```org.apache.drill``` package
name. Like:
```java
Set drillStackTrace = Stream.of(trace)
.map(StackTraceElement::getClassName)
.filter(cn -> cn.startsWith(DRILL_PACKAGE_NAME))
.collect(Collectors.toSet());
```
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for
operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376919500
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java
##
@@ -83,47 +83,47 @@ public boolean innerNext() {
IterOutcome outcome = next(incoming);
logger.trace("Screen Outcome {}", outcome);
switch (outcome) {
- case STOP:
-return false;
- case NONE:
-if (firstBatch) {
- // this is the only data message sent to the client and may contain
the schema
- QueryWritableBatch batch;
- QueryData header = QueryData.newBuilder()
-.setQueryId(context.getHandle().getQueryId())
-.setRowCount(0)
-.setDef(RecordBatchDef.getDefaultInstance())
-.build();
- batch = new QueryWritableBatch(header);
+case STOP:
+ return false;
+case NONE:
+ if (firstBatch) {
+// this is the only data message sent to the client and may
contain the schema
+QueryWritableBatch batch;
+QueryData header = QueryData.newBuilder()
+ .setQueryId(context.getHandle().getQueryId())
+ .setRowCount(0)
+ .setDef(RecordBatchDef.getDefaultInstance())
+ .build();
+batch = new QueryWritableBatch(header);
+
+stats.startWait();
+try {
+ userConnection.sendData(batch);
+} finally {
+ stats.stopWait();
+}
+firstBatch = false; // we don't really need to set this. But who
knows!
+ }
+ return false;
+case OK_NEW_SCHEMA:
+ materializer = new VectorRecordMaterializer(context, oContext,
incoming);
+ //$FALL-THROUGH$
+case OK:
+ injector.injectPause(context.getExecutionControls(), "sending-data",
logger);
+ final QueryWritableBatch batch = materializer.convertNext();
+ updateStats(batch);
stats.startWait();
try {
userConnection.sendData(batch);
} finally {
stats.stopWait();
}
- firstBatch = false; // we don't really need to set this. But who
knows!
-}
-
-return false;
- case OK_NEW_SCHEMA:
-materializer = new VectorRecordMaterializer(context, oContext,
incoming);
-//$FALL-THROUGH$
- case OK:
-injector.injectPause(context.getExecutionControls(), "sending-data",
logger);
-final QueryWritableBatch batch = materializer.convertNext();
-updateStats(batch);
-stats.startWait();
-try {
- userConnection.sendData(batch);
-} finally {
- stats.stopWait();
-}
-firstBatch = false;
-
-return true;
- default:
-throw new UnsupportedOperationException();
+ firstBatch = false;
+
+ return true;
+default:
+ throw new UnsupportedOperationException();
Review comment:
Here would be nice to add message containing ```outcome``` which caused the
exception.
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for
operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376951117
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/filter/RuntimeFilterBatchCreator.java
##
@@ -29,7 +29,9 @@
public class RuntimeFilterBatchCreator implements
BatchCreator{
Review comment:
```suggestion
public class RuntimeFilterBatchCreator implements
BatchCreator {
```
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for
operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376947666
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/aggregate/SpilledRecordBatch.java
##
@@ -163,11 +162,12 @@ public IterOutcome next() {
container = vas.get();
}
} catch (IOException e) {
- lastOutcome = IterOutcome.STOP;
- throw UserException.dataReadError(e).addContext("Failed reading from a
spill file").build(logger);
+ throw UserException.dataReadError(e)
+ .addContext("Failed reading from a spill file")
+ .build(logger);
} catch (Exception e) {
- lastOutcome = IterOutcome.STOP;
- throw e;
+ // TODO: Catch the error closer to the cause and create a better error
message.
Review comment:
can we avoid adding new todos?
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for
operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376951049
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/filter/RuntimeFilterBatchCreator.java
##
@@ -29,7 +29,9 @@
public class RuntimeFilterBatchCreator implements
BatchCreator{
@Override
- public CloseableRecordBatch getBatch(ExecutorFragmentContext context,
RuntimeFilterPOP config, List children) throws
ExecutionSetupException {
+ public CloseableRecordBatch getBatch(ExecutorFragmentContext context,
+ RuntimeFilterPOP config, List children)
+ throws ExecutionSetupException {
Review comment:
```java
public CloseableRecordBatch getBatch(ExecutorFragmentContext context,
RuntimeFilterPOP config,
List children) throws
ExecutionSetupException {
```
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for
operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376945843
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/aggregate/HashAggBatch.java
##
@@ -340,16 +340,12 @@ public IterOutcome innerNext() {
}
case UPDATE_AGGREGATOR:
- context.getExecutorState().fail(UserException.unsupportedError()
+ throw UserException.unsupportedError()
.message(SchemaChangeException.schemaChanged(
"Hash aggregate does not support schema change",
incomingSchema,
incoming.getSchema()).getMessage())
- .build(logger));
- close();
- killIncoming(false);
- firstBatch = false;
- return IterOutcome.STOP;
+ .build(logger);
Review comment:
could you please explain why ```close()``` and ```killIncoming(false)```
invocations were removed for the case?
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for
operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376923043
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/StatisticsWriterRecordBatch.java
##
@@ -91,41 +93,47 @@ public IterOutcome innerNext() {
}
// process the complete upstream in one next() call
IterOutcome upstream;
-try {
- do {
-upstream = next(incoming);
-
-switch(upstream) {
- case STOP:
-return upstream;
-
- case NOT_YET:
- case NONE:
-break;
-
- case OK_NEW_SCHEMA:
-setupNewSchema();
-// $FALL-THROUGH$
- case OK:
+do {
+ upstream = next(incoming);
+
+ switch(upstream) {
+case STOP:
+ return upstream;
+
+case NOT_YET:
+case NONE:
+ break;
+
+case OK_NEW_SCHEMA:
+ setupNewSchema();
+ // $FALL-THROUGH$
+case OK:
+ try {
counter +=
statsRecordWriterImpl.writeStatistics(incoming.getRecordCount());
-logger.debug("Total records written so far: {}", counter);
-
-for(final VectorWrapper v : incoming) {
- v.getValueVector().clear();
-}
-break;
-
- default:
-throw new UnsupportedOperationException();
-}
- } while(upstream != IterOutcome.NONE);
- // Flush blocking writers now
+ } catch (IOException e) {
+// TODO: Better handled inside the write() method.
Review comment:
such todos usually live forever in the code, could you please improve the
```StatisticsRecordWriterImpl.writeStatistics``` in this PR?
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for
operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r377003222
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/FrameSupportTemplate.java
##
@@ -156,7 +158,7 @@ private int processPartition(final int currentRow) throws
DrillException {
}
}
- private int processROWS(int row) throws DrillException {
+ private int processROWS(int row) throws SchemaChangeException {
Review comment:
```suggestion
private int processRows(int row) throws SchemaChangeException {
```
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for
operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376926556
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/TopN/PriorityQueueTemplate.java
##
@@ -142,11 +143,7 @@ public void add(RecordBatchData batch) throws
SchemaChangeException{
public void generate() {
Stopwatch watch = Stopwatch.createStarted();
final DrillBuf drillBuf = allocator.buffer(4 * queueSize);
-try {
- finalSv4 = new SelectionVector4(drillBuf, queueSize, 4000);
-} catch (SchemaChangeException e) {
- throw AbstractRecordBatch.schemaChangeException(e, "Priority Queue",
logger);
-}
+finalSv4 = new SelectionVector4(drillBuf, queueSize, 4000);
Review comment:
please convert magic number to constant.
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for
operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376942403
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/WriterRecordBatch.java
##
@@ -77,54 +80,53 @@ public BatchSchema getSchema() {
@Override
public IterOutcome innerNext() {
if (processed) {
-// cleanup();
// if the upstream record batch is already processed and next() is
called by
// downstream then return NONE to indicate completion
return IterOutcome.NONE;
}
// process the complete upstream in one next() call
IterOutcome upstream;
-try {
- do {
-upstream = next(incoming);
-
-switch(upstream) {
- case STOP:
-return upstream;
-
- case NOT_YET:
+do {
+ upstream = next(incoming);
+
+ switch(upstream) {
+case STOP:
+ return upstream;
+
+case NOT_YET:
+ break;
+case NONE:
+ if (schema != null) {
+// Schema is for the output batch schema which is setup in
setupNewSchema(). Since the output
+// schema is fixed ((Fragment(VARCHAR), Number of records written
(BIGINT)) we should set it
+// up even with 0 records for it to be reported back to the client.
break;
- case NONE:
-if (schema != null) {
- // Schema is for the output batch schema which is setup in
setupNewSchema(). Since the output
- // schema is fixed ((Fragment(VARCHAR), Number of records
written (BIGINT)) we should set it
- // up even with 0 records for it to be reported back to the
client.
- break;
-}
-
- case OK_NEW_SCHEMA:
-setupNewSchema();
-// $FALL-THROUGH$
- case OK:
-counter += eventBasedRecordWriter.write(incoming.getRecordCount());
-logger.debug("Total records written so far: {}", counter);
+ }
-for(final VectorWrapper v : incoming) {
- v.getValueVector().clear();
-}
-break;
-
- default:
-throw new UnsupportedOperationException();
-}
- } while(upstream != IterOutcome.NONE);
-} catch(IOException ex) {
- logger.error("Failure during query", ex);
- kill(false);
- context.getExecutorState().fail(ex);
- return IterOutcome.STOP;
-}
+case OK_NEW_SCHEMA:
+ setupNewSchema();
+ // $FALL-THROUGH$
+case OK:
+ try {
+counter += eventBasedRecordWriter.write(incoming.getRecordCount());
+ } catch (IOException e) {
+// TODO: Better handled inside the write() method.
Review comment:
can we avoid leaving new todos in the code?
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for
operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376890243
##
File path:
common/src/main/java/org/apache/drill/common/exceptions/DrillRuntimeException.java
##
@@ -17,8 +17,11 @@
*/
package org.apache.drill.common.exceptions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
public class DrillRuntimeException extends RuntimeException {
- private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(DrillRuntimeException.class);
+ private static final Logger logger =
LoggerFactory.getLogger(DrillRuntimeException.class);
Review comment:
Seems like the logger is unused and may be safely removed.
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for
operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376924072
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/StatisticsWriterRecordBatch.java
##
@@ -154,11 +162,18 @@ private void addOutputContainerData() {
container.setRecordCount(1);
}
- protected void setupNewSchema() throws IOException {
+ protected void setupNewSchema() {
try {
// update the schema in RecordWriter
stats.startSetup();
- recordWriter.updateSchema(incoming);
+ try {
+recordWriter.updateSchema(incoming);
+ } catch (IOException e) {
+// TODO: This is better handled inside updateSchema()
Review comment:
I would like to ask for fixing it, thank you in advance.
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for
operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376954034
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/filter/RuntimeFilterRecordBatch.java
##
@@ -196,20 +194,24 @@ private void setupHashHelper() {
ValueVectorReadExpression toHashFieldExp = new
ValueVectorReadExpression(typedFieldId);
hashFieldExps.add(toHashFieldExp);
}
-hash64 = hashHelper.getHash64(hashFieldExps.toArray(new
LogicalExpression[hashFieldExps.size()]), typedFieldIds.toArray(new
TypedFieldId[typedFieldIds.size()]));
+hash64 = hashHelper.getHash64(hashFieldExps.toArray(
+new LogicalExpression[hashFieldExps.size()]),
+typedFieldIds.toArray(new TypedFieldId[typedFieldIds.size()]));
} catch (Exception e) {
throw UserException.internalError(e).build(logger);
}
}
}
/**
- * If RuntimeFilter is available then applies the filter condition on the
incoming batch records and creates an SV2
- * to store indexes which passes the filter condition. In case when
RuntimeFilter is not available it just pass
+ * If RuntimeFilter is available then applies the filter condition on the
+ * incoming batch records and creates an SV2 to store indexes which passes
the
+ * filter condition. In case when RuntimeFilter is not available it just pass
* through all the records from incoming batch to downstream.
+ *
* @throws SchemaChangeException
Review comment:
```suggestion
```
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for
operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376912149
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScanBatch.java
##
@@ -212,13 +213,24 @@ private boolean shouldContinueAfterNoRecords() throws
Exception {
}
return true;
} else { // Regular scan
- currentReader.close();
- currentReader = null;
+ closeCurrentReader();
return true; // In regular case, we always continue the iteration, if no
more reader, we will break out at the head of loop
}
}
- private IterOutcome internalNext() throws Exception {
+ private void closeCurrentReader() {
+try {
+ if (currentReader != null) {
Review comment:
I would suggest putting whole ```try-catch-finally``` inside the if, or
using our ```Autocloseables``` like:
```java
AutoCloseables.closeSilently(currentReader);
currentReader = null;
```
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
With regards,
Apache Git Services
