jihoonson commented on a change in pull request #10383:
URL: https://github.com/apache/druid/pull/10383#discussion_r513028037
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/overlord/sampler/InputSourceSampler.java
##########
@@ -131,17 +130,27 @@ public SamplerResponse sample(
continue;
}
- for (InputRow row : inputRowListPlusRawValues.getInputRows()) {
- index.add(new SamplerInputRow(row, counter), true);
+ for (int i = 0; i < rawColumnsList.size(); i++) {
+ Map<String, Object> rawColumns = rawColumnsList.get(i);
+ InputRow row = inputRowListPlusRawValues.getInputRows().get(i);
+
+ //keep the index of the row to be added to responseRows for
further use
+ final int rowIndex = responseRows.size();
+ index.add(new SamplerInputRow(row, rowIndex), true);
+
// store the raw value; will be merged with the data from the
IncrementalIndex later
- responseRows[counter] = new SamplerResponseRow(rawColumns, null,
null, null);
- counter++;
+ responseRows.add(new SamplerResponseRow(rawColumns, null, null,
null));
numRowsIndexed++;
}
}
catch (ParseException e) {
- responseRows[counter] = new SamplerResponseRow(rawColumns, null,
true, e.getMessage());
- counter++;
+ if (rawColumnsList != null) {
+ responseRows.addAll(rawColumnsList.stream()
+ .map(rawColumns -> new
SamplerResponseRow(rawColumns, null, true, e.getMessage()))
+ .collect(Collectors.toList()));
Review comment:
> I notice that `index.add` does not throw `ParseException` but returns
the exception in its returning result. The previous version of
`InputSourceSampler` checks the result to rethrow this exception, while the
[change](https://github.com/apache/druid/pull/10336/files#diff-a35198e960bc90b96d559e9ea93be1d4de52655e9a4f04016173114952ae0001)
introduced by #10336 deletes these exception related code, which means the
code here won't throw ParseException.
>
> Of course, there's possibility that the code in for-loop throws
`ParseException` in future. I've made little changes to avoid this potential
situation to happen. Let's check it once the CI passes.
Oops, good point. I forgot what I have done in #10336.. I think
`index.add()` should never throw parseException directly, but return them in
`IncrementalIndexAddResult`. We can add this contract on the Javadoc of
`IncrementalIndex.add()`. I suggest removing the `catch` clause here to avoid
future confusion (there is no point in catching exceptions which cannot be
thrown here).
##########
File path:
core/src/main/java/org/apache/druid/data/input/InputRowListPlusRawValues.java
##########
@@ -82,8 +121,16 @@ private InputRowListPlusRawValues(
return inputRows;
}
+ /**
+ * This method is left here only for test cases
+ */
@Nullable
public Map<String, Object> getRawValues()
+ {
+ return CollectionUtils.isNullOrEmpty(rawValues) ? null : rawValues.get(0);
Review comment:
I meant, for `rawValues.get(0)` to make sure there is only one element
in there if this method is called.
----------------------------------------------------------------
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]
