Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add table and column information to query errors #14388

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nit: Have you considered adding the catch block in

instead since there is a try block already ?

_dataFetcher.fetchIntValues(column, _docIds, _length, intValues);
} catch (RuntimeException re) {
throw PinotRuntimeException.create(re).withColumnName(column);
}
}
return intValues;
}
Expand All @@ -147,7 +151,11 @@ public long[] getLongValuesForSVColumn(String column) {
longValues = new long[_length];
putValues(FieldSpec.DataType.LONG, column, longValues);
}
_dataFetcher.fetchLongValues(column, _docIds, _length, longValues);
try {
_dataFetcher.fetchLongValues(column, _docIds, _length, longValues);
} catch (RuntimeException re) {
throw PinotRuntimeException.create(re).withColumnName(column);
}
}
return longValues;
}
Expand All @@ -165,7 +173,11 @@ public float[] getFloatValuesForSVColumn(String column) {
floatValues = new float[_length];
putValues(FieldSpec.DataType.FLOAT, column, floatValues);
}
_dataFetcher.fetchFloatValues(column, _docIds, _length, floatValues);
try {
_dataFetcher.fetchFloatValues(column, _docIds, _length, floatValues);
} catch (RuntimeException re) {
throw PinotRuntimeException.create(re).withColumnName(column);
}
}
return floatValues;
}
Expand All @@ -183,7 +195,11 @@ public double[] getDoubleValuesForSVColumn(String column) {
doubleValues = new double[_length];
putValues(FieldSpec.DataType.DOUBLE, column, doubleValues);
}
_dataFetcher.fetchDoubleValues(column, _docIds, _length, doubleValues);
try {
_dataFetcher.fetchDoubleValues(column, _docIds, _length, doubleValues);
} catch (RuntimeException re) {
throw PinotRuntimeException.create(re).withColumnName(column);
}
}
return doubleValues;
}
Expand All @@ -201,7 +217,11 @@ public BigDecimal[] getBigDecimalValuesForSVColumn(String column) {
bigDecimalValues = new BigDecimal[_length];
putValues(FieldSpec.DataType.BIG_DECIMAL, column, bigDecimalValues);
}
_dataFetcher.fetchBigDecimalValues(column, _docIds, _length, bigDecimalValues);
try {
_dataFetcher.fetchBigDecimalValues(column, _docIds, _length, bigDecimalValues);
} catch (RuntimeException re) {
throw PinotRuntimeException.create(re).withColumnName(column);
}
}
return bigDecimalValues;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/**
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.pinot.core.common;

Copy link
Contributor Author

@bziobrowski bziobrowski Nov 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one's a just a simple exception that makes it possible to fetch important context information spread throught the stack.

I'd be good to discuss how to integrate the approach with other exceptions and re-organize exceptions thrown by the engine. There are many more places that don't report conversion errors properly (e.g. plenty of parse calls for function arguments that won't report which argument they failed for).

@Jackie-Jiang @gortiz @yashmayya @vrajat

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a really nice improvement! IMO we could add similar custom runtime exceptions for different classes of processing errors as a subsequent enhancement. Although in that case it might be nice to name this class something more specific than PinotRuntimeException, WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completely agree we need to have our own family of exceptions, but I think it will need more discussion.

My initial suggestion is to reserve PinotRuntimeException name for a marker exception that extends RuntimeException, then add another for queries (something like QueryException) and then

---
config:
  look: handDrawn
  theme: neutral
---
classDiagram
    RuntimeException
    note for RuntimeException "From JDK"

    PinotRuntimeException
    note for PinotRuntimeException "All new Pinot exceptions"

    QException
    note for QException "All new Pinot exceptions"

    QException <|-- ParsingQException
    QException <|-- OptimizationQException

    ExecutionQException
    note for ExecutionQException "Exceptions thrown by Operators"

    RuntimeException <|-- PinotRuntimeException
    PinotRuntimeException <|-- QException
    QException <|-- ExecutionQException
    ExecutionQException <|-- ColumnExecutionQException
    
    class ExecutionQException {
        +stage
    }

    class ColumnExecutionQException {
        +table
        +column
    }
    note for ColumnExecutionQException "Execution exception dealing with a column"
Loading

/**
* Simple runtime query exception containing useful contextual information, e.g. table or column name.
*/
public class PinotRuntimeException extends RuntimeException {

//table name or alias
private String _tableName;

// column or expression name
private String _columnName;

private PinotRuntimeException(Throwable e) {
super(e);
}

public static PinotRuntimeException create(Throwable e) {
if (e instanceof PinotRuntimeException) {
return (PinotRuntimeException) e;
} else {
return new PinotRuntimeException(e);
}
}

public PinotRuntimeException withColumnName(String columnName) {
_columnName = columnName;
return this;
}

public PinotRuntimeException withTableName(String tableName) {
_tableName = tableName;
return this;
}

@Override
public String getMessage() {
StringBuilder message = new StringBuilder("Error when processing");
if (_tableName != null) {
message.append(" ").append(_tableName);
}
if (_columnName != null) {
if (_tableName != null) {
message.append(".");
} else {
message.append(" ");
}
message.append(_columnName);
}
message.append(": ");
message.append(super.getMessage());

return message.toString();
Comment on lines +56 to +71
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be clearer to change this to something like Error when processing column "abc" in table "xyz"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to suggestions. We've to keep in mind that table could actually mean alias and column - an expression - e.g. max(x) so it should be generic but still readable :).
I guess we can add more useful bits of context data.

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import java.util.concurrent.atomic.AtomicReference;
import org.apache.pinot.common.exception.QueryException;
import org.apache.pinot.core.common.Operator;
import org.apache.pinot.core.common.PinotRuntimeException;
import org.apache.pinot.core.operator.BaseOperator;
import org.apache.pinot.core.operator.blocks.results.BaseResultsBlock;
import org.apache.pinot.core.operator.blocks.results.ExceptionResultsBlock;
Expand Down Expand Up @@ -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());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

if (t instanceof Exception) {
LOGGER.error("Caught exception while processing query: {}", _queryContext, t);
LOGGER.error("Caught exception while processing query: {}", _queryContext, pe);
} else {
LOGGER.error("Caught serious error while processing query: {}", _queryContext, t);
LOGGER.error("Caught serious error while processing query: {}", _queryContext, pe);
}
onProcessSegmentsException(t);
onProcessSegmentsException(pe);
} finally {
onProcessSegmentsFinish();
_phaser.arriveAndDeregister();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import javax.annotation.Nullable;
import org.apache.pinot.common.utils.DataSchema.ColumnDataType;
import org.apache.pinot.core.common.BlockValSet;
import org.apache.pinot.core.common.PinotRuntimeException;
import org.apache.pinot.segment.spi.index.reader.Dictionary;
import org.apache.pinot.spi.data.FieldSpec.DataType;
import org.apache.pinot.spi.utils.BigDecimalUtils;
Expand All @@ -43,17 +44,20 @@ public class FilteredRowBasedBlockValSet implements BlockValSet {
private final DataType _dataType;
private final DataType _storedType;
private final List<Object[]> _rows;
private final String _colName;
private final int _colId;
private final int _numMatchedRows;
private final RoaringBitmap _matchedBitmap;
private final RoaringBitmap _matchedNullBitmap;

public FilteredRowBasedBlockValSet(ColumnDataType columnDataType, List<Object[]> rows, int colId, int numMatchedRows,
public FilteredRowBasedBlockValSet(ColumnDataType columnDataType, List<Object[]> rows, int colId, String columnName,
int numMatchedRows,
RoaringBitmap matchedBitmap, boolean nullHandlingEnabled) {
_dataType = columnDataType.toDataType();
_storedType = _dataType.getStoredType();
_rows = rows;
_colId = colId;
_colName = columnName;
_numMatchedRows = numMatchedRows;
_matchedBitmap = matchedBitmap;

Expand Down Expand Up @@ -108,16 +112,20 @@ public int[] getIntValuesSV() {
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]).intValue();
} else {
values[matchedRowId] = Integer.parseInt((String) _rows.get(rowId)[_colId]);
try {
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]).intValue();
} else {
values[matchedRowId] = Integer.parseInt((String) _rows.get(rowId)[_colId]);
}
}
} catch (RuntimeException re) {
throw PinotRuntimeException.create(re).withColumnName(_colName);
}
return values;
}
Expand All @@ -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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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]);
}
}
} catch (RuntimeException re) {
throw PinotRuntimeException.create(re).withColumnName(_colName);
}
return values;
}
Expand All @@ -154,16 +166,20 @@ public float[] getFloatValuesSV() {
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]).floatValue();
} else {
values[matchedRowId] = Float.parseFloat((String) _rows.get(rowId)[_colId]);
try {
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]).floatValue();
} else {
values[matchedRowId] = Float.parseFloat((String) _rows.get(rowId)[_colId]);
}
}
} catch (RuntimeException re) {
throw PinotRuntimeException.create(re).withColumnName(_colName);
}
return values;
}
Expand All @@ -177,16 +193,20 @@ public double[] getDoubleValuesSV() {
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]).doubleValue();
} else {
values[matchedRowId] = Double.parseDouble((String) _rows.get(rowId)[_colId]);
try {
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]).doubleValue();
} else {
values[matchedRowId] = Double.parseDouble((String) _rows.get(rowId)[_colId]);
}
}
} catch (RuntimeException re) {
throw PinotRuntimeException.create(re).withColumnName(_colName);
}
return values;
}
Expand All @@ -202,31 +222,35 @@ public BigDecimal[] getBigDecimalValuesSV() {
return values;
}
PeekableIntIterator iterator = _matchedBitmap.getIntIterator();
for (int matchedRowId = 0; matchedRowId < _numMatchedRows; matchedRowId++) {
int rowId = iterator.next();
if (_matchedNullBitmap != null && _matchedNullBitmap.contains(matchedRowId)) {
values[matchedRowId] = NullValuePlaceHolder.BIG_DECIMAL;
continue;
}
switch (_storedType) {
case INT:
case LONG:
values[matchedRowId] = BigDecimal.valueOf(((Number) _rows.get(rowId)[_colId]).longValue());
break;
case FLOAT:
case DOUBLE:
case STRING:
values[matchedRowId] = new BigDecimal(_rows.get(rowId)[_colId].toString());
break;
case BIG_DECIMAL:
values[matchedRowId] = (BigDecimal) _rows.get(rowId)[_colId];
break;
case BYTES:
values[matchedRowId] = BigDecimalUtils.deserialize((ByteArray) _rows.get(rowId)[_colId]);
break;
default:
throw new IllegalStateException("Cannot read BigDecimal values from data type: " + _dataType);
try {
for (int matchedRowId = 0; matchedRowId < _numMatchedRows; matchedRowId++) {
int rowId = iterator.next();
if (_matchedNullBitmap != null && _matchedNullBitmap.contains(matchedRowId)) {
values[matchedRowId] = NullValuePlaceHolder.BIG_DECIMAL;
continue;
}
switch (_storedType) {
case INT:
case LONG:
values[matchedRowId] = BigDecimal.valueOf(((Number) _rows.get(rowId)[_colId]).longValue());
break;
case FLOAT:
case DOUBLE:
case STRING:
values[matchedRowId] = new BigDecimal(_rows.get(rowId)[_colId].toString());
break;
case BIG_DECIMAL:
values[matchedRowId] = (BigDecimal) _rows.get(rowId)[_colId];
break;
case BYTES:
values[matchedRowId] = BigDecimalUtils.deserialize((ByteArray) _rows.get(rowId)[_colId]);
break;
default:
throw new IllegalStateException("Cannot read BigDecimal values from data type: " + _dataType);
}
}
} catch (RuntimeException re) {
throw PinotRuntimeException.create(re).withColumnName(_colName);
}
return values;
}
Expand Down
Loading
Loading