vrajat commented on code in PR #14388:
URL: https://github.com/apache/pinot/pull/14388#discussion_r1830634421
##########
pinot-core/src/main/java/org/apache/pinot/core/common/DataBlockCache.java:
##########
@@ -129,7 +129,11 @@ public int[] getIntValuesForSVColumn(String column) {
intValues = new int[_length];
putValues(FieldSpec.DataType.INT, column, intValues);
}
- _dataFetcher.fetchIntValues(column, _docIds, _length, intValues);
+ try {
Review Comment:
This is a nit: Have you considered adding the catch block in
https://github.com/apache/pinot/blob/969bbf00d4ab0a421beaf75fdad9b189997f0e59/pinot-core/src/main/java/org/apache/pinot/core/operator/docvalsets/ProjectionBlockValSet.java#L110-L110
instead since there is a try block already ?
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/combine/BaseCombineOperator.java:
##########
@@ -122,12 +123,13 @@ public void runJob() {
// NOTE: We need to handle Error here, or the execution threads
will die without adding the execution
// exception into the query response, and the main thread
might wait infinitely (until timeout) or
// throw unexpected exceptions (such as NPE).
+ PinotRuntimeException pe =
PinotRuntimeException.create(t).withTableName(_queryContext.getTableName());
Review Comment:
One issue is that other types of exceptions like NPE may get missed when
looking at stack traces. I know that the NPE will still appear in the stack
trace etc. However as on-call our senses are trained to certain exception types
in the top message. Is there an example of how the error looks like ? Maybe you
can simulate by forcibly throwing an NPE ?
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/docvalsets/FilteredRowBasedBlockValSet.java:
##########
@@ -131,16 +139,20 @@ public long[] getLongValuesSV() {
return values;
}
PeekableIntIterator iterator = _matchedBitmap.getIntIterator();
- for (int matchedRowId = 0; matchedRowId < _numMatchedRows; matchedRowId++)
{
- int rowId = iterator.next();
- if (_matchedNullBitmap != null &&
_matchedNullBitmap.contains(matchedRowId)) {
- continue;
- }
- if (_storedType.isNumeric()) {
- values[matchedRowId] = ((Number) _rows.get(rowId)[_colId]).longValue();
- } else {
- values[matchedRowId] = Long.parseLong((String)
_rows.get(rowId)[_colId]);
+ try {
Review Comment:
I cant comment at the right line no. In line 136, column names maybe useful
in Precondition checks as well. I dont know how often they are triggered
though.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
