suneet-s commented on a change in pull request #10336:
URL: https://github.com/apache/druid/pull/10336#discussion_r482408249
##########
File path:
processing/src/main/java/org/apache/druid/segment/incremental/OnheapIncrementalIndex.java
##########
@@ -186,7 +185,7 @@ protected AddToFactsResult addToFacts(
} else {
// We lost a race
aggs = concurrentGet(prev);
- parseExceptionMessages = doAggregate(metrics, aggs, rowContainer, row);
+ doAggregate(metrics, aggs, rowContainer, row, parseExceptionMessages);
Review comment:
why not just return a List<String> like before? That seems less risky
than assuming the list is mutated correctly. In general, I'm not a big fan of
returning values from a function using parameters because of cases like this.
Either way, this comment shouldn't be considered a blocker, just my 2 cents
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/StreamChunkParser.java
##########
@@ -54,12 +64,16 @@
@Nullable InputFormat inputFormat,
InputRowSchema inputRowSchema,
TransformSpec transformSpec,
- File indexingTmpDir
+ File indexingTmpDir,
+ Predicate<InputRow> rowFilter,
Review comment:
I don't think we should use a `Predicate<InputRow>` here and in other
similar places. Throwing a `ParseException` is expected behavior, so I think
callers should know that they need to handle that exception or throw it
themselves.
It's not obvious to me how the caller in
`FilteringCloseableInputRowIterator` should know they should handle a parse
exception
https://github.com/apache/druid/pull/10336/files#diff-b7f61f3d28afdd25bedf2efa607fdf88R67
How can I check that the ParseException is handled at the correct level
everywhere? I realize this is challenging because ParseException is a
RuntimeException.
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
