vvivekiyer commented on code in PR #15350:
URL: https://github.com/apache/pinot/pull/15350#discussion_r2065179730
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -1730,6 +1730,7 @@ static void updateColumnNames(String rawTableName,
PinotQuery pinotQuery, boolea
}
}
//if query has a '*' selection along with other columns
+ pinotQuery.getQueryOptions().put(QueryOptionKey.IS_SELECT_STAR_QUERY,
String.valueOf(hasStar));
Review Comment:
move this into `if(hasStar)`. We want to send this query option only when
applicable.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/ControllerRequestClient.java:
##########
@@ -241,11 +241,12 @@ public String checkIfReloadIsNeeded(String
tableNameWithType, Boolean verbose)
}
}
- public void reloadSegment(String tableName, String segmentName, boolean
forceReload)
+ public String reloadSegment(String tableName, String segmentName, boolean
forceReload)
Review Comment:
Why are we changing the return type here?
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/DataSchema.java:
##########
@@ -235,6 +237,18 @@ public RelDataType toRelDataType(RelDataTypeFactory
typeFactory) {
return typeFactory.createStructType(columnTypes,
Arrays.asList(_columnNames));
}
+ @JsonIgnore
+ public Map<String, Integer> getColumnNameToIndexMap() {
Review Comment:
Given that expanded columns for `select *` queries are always in sorted
order, do we really need this map?
We can iterate through the 2 sorted lists to find the common columns right?
##########
pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java:
##########
@@ -495,6 +497,14 @@ public boolean isIndexUseAllowed(String columnName,
FieldConfig.IndexType indexT
return !_skipIndexes.getOrDefault(columnName,
Collections.EMPTY_SET).contains(indexType);
}
+ public void setHasSchemaMisMatch(boolean hasSchemaMismatch) {
Review Comment:
Why are these setters/getters needed in QueryContext? We are detecting this
in BrokerReduceService right?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -1730,6 +1730,7 @@ static void updateColumnNames(String rawTableName,
PinotQuery pinotQuery, boolea
}
}
//if query has a '*' selection along with other columns
+ pinotQuery.getQueryOptions().put(QueryOptionKey.IS_SELECT_STAR_QUERY,
String.valueOf(hasStar));
Review Comment:
Can you add a test for what happens when tableCache doesn't contain the
table?
##########
pinot-core/src/main/java/org/apache/pinot/core/query/selection/SelectionOperatorService.java:
##########
@@ -71,62 +70,42 @@ public SelectionOperatorService(QueryContext queryContext,
DataSchema dataSchema
_offset = queryContext.getOffset();
_numRowsToKeep = _offset + queryContext.getLimit();
assert queryContext.getOrderByExpressions() != null;
- _rows = new PriorityQueue<>(Math.min(_numRowsToKeep,
SelectionOperatorUtils.MAX_ROW_HOLDER_INITIAL_CAPACITY),
-
OrderByComparatorFactory.getComparator(queryContext.getOrderByExpressions(),
- _queryContext.isNullHandlingEnabled()).reversed());
+ _rows = new ArrayList<>();
}
/**
* Reduces a collection of {@link DataTable}s to selection rows for
selection queries with <code>ORDER BY</code>.
- * TODO: Do merge sort after releasing 0.13.0 when server side results are
sorted
Review Comment:
Thanks for making this change!
##########
pinot-core/src/main/java/org/apache/pinot/core/query/selection/SelectionOperatorUtils.java:
##########
@@ -182,10 +187,12 @@ public static Pair<DataSchema, int[]>
getResultTableDataSchemaAndColumnIndices(Q
// No order-by expression
// NOTE: Order-by expressions are ignored for queries with LIMIT 0.
+ boolean isSelectStar = queryContext.getQueryOptions() != null
+ && QueryOptionsUtils.isSelectStarQuery(queryContext.getQueryOptions());
List<OrderByExpressionContext> orderByExpressions =
queryContext.getOrderByExpressions();
if (orderByExpressions == null || queryContext.getLimit() == 0) {
// For 'SELECT *', use the server response data schema as the final
results data schema.
- if ((numSelectExpressions == 1 &&
selectExpressions.get(0).equals(IDENTIFIER_STAR))) {
+ if ((numSelectExpressions == 1 &&
selectExpressions.get(0).equals(IDENTIFIER_STAR) || isSelectStar)) {
Review Comment:
If this called on the broker, select(*) is always expanded. Can we just use
the isSelectStart condition check then?
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/query/SelectionOrderByOperator.java:
##########
@@ -279,7 +279,13 @@ private SelectionResultsBlock computePartiallyOrdered() {
int numColumns = columns.size();
Map<String, DataSource> dataSourceMap = new HashMap<>();
for (String column : columns) {
- dataSourceMap.put(column, _indexSegment.getDataSource(column));
+ DataSource dataSource = _indexSegment.getDataSource(column);
+ if (dataSource != null) {
+ dataSourceMap.put(column, dataSource);
+ } else {
+ nonOrderByExpressions.remove(ExpressionContext.forIdentifier(column));
Review Comment:
Have we tested the following cases? Can we please add tests for these cases.
select * on table where:
1. WHERE filter on a column that's not fully loaded.
2. ORDER by on a column that's not fully loaded.
3. WHERE/ORDER BY on column (with transform) where column is not fully
loaded.
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
