[GitHub] vdiravka commented on a change in pull request #1455: DRILL-6724: Convert IndexOutOfBounds exception to UserException with …
vdiravka commented on a change in pull request #1455: DRILL-6724: Convert
IndexOutOfBounds exception to UserException with …
URL: https://github.com/apache/drill/pull/1455#discussion_r214899539
##
File path:
contrib/storage-kafka/src/main/java/org/apache/drill/exec/store/kafka/KafkaRecordReader.java
##
@@ -116,6 +118,7 @@ public int next() {
if (++messageCount >= DEFAULT_MESSAGES_PER_BATCH) {
break;
}
+currentMessageCount = messageCount;
Review comment:
If `currentMessageCount` field is introduced, do we still need a local
variable?
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] vdiravka commented on a change in pull request #1455: DRILL-6724: Convert IndexOutOfBounds exception to UserException with …
vdiravka commented on a change in pull request #1455: DRILL-6724: Convert
IndexOutOfBounds exception to UserException with …
URL: https://github.com/apache/drill/pull/1455#discussion_r214904295
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JSONRecordReader.java
##
@@ -205,36 +191,32 @@ public int next() {
writer.reset();
recordCount = 0;
parseErrorCount = 0;
-if(write == ReadState.JSON_RECORD_PARSE_EOF_ERROR){
+if (write == ReadState.JSON_RECORD_PARSE_EOF_ERROR) {
return recordCount;
}
-outside: while(recordCount < DEFAULT_ROWS_PER_BATCH){
- try{
+while (recordCount < DEFAULT_ROWS_PER_BATCH) {
+ try {
writer.setPosition(recordCount);
write = jsonReader.write(writer);
-if(write == ReadState.WRITE_SUCCEED){
+if (write == ReadState.WRITE_SUCCEED) {
recordCount++;
-}
-else if(write == ReadState.JSON_RECORD_PARSE_ERROR || write ==
ReadState.JSON_RECORD_PARSE_EOF_ERROR){
- if(skipMalformedJSONRecords == false){
-handleAndRaise("Error parsing JSON", new
Exception(hadoopPath.getName() + " : line nos :" + (recordCount+1)));
+} else if (write == ReadState.JSON_RECORD_PARSE_ERROR || write ==
ReadState.JSON_RECORD_PARSE_EOF_ERROR) {
+ if (!skipMalformedJSONRecords) {
+handleAndRaise("Error parsing JSON", new Exception());
}
++parseErrorCount;
- if(printSkippedMalformedJSONRecordLineNumber){
-logger.debug("Error parsing JSON in " + hadoopPath.getName() + " :
line nos :" + (recordCount+parseErrorCount));
+ if (printSkippedMalformedJSONRecordLineNumber) {
+logger.debug("Error parsing JSON in " + hadoopPath.getName() + " :
line nos :" + (recordCount + parseErrorCount));
Review comment:
spaces near `+` operator
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] vdiravka commented on a change in pull request #1455: DRILL-6724: Convert IndexOutOfBounds exception to UserException with …
vdiravka commented on a change in pull request #1455: DRILL-6724: Convert
IndexOutOfBounds exception to UserException with …
URL: https://github.com/apache/drill/pull/1455#discussion_r214906760
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JSONRecordReader.java
##
@@ -162,37 +162,23 @@ public void setup(final OperatorContext context, final
OutputMutator output) thr
}
private void setupParser() throws IOException {
-if(hadoopPath != null){
+if (hadoopPath != null) {
jsonReader.setSource(stream);
-}else{
+} else {
jsonReader.setSource(embeddedContent);
}
jsonReader.setIgnoreJSONParseErrors(skipMalformedJSONRecords);
}
protected void handleAndRaise(String suffix, Exception e) throws
UserException {
-
String message = e.getMessage();
-int columnNr = -1;
if (e instanceof JsonParseException) {
- final JsonParseException ex = (JsonParseException) e;
- message = ex.getOriginalMessage();
- columnNr = ex.getLocation().getColumnNr();
-}
-
-UserException.Builder exceptionBuilder = UserException.dataReadError(e)
-.message("%s - %s", suffix, message);
-if (columnNr > 0) {
- exceptionBuilder.pushContext("Column ", columnNr);
-}
-
-if (hadoopPath != null) {
- exceptionBuilder.pushContext("Record ", currentRecordNumberInFile())
- .pushContext("File ", hadoopPath.toUri().getPath());
+ message = ((JsonParseException) e).getOriginalMessage();
Review comment:
redundant spaces here and above
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] vdiravka commented on a change in pull request #1455: DRILL-6724: Convert IndexOutOfBounds exception to UserException with …
vdiravka commented on a change in pull request #1455: DRILL-6724: Convert
IndexOutOfBounds exception to UserException with …
URL: https://github.com/apache/drill/pull/1455#discussion_r214906574
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JSONRecordReader.java
##
@@ -162,37 +162,23 @@ public void setup(final OperatorContext context, final
OutputMutator output) thr
}
private void setupParser() throws IOException {
-if(hadoopPath != null){
+if (hadoopPath != null) {
jsonReader.setSource(stream);
-}else{
+} else {
Review comment:
spaces
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] vdiravka commented on a change in pull request #1455: DRILL-6724: Convert IndexOutOfBounds exception to UserException with …
vdiravka commented on a change in pull request #1455: DRILL-6724: Convert
IndexOutOfBounds exception to UserException with …
URL: https://github.com/apache/drill/pull/1455#discussion_r214904295
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JSONRecordReader.java
##
@@ -205,36 +191,32 @@ public int next() {
writer.reset();
recordCount = 0;
parseErrorCount = 0;
-if(write == ReadState.JSON_RECORD_PARSE_EOF_ERROR){
+if (write == ReadState.JSON_RECORD_PARSE_EOF_ERROR) {
return recordCount;
}
-outside: while(recordCount < DEFAULT_ROWS_PER_BATCH){
- try{
+while (recordCount < DEFAULT_ROWS_PER_BATCH) {
+ try {
writer.setPosition(recordCount);
write = jsonReader.write(writer);
-if(write == ReadState.WRITE_SUCCEED){
+if (write == ReadState.WRITE_SUCCEED) {
recordCount++;
-}
-else if(write == ReadState.JSON_RECORD_PARSE_ERROR || write ==
ReadState.JSON_RECORD_PARSE_EOF_ERROR){
- if(skipMalformedJSONRecords == false){
-handleAndRaise("Error parsing JSON", new
Exception(hadoopPath.getName() + " : line nos :" + (recordCount+1)));
+} else if (write == ReadState.JSON_RECORD_PARSE_ERROR || write ==
ReadState.JSON_RECORD_PARSE_EOF_ERROR) {
+ if (!skipMalformedJSONRecords) {
+handleAndRaise("Error parsing JSON", new Exception());
}
++parseErrorCount;
- if(printSkippedMalformedJSONRecordLineNumber){
-logger.debug("Error parsing JSON in " + hadoopPath.getName() + " :
line nos :" + (recordCount+parseErrorCount));
+ if (printSkippedMalformedJSONRecordLineNumber) {
+logger.debug("Error parsing JSON in " + hadoopPath.getName() + " :
line nos :" + (recordCount + parseErrorCount));
Review comment:
spaces near `+` operator
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] vdiravka commented on a change in pull request #1455: DRILL-6724: Convert IndexOutOfBounds exception to UserException with …
vdiravka commented on a change in pull request #1455: DRILL-6724: Convert
IndexOutOfBounds exception to UserException with …
URL: https://github.com/apache/drill/pull/1455#discussion_r214878343
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/AbstractRecordReader.java
##
@@ -105,4 +106,22 @@ public void allocate(Map vectorMap)
throws OutOfMemoryExcep
return GroupScan.ALL_COLUMNS;
}
+ @Override
+ public UserException createUserException(int type, Throwable cause, String
message) {
+return null;
+ }
+
+ // Construct builder depending on type
+ protected UserException.Builder getExceptionBuilder(int type, Throwable
cause) {
+if (type < ERROR_TYPE_DATA_READ || type > ERROR_TYPE_INTERNAL) {
Review comment:
If a new ERROR type is introduced between above two ones, it should be
obligatorily reflected here
What about:
```
UserException.Builder builder;
if (type == ERROR_TYPE_DATA_READ) {
builder = UserException.dataReadError(cause);
} else if (type == ERROR_TYPE_INTERNAL) {
builder = UserException.internalError(cause);
} else {
throw new IllegalArgumentException("Unknown error type: " + type);
}
return builder;
```
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] vdiravka commented on a change in pull request #1455: DRILL-6724: Convert IndexOutOfBounds exception to UserException with …
vdiravka commented on a change in pull request #1455: DRILL-6724: Convert
IndexOutOfBounds exception to UserException with …
URL: https://github.com/apache/drill/pull/1455#discussion_r214905003
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JSONRecordReader.java
##
@@ -205,36 +191,32 @@ public int next() {
writer.reset();
recordCount = 0;
parseErrorCount = 0;
-if(write == ReadState.JSON_RECORD_PARSE_EOF_ERROR){
+if (write == ReadState.JSON_RECORD_PARSE_EOF_ERROR) {
return recordCount;
}
-outside: while(recordCount < DEFAULT_ROWS_PER_BATCH){
- try{
+while (recordCount < DEFAULT_ROWS_PER_BATCH) {
+ try {
writer.setPosition(recordCount);
write = jsonReader.write(writer);
-if(write == ReadState.WRITE_SUCCEED){
+if (write == ReadState.WRITE_SUCCEED) {
recordCount++;
-}
-else if(write == ReadState.JSON_RECORD_PARSE_ERROR || write ==
ReadState.JSON_RECORD_PARSE_EOF_ERROR){
- if(skipMalformedJSONRecords == false){
-handleAndRaise("Error parsing JSON", new
Exception(hadoopPath.getName() + " : line nos :" + (recordCount+1)));
+} else if (write == ReadState.JSON_RECORD_PARSE_ERROR || write ==
ReadState.JSON_RECORD_PARSE_EOF_ERROR) {
+ if (!skipMalformedJSONRecords) {
+handleAndRaise("Error parsing JSON", new Exception());
}
++parseErrorCount;
- if(printSkippedMalformedJSONRecordLineNumber){
-logger.debug("Error parsing JSON in " + hadoopPath.getName() + " :
line nos :" + (recordCount+parseErrorCount));
+ if (printSkippedMalformedJSONRecordLineNumber) {
+logger.debug("Error parsing JSON in " + hadoopPath.getName() + " :
line nos :" + (recordCount + parseErrorCount));
}
- if(write == ReadState.JSON_RECORD_PARSE_EOF_ERROR){
-break outside;
+ if (write == ReadState.JSON_RECORD_PARSE_EOF_ERROR) {
+break;
}
+} else {
+ break;
}
-else{
- break outside;
-}
+ } catch(IOException ex) {
Review comment:
space after `catch`
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] vdiravka commented on a change in pull request #1455: DRILL-6724: Convert IndexOutOfBounds exception to UserException with …
vdiravka commented on a change in pull request #1455: DRILL-6724: Convert
IndexOutOfBounds exception to UserException with …
URL: https://github.com/apache/drill/pull/1455#discussion_r214906574
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JSONRecordReader.java
##
@@ -162,37 +162,23 @@ public void setup(final OperatorContext context, final
OutputMutator output) thr
}
private void setupParser() throws IOException {
-if(hadoopPath != null){
+if (hadoopPath != null) {
jsonReader.setSource(stream);
-}else{
+} else {
Review comment:
spaces
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] vdiravka commented on a change in pull request #1455: DRILL-6724: Convert IndexOutOfBounds exception to UserException with …
vdiravka commented on a change in pull request #1455: DRILL-6724: Convert
IndexOutOfBounds exception to UserException with …
URL: https://github.com/apache/drill/pull/1455#discussion_r214899539
##
File path:
contrib/storage-kafka/src/main/java/org/apache/drill/exec/store/kafka/KafkaRecordReader.java
##
@@ -116,6 +118,7 @@ public int next() {
if (++messageCount >= DEFAULT_MESSAGES_PER_BATCH) {
break;
}
+currentMessageCount = messageCount;
Review comment:
If `currentMessageCount` field is introduced, do we still need local
variable?
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] vdiravka commented on a change in pull request #1455: DRILL-6724: Convert IndexOutOfBounds exception to UserException with …
vdiravka commented on a change in pull request #1455: DRILL-6724: Convert
IndexOutOfBounds exception to UserException with …
URL: https://github.com/apache/drill/pull/1455#discussion_r214906760
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JSONRecordReader.java
##
@@ -162,37 +162,23 @@ public void setup(final OperatorContext context, final
OutputMutator output) thr
}
private void setupParser() throws IOException {
-if(hadoopPath != null){
+if (hadoopPath != null) {
jsonReader.setSource(stream);
-}else{
+} else {
jsonReader.setSource(embeddedContent);
}
jsonReader.setIgnoreJSONParseErrors(skipMalformedJSONRecords);
}
protected void handleAndRaise(String suffix, Exception e) throws
UserException {
-
String message = e.getMessage();
-int columnNr = -1;
if (e instanceof JsonParseException) {
- final JsonParseException ex = (JsonParseException) e;
- message = ex.getOriginalMessage();
- columnNr = ex.getLocation().getColumnNr();
-}
-
-UserException.Builder exceptionBuilder = UserException.dataReadError(e)
-.message("%s - %s", suffix, message);
-if (columnNr > 0) {
- exceptionBuilder.pushContext("Column ", columnNr);
-}
-
-if (hadoopPath != null) {
- exceptionBuilder.pushContext("Record ", currentRecordNumberInFile())
- .pushContext("File ", hadoopPath.toUri().getPath());
+ message = ((JsonParseException) e).getOriginalMessage();
Review comment:
redundant spaces here and above
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] vdiravka commented on a change in pull request #1455: DRILL-6724: Convert IndexOutOfBounds exception to UserException with …
vdiravka commented on a change in pull request #1455: DRILL-6724: Convert
IndexOutOfBounds exception to UserException with …
URL: https://github.com/apache/drill/pull/1455#discussion_r214674475
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/bson/BsonRecordReader.java
##
@@ -364,13 +367,14 @@ public void ensureAtLeastOneField(ComplexWriter writer) {
}
}
- public UserException.Builder getExceptionWithContext(UserException.Builder
exceptionBuilder, String field,
- String msg, Object... args) {
-return null;
- }
-
- public UserException.Builder getExceptionWithContext(Throwable exception,
String field, String msg, Object... args) {
-return null;
+ public UserException.Builder createExceptionWithContext(Throwable cause,
String message) {
+UserException.Builder builder = UserException.dataReadError(cause)
+.message(message);
+if (reader != null) {
+ builder.addContext("Name", reader.getCurrentName())
Review comment:
`addContext` from new line will look better
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] vdiravka commented on a change in pull request #1455: DRILL-6724: Convert IndexOutOfBounds exception to UserException with …
vdiravka commented on a change in pull request #1455: DRILL-6724: Convert
IndexOutOfBounds exception to UserException with …
URL: https://github.com/apache/drill/pull/1455#discussion_r214548619
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/log/LogRecordReader.java
##
@@ -761,4 +761,13 @@ public void close() {
reader = null;
}
}
+
+ @Override
+ public UserException createDataReadException(Throwable cause, String
message) {
+return UserException.dataReadError(cause)
+.message(message)
+.addContext("File", fileWork.getPath())
+.addContext("Line", rowIndex)
+.build(logger);
+ }
}
Review comment:
nl
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] vdiravka commented on a change in pull request #1455: DRILL-6724: Convert IndexOutOfBounds exception to UserException with …
vdiravka commented on a change in pull request #1455: DRILL-6724: Convert
IndexOutOfBounds exception to UserException with …
URL: https://github.com/apache/drill/pull/1455#discussion_r214548619
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/log/LogRecordReader.java
##
@@ -761,4 +761,13 @@ public void close() {
reader = null;
}
}
+
+ @Override
+ public UserException createDataReadException(Throwable cause, String
message) {
+return UserException.dataReadError(cause)
+.message(message)
+.addContext("File", fileWork.getPath())
+.addContext("Line", rowIndex)
+.build(logger);
+ }
}
Review comment:
nl
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] vdiravka commented on a change in pull request #1455: DRILL-6724: Convert IndexOutOfBounds exception to UserException with …
vdiravka commented on a change in pull request #1455: DRILL-6724: Convert
IndexOutOfBounds exception to UserException with …
URL: https://github.com/apache/drill/pull/1455#discussion_r214548571
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/RecordReader.java
##
@@ -49,4 +50,15 @@
* @return The number of additional records added to the output.
*/
int next();
+
+ /**
+ * Creates {@link UserException} with reader context. Should be analogous to
+ * {@link UserException#dataReadError(Throwable)} with provided context data.
+ * Returns {@code null} if context for a reader is unavailable.
Review comment:
Use `` or `` for the new line
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] vdiravka commented on a change in pull request #1455: DRILL-6724: Convert IndexOutOfBounds exception to UserException with …
vdiravka commented on a change in pull request #1455: DRILL-6724: Convert
IndexOutOfBounds exception to UserException with …
URL: https://github.com/apache/drill/pull/1455#discussion_r214548608
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/sequencefile/SequenceFileRecordReader.java
##
@@ -163,4 +163,23 @@ public void close() {
logger.warn("Exception closing reader: {}", e);
}
}
+
+ @Override
+ public UserException createDataReadException(Throwable cause, String
message) {
+UserException.Builder builder = UserException.dataReadError(cause)
+.message(message)
+.addContext("File ", split.getPath().toString());
+long position = -1L;
+try {
+ if (reader != null) {
+position = reader.getPos();
+ }
+} catch (IOException e) {
+ // Ignore as this is to provide information only
+}
+if (position > 0) {
+ builder.addContext("Position ", position);
+}
+return builder.build(logger);
+ }
}
Review comment:
Please add new line in the eof
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] vdiravka commented on a change in pull request #1455: DRILL-6724: Convert IndexOutOfBounds exception to UserException with …
vdiravka commented on a change in pull request #1455: DRILL-6724: Convert
IndexOutOfBounds exception to UserException with …
URL: https://github.com/apache/drill/pull/1455#discussion_r214706554
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/sequencefile/SequenceFileRecordReader.java
##
@@ -163,4 +163,23 @@ public void close() {
logger.warn("Exception closing reader: {}", e);
}
}
+
+ @Override
+ public UserException createDataReadException(Throwable cause, String
message) {
+UserException.Builder builder = UserException.dataReadError(cause)
+.message(message)
+.addContext("File ", split.getPath().toString());
+long position = -1L;
+try {
+ if (reader != null) {
+position = reader.getPos();
+ }
+} catch (IOException e) {
+ // Ignore as this is to provide information only
Review comment:
`logger.trace()`
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] vdiravka commented on a change in pull request #1455: DRILL-6724: Convert IndexOutOfBounds exception to UserException with …
vdiravka commented on a change in pull request #1455: DRILL-6724: Convert
IndexOutOfBounds exception to UserException with …
URL: https://github.com/apache/drill/pull/1455#discussion_r214548665
##
File path:
contrib/storage-hive/core/src/main/java/org/apache/drill/exec/store/hive/readers/HiveAbstractReader.java
##
@@ -430,4 +431,23 @@ protected boolean hasNextValue(Object value) {
}
}
+ @Override
+ public UserException createDataReadException(Throwable cause, String
message) {
+UserException.Builder builder = UserException.dataReadError(cause)
+.message(message)
+.addContext("Database ", table.getDbName())
+.addContext("Table ", table.getTableName());
+long position = -1;
+try {
+ if (reader != null) {
+position = reader.getPos();
+ }
+} catch (IOException e) {
+ // Ignore as this is to provide information only
Review comment:
It would be good to log this exception message on TRACE level.
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] vdiravka commented on a change in pull request #1455: DRILL-6724: Convert IndexOutOfBounds exception to UserException with …
vdiravka commented on a change in pull request #1455: DRILL-6724: Convert
IndexOutOfBounds exception to UserException with …
URL: https://github.com/apache/drill/pull/1455#discussion_r214697604
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/bson/BsonRecordReader.java
##
@@ -364,13 +367,14 @@ public void ensureAtLeastOneField(ComplexWriter writer) {
}
}
- public UserException.Builder getExceptionWithContext(UserException.Builder
exceptionBuilder, String field,
- String msg, Object... args) {
-return null;
- }
-
- public UserException.Builder getExceptionWithContext(Throwable exception,
String field, String msg, Object... args) {
-return null;
+ public UserException.Builder createExceptionWithContext(Throwable cause,
String message) {
Review comment:
It will be better to lead it to the same method name as
`JsonProcessor.getExceptionWithContext()` for consistency.
Because naturally in Mongo both `JsonReader` and `BsonBinaryreader` extend
`AbstractBsonReader`
https://github.com/mongodb/mongo-java-driver/blob/master/bson/src/main/org/bson/json/JsonReader.java#L64
Possibly it should be improved in Drill in future.
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] vdiravka commented on a change in pull request #1455: DRILL-6724: Convert IndexOutOfBounds exception to UserException with …
vdiravka commented on a change in pull request #1455: DRILL-6724: Convert
IndexOutOfBounds exception to UserException with …
URL: https://github.com/apache/drill/pull/1455#discussion_r214706522
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/avro/AvroRecordReader.java
##
@@ -398,4 +399,21 @@ public void close() {
}
}
}
+
+ @Override
+ public UserException createDataReadException(Throwable cause, String
message) {
+UserException.Builder builder = UserException.dataReadError(cause)
+.message(message)
+.addContext("File ", hadoop);
+long currentPosition = -1L;
+try {
+ currentPosition = reader.tell();
+} catch (IOException e) {
+ // Ignore as this is to provide information only
Review comment:
`logger.trace()`
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] vdiravka commented on a change in pull request #1455: DRILL-6724: Convert IndexOutOfBounds exception to UserException with …
vdiravka commented on a change in pull request #1455: DRILL-6724: Convert
IndexOutOfBounds exception to UserException with …
URL: https://github.com/apache/drill/pull/1455#discussion_r214681934
##
File path:
exec/java-exec/src/main/java/org/apache/parquet/hadoop/ColumnChunkIncReadStore.java
##
@@ -269,4 +270,10 @@ public PageReader getPageReader(ColumnDescriptor
descriptor) {
public long getRowCount() {
return rowCount;
}
+
+ public UserException.Builder getExceptionWithContext(Throwable cause, String
message) {
+return UserException.dataReadError(cause)
Review comment:
Should it be in consistency with `DrillRuntimeException` in 117 line of this
class?
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] vdiravka commented on a change in pull request #1455: DRILL-6724: Convert IndexOutOfBounds exception to UserException with …
vdiravka commented on a change in pull request #1455: DRILL-6724: Convert
IndexOutOfBounds exception to UserException with …
URL: https://github.com/apache/drill/pull/1455#discussion_r214687014
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/RecordReader.java
##
@@ -49,4 +50,15 @@
* @return The number of additional records added to the output.
*/
int next();
+
+ /**
+ * Creates {@link UserException} with reader context. Should be analogous to
+ * {@link UserException#dataReadError(Throwable)} with provided context data.
+ * Returns {@code null} if context for a reader is unavailable.
+ *
+ * @param cause a {@code Throwable} to be wrapped into {@code UserException}
+ * @param message message to be passed to resulting {@code UserException}.
Nullable
+ * @return exception with context data
Review comment:
just add `instance` word to clarify that it is not thrown, but returned.
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] vdiravka commented on a change in pull request #1455: DRILL-6724: Convert IndexOutOfBounds exception to UserException with …
vdiravka commented on a change in pull request #1455: DRILL-6724: Convert
IndexOutOfBounds exception to UserException with …
URL: https://github.com/apache/drill/pull/1455#discussion_r214674475
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/bson/BsonRecordReader.java
##
@@ -364,13 +367,14 @@ public void ensureAtLeastOneField(ComplexWriter writer) {
}
}
- public UserException.Builder getExceptionWithContext(UserException.Builder
exceptionBuilder, String field,
- String msg, Object... args) {
-return null;
- }
-
- public UserException.Builder getExceptionWithContext(Throwable exception,
String field, String msg, Object... args) {
-return null;
+ public UserException.Builder createExceptionWithContext(Throwable cause,
String message) {
+UserException.Builder builder = UserException.dataReadError(cause)
+.message(message);
+if (reader != null) {
+ builder.addContext("Name", reader.getCurrentName())
Review comment:
`addContext` from new line
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] vdiravka commented on a change in pull request #1455: DRILL-6724: Convert IndexOutOfBounds exception to UserException with …
vdiravka commented on a change in pull request #1455: DRILL-6724: Convert
IndexOutOfBounds exception to UserException with …
URL: https://github.com/apache/drill/pull/1455#discussion_r214548521
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/RecordReader.java
##
@@ -20,14 +20,15 @@
import java.util.Map;
import org.apache.drill.common.exceptions.ExecutionSetupException;
+import org.apache.drill.common.exceptions.UserException;
import org.apache.drill.exec.exception.OutOfMemoryException;
import org.apache.drill.exec.ops.OperatorContext;
import org.apache.drill.exec.physical.impl.OutputMutator;
import org.apache.drill.exec.vector.ValueVector;
public interface RecordReader extends AutoCloseable {
- public static final long ALLOCATOR_INITIAL_RESERVATION = 1*1024*1024;
- public static final long ALLOCATOR_MAX_RESERVATION = 20L*1000*1000*1000;
+ long ALLOCATOR_INITIAL_RESERVATION = 1*1024*1024;
+ long ALLOCATOR_MAX_RESERVATION = 20L*1000*1000*1000;
Review comment:
spaces
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] vdiravka commented on a change in pull request #1455: DRILL-6724: Convert IndexOutOfBounds exception to UserException with …
vdiravka commented on a change in pull request #1455: DRILL-6724: Convert
IndexOutOfBounds exception to UserException with …
URL: https://github.com/apache/drill/pull/1455#discussion_r214651648
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/sequencefile/SequenceFileRecordReader.java
##
@@ -148,7 +148,7 @@ public int next() {
return recordCount;
} catch (IOException ioe) {
close();
- throw UserException.dataReadError(ioe).addContext("File Path",
split.getPath().toString()).build(logger);
+ throw createDataReadException(ioe, null);
Review comment:
Looks like we will never get position value, so message will not be improved.
Does it make sense to close reader after creating Data Read Exception?
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] vdiravka commented on a change in pull request #1455: DRILL-6724: Convert IndexOutOfBounds exception to UserException with …
vdiravka commented on a change in pull request #1455: DRILL-6724: Convert
IndexOutOfBounds exception to UserException with …
URL: https://github.com/apache/drill/pull/1455#discussion_r214654837
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JSONRecordReader.java
##
@@ -256,4 +256,13 @@ public void close() throws Exception {
stream.close();
}
}
+
+ @Override
+ public UserException createDataReadException(Throwable cause, String
message) {
Review comment:
should it be consistent with other Readers and used in `handleAndRaise`
method or replace 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
