You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/06/22 23:54:17 UTC
[GitHub] [incubator-pinot] Jackie-Jiang opened a new pull request #5605: [QueryContext] Use QueryContext in Operators and DataTableReducers
Jackie-Jiang opened a new pull request #5605:
URL: https://github.com/apache/incubator-pinot/pull/5605
## Description
Replace BrokerRequest with QueryContext in Operators (server side) and DataTableReducers (broker side)
The reason to put these 2 part in one PR is because they share the same code for selection queries
The change is backward-compatible because it does not involve any change on the wiring layer
Changes for QueryContext:
- Select expressions will contain both aggregation and non-aggregation expressions so that the columns in the result table can be correctly ordered
- Change method signature from `getAlias(ExpressionCOntext expression)` to `getAliasMap()` and return an unmodifiable map to make it easier to use
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5605: [QueryContext] Use QueryContext in Operators and DataTableReducers
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5605:
URL: https://github.com/apache/incubator-pinot/pull/5605#discussion_r445183627
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/reduce/GroupByDataTableReducer.java
##########
@@ -486,69 +447,44 @@ private void setGroupByResults(BrokerResponseNative brokerResponseNative, boolea
finalResultMaps[i] = finalResultMap;
}
- int aggregationNumsInFinalResult = 0;
- for (int i = 0; i < _numAggregationFunctions; i++) {
- if (aggregationFunctionsSelectStatus[i]) {
- aggregationNumsInFinalResult++;
- }
- }
-
- if (aggregationNumsInFinalResult > 0) {
Review comment:
The `aggregationFunctionsSelectStatus` is redundant (was introduced for HAVING clause), and thus this if check is no longer valid (always true). This part of the code is the same as before, the only change is the indentation.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #5605: [QueryContext] Use QueryContext in Operators and DataTableReducers
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #5605:
URL: https://github.com/apache/incubator-pinot/pull/5605#discussion_r445221361
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/reduce/GroupByDataTableReducer.java
##########
@@ -171,106 +160,81 @@ public void reduceAndSetResults(String tableName, DataSchema dataSchema,
*/
private void setSQLGroupByInResultTable(BrokerResponseNative brokerResponseNative, DataSchema dataSchema,
Collection<DataTable> dataTables) {
-
+ DataSchema resultTableSchema = getSQLResultTableSchema(dataSchema);
IndexedTable indexedTable = getIndexedTable(dataSchema, dataTables);
-
- int[] finalSchemaMapIdx = null;
- if (_sqlSelectionList != null) {
- finalSchemaMapIdx = getFinalSchemaMapIdx();
- }
- List<Object[]> rows = new ArrayList<>();
Iterator<Record> sortedIterator = indexedTable.iterator();
- int numRows = 0;
- while (numRows < _groupBy.getTopN() && sortedIterator.hasNext()) {
- Record nextRecord = sortedIterator.next();
- Object[] values = nextRecord.getValues();
-
- int index = _numGroupBy;
- int aggNum = 0;
- while (index < _numColumns) {
- values[index] = AggregationFunctionUtils
- .getSerializableValue(_aggregationFunctions[aggNum++].extractFinalResult(values[index]));
- index++;
+ int limit = _queryContext.getLimit();
+ List<Object[]> rows = new ArrayList<>(limit);
+
+ if (_sqlQuery) {
+ // NOTE: For SQL query, need to reorder the columns in the data table based on the select expressions.
+
+ int[] selectExpressionIndexMap = getSelectExpressionIndexMap();
+ int numSelectExpressions = selectExpressionIndexMap.length;
+ String[] columnNames = resultTableSchema.getColumnNames();
+ DataSchema.ColumnDataType[] columnDataTypes = resultTableSchema.getColumnDataTypes();
+ String[] reorderedColumnNames = new String[numSelectExpressions];
+ DataSchema.ColumnDataType[] reorderedColumnDataTypes = new DataSchema.ColumnDataType[numSelectExpressions];
+ resultTableSchema = new DataSchema(reorderedColumnNames, reorderedColumnDataTypes);
+ for (int i = 0; i < numSelectExpressions; i++) {
+ reorderedColumnNames[i] = columnNames[selectExpressionIndexMap[i]];
+ reorderedColumnDataTypes[i] = columnDataTypes[selectExpressionIndexMap[i]];
}
- if (_sqlSelectionList != null) {
- Object[] finalValues = new Object[_sqlSelectionList.size()];
- for (int i = 0; i < finalSchemaMapIdx.length; i++) {
- finalValues[i] = values[finalSchemaMapIdx[i]];
+ while (rows.size() < limit && sortedIterator.hasNext()) {
+ Record nextRecord = sortedIterator.next();
+ Object[] values = nextRecord.getValues();
+ for (int i = 0; i < _numAggregationFunctions; i++) {
+ int valueIndex = i + _numGroupByExpressions;
+ values[valueIndex] = AggregationFunctionUtils
+ .getSerializableValue(_aggregationFunctions[i].extractFinalResult(values[valueIndex]));
}
- rows.add(finalValues);
- } else {
- rows.add(values);
- }
- numRows++;
- }
- DataSchema finalDataSchema = getSQLResultTableSchema(dataSchema);
- if (_sqlSelectionList != null) {
- int columnSize = _sqlSelectionList.size();
- String[] columns = new String[columnSize];
- DataSchema.ColumnDataType[] columnDataTypes = new DataSchema.ColumnDataType[columnSize];
- for (int i = 0; i < columnSize; i++) {
- columns[i] = finalDataSchema.getColumnName(finalSchemaMapIdx[i]);
- columnDataTypes[i] = finalDataSchema.getColumnDataType(finalSchemaMapIdx[i]);
+ Object[] reorderedValues = new Object[numSelectExpressions];
+ for (int i = 0; i < numSelectExpressions; i++) {
+ reorderedValues[i] = values[selectExpressionIndexMap[i]];
+ }
+ rows.add(reorderedValues);
}
- finalDataSchema = new DataSchema(columns, columnDataTypes);
- }
- brokerResponseNative.setResultTable(new ResultTable(finalDataSchema, rows));
- }
-
- /**
- * Generate index mapping based on selection expression to DataTable schema, which is groupBy columns,
- * then aggregation functions.
- *
- * @return a mapping from final schema idx to corresponding idx in data table schema.
- */
- private int[] getFinalSchemaMapIdx() {
- int[] finalSchemaMapIdx = new int[_sqlSelectionList.size()];
- int nextAggregationIdx = _numGroupBy;
- for (int i = 0; i < _sqlSelectionList.size(); i++) {
- finalSchemaMapIdx[i] = getExpressionMapIdx(_sqlSelectionList.get(i), nextAggregationIdx);
- if (finalSchemaMapIdx[i] == nextAggregationIdx) {
- nextAggregationIdx++;
+ } else {
+ while (rows.size() < limit && sortedIterator.hasNext()) {
Review comment:
It will be good to add a comment here stating that this is for the code path for PQL query compiled by PQLCompiler but having both the options (groupByModeSql and responseFormatSqL to true).
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang merged pull request #5605: [QueryContext] Use QueryContext in Operators and DataTableReducers
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged pull request #5605:
URL: https://github.com/apache/incubator-pinot/pull/5605
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #5605: [QueryContext] Use QueryContext in Operators and DataTableReducers
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #5605:
URL: https://github.com/apache/incubator-pinot/pull/5605#discussion_r445137891
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/reduce/GroupByDataTableReducer.java
##########
@@ -171,106 +160,81 @@ public void reduceAndSetResults(String tableName, DataSchema dataSchema,
*/
private void setSQLGroupByInResultTable(BrokerResponseNative brokerResponseNative, DataSchema dataSchema,
Collection<DataTable> dataTables) {
-
+ DataSchema resultTableSchema = getSQLResultTableSchema(dataSchema);
IndexedTable indexedTable = getIndexedTable(dataSchema, dataTables);
-
- int[] finalSchemaMapIdx = null;
- if (_sqlSelectionList != null) {
- finalSchemaMapIdx = getFinalSchemaMapIdx();
- }
- List<Object[]> rows = new ArrayList<>();
Iterator<Record> sortedIterator = indexedTable.iterator();
- int numRows = 0;
- while (numRows < _groupBy.getTopN() && sortedIterator.hasNext()) {
- Record nextRecord = sortedIterator.next();
- Object[] values = nextRecord.getValues();
-
- int index = _numGroupBy;
- int aggNum = 0;
- while (index < _numColumns) {
- values[index] = AggregationFunctionUtils
- .getSerializableValue(_aggregationFunctions[aggNum++].extractFinalResult(values[index]));
- index++;
+ int limit = _queryContext.getLimit();
+ List<Object[]> rows = new ArrayList<>(limit);
+
+ if (_sqlQuery) {
Review comment:
I am not sure why we had to change the logic. We already have support for schema mapping for a column right? getFinalSchemaMapIdx() is supposed to do that.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #5605: [QueryContext] Use QueryContext in Operators and DataTableReducers
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #5605:
URL: https://github.com/apache/incubator-pinot/pull/5605#discussion_r445110082
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/table/SimpleIndexedTable.java
##########
@@ -49,12 +50,12 @@
* Initializes the data structures needed for this Table
* @param dataSchema data schema of the record's keys and values
* @param aggregationFunctions aggregation functions for the record's values
Review comment:
Shouldn't this be replaced with FunctionContext?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5605: [QueryContext] Use QueryContext in Operators and DataTableReducers
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5605:
URL: https://github.com/apache/incubator-pinot/pull/5605#discussion_r445182136
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/reduce/GroupByDataTableReducer.java
##########
@@ -486,69 +447,44 @@ private void setGroupByResults(BrokerResponseNative brokerResponseNative, boolea
finalResultMaps[i] = finalResultMap;
}
- int aggregationNumsInFinalResult = 0;
- for (int i = 0; i < _numAggregationFunctions; i++) {
- if (aggregationFunctionsSelectStatus[i]) {
- aggregationNumsInFinalResult++;
- }
- }
-
- if (aggregationNumsInFinalResult > 0) {
- String[] finalColumnNames = new String[aggregationNumsInFinalResult];
- Map<String, Comparable>[] finalOutResultMaps = new Map[aggregationNumsInFinalResult];
- String[] finalResultTableAggNames = new String[aggregationNumsInFinalResult];
- AggregationFunction[] finalAggregationFunctions = new AggregationFunction[aggregationNumsInFinalResult];
- int count = 0;
- for (int i = 0; i < _numAggregationFunctions; i++) {
- if (aggregationFunctionsSelectStatus[i]) {
- finalColumnNames[count] = columnNames[i];
- finalOutResultMaps[count] = finalResultMaps[i];
- AggregationFunction aggregationFunction = _aggregationFunctions[i];
- finalResultTableAggNames[count] = aggregationFunction.getResultColumnName();
- finalAggregationFunctions[count] = aggregationFunction;
- count++;
+ // Trim the final result maps and set them into the broker response.
+ AggregationGroupByTrimmingService aggregationGroupByTrimmingService =
+ new AggregationGroupByTrimmingService(_aggregationFunctions, _queryContext.getLimit());
+ List<GroupByResult>[] groupByResultLists = aggregationGroupByTrimmingService.trimFinalResults(finalResultMaps);
+
+ if (_responseFormatSql) {
Review comment:
Agree, but we have to keep this behavior for backward-compatibility.
Once we deprecate PQL semantic, all these checks will gone
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5605: [QueryContext] Use QueryContext in Operators and DataTableReducers
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5605:
URL: https://github.com/apache/incubator-pinot/pull/5605#discussion_r445132858
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/table/SimpleIndexedTable.java
##########
@@ -49,12 +50,12 @@
* Initializes the data structures needed for this Table
* @param dataSchema data schema of the record's keys and values
* @param aggregationFunctions aggregation functions for the record's values
Review comment:
No. `AggregationFunction` is the execution class instead of the info class. `FunctionContext` will replace `AggregationInfo` in the following PR of replacing BrokerRequest classes in aggregation and transform functions
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #5605: [QueryContext] Use QueryContext in Operators and DataTableReducers
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #5605:
URL: https://github.com/apache/incubator-pinot/pull/5605#discussion_r445148299
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/reduce/GroupByDataTableReducer.java
##########
@@ -486,69 +447,44 @@ private void setGroupByResults(BrokerResponseNative brokerResponseNative, boolea
finalResultMaps[i] = finalResultMap;
}
- int aggregationNumsInFinalResult = 0;
- for (int i = 0; i < _numAggregationFunctions; i++) {
- if (aggregationFunctionsSelectStatus[i]) {
- aggregationNumsInFinalResult++;
- }
- }
-
- if (aggregationNumsInFinalResult > 0) {
Review comment:
Looks like this piece of code was redundant and that'w why you invoke trimming on already compute finalResultMaps in the new code?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #5605: [QueryContext] Use QueryContext in Operators and DataTableReducers
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #5605:
URL: https://github.com/apache/incubator-pinot/pull/5605#discussion_r445221833
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/reduce/GroupByDataTableReducer.java
##########
@@ -171,106 +160,81 @@ public void reduceAndSetResults(String tableName, DataSchema dataSchema,
*/
private void setSQLGroupByInResultTable(BrokerResponseNative brokerResponseNative, DataSchema dataSchema,
Collection<DataTable> dataTables) {
-
+ DataSchema resultTableSchema = getSQLResultTableSchema(dataSchema);
IndexedTable indexedTable = getIndexedTable(dataSchema, dataTables);
-
- int[] finalSchemaMapIdx = null;
- if (_sqlSelectionList != null) {
- finalSchemaMapIdx = getFinalSchemaMapIdx();
- }
- List<Object[]> rows = new ArrayList<>();
Iterator<Record> sortedIterator = indexedTable.iterator();
- int numRows = 0;
- while (numRows < _groupBy.getTopN() && sortedIterator.hasNext()) {
- Record nextRecord = sortedIterator.next();
- Object[] values = nextRecord.getValues();
-
- int index = _numGroupBy;
- int aggNum = 0;
- while (index < _numColumns) {
- values[index] = AggregationFunctionUtils
- .getSerializableValue(_aggregationFunctions[aggNum++].extractFinalResult(values[index]));
- index++;
+ int limit = _queryContext.getLimit();
+ List<Object[]> rows = new ArrayList<>(limit);
+
+ if (_sqlQuery) {
+ // NOTE: For SQL query, need to reorder the columns in the data table based on the select expressions.
+
+ int[] selectExpressionIndexMap = getSelectExpressionIndexMap();
Review comment:
Should add a comment here that this is for SQL query compiled by calcite compiler, parser and having both the options (groupByModeSql and responseFormatSqL to true). Eventually we just want this path (SQL only) after PQL is completely deleted.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5605: [QueryContext] Use QueryContext in Operators and DataTableReducers
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5605:
URL: https://github.com/apache/incubator-pinot/pull/5605#discussion_r445226312
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/reduce/GroupByDataTableReducer.java
##########
@@ -171,106 +160,81 @@ public void reduceAndSetResults(String tableName, DataSchema dataSchema,
*/
private void setSQLGroupByInResultTable(BrokerResponseNative brokerResponseNative, DataSchema dataSchema,
Collection<DataTable> dataTables) {
-
+ DataSchema resultTableSchema = getSQLResultTableSchema(dataSchema);
IndexedTable indexedTable = getIndexedTable(dataSchema, dataTables);
-
- int[] finalSchemaMapIdx = null;
- if (_sqlSelectionList != null) {
- finalSchemaMapIdx = getFinalSchemaMapIdx();
- }
- List<Object[]> rows = new ArrayList<>();
Iterator<Record> sortedIterator = indexedTable.iterator();
- int numRows = 0;
- while (numRows < _groupBy.getTopN() && sortedIterator.hasNext()) {
- Record nextRecord = sortedIterator.next();
- Object[] values = nextRecord.getValues();
-
- int index = _numGroupBy;
- int aggNum = 0;
- while (index < _numColumns) {
- values[index] = AggregationFunctionUtils
- .getSerializableValue(_aggregationFunctions[aggNum++].extractFinalResult(values[index]));
- index++;
+ int limit = _queryContext.getLimit();
+ List<Object[]> rows = new ArrayList<>(limit);
+
+ if (_sqlQuery) {
+ // NOTE: For SQL query, need to reorder the columns in the data table based on the select expressions.
+
+ int[] selectExpressionIndexMap = getSelectExpressionIndexMap();
Review comment:
Good point, added
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5605: [QueryContext] Use QueryContext in Operators and DataTableReducers
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5605:
URL: https://github.com/apache/incubator-pinot/pull/5605#discussion_r445181570
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/reduce/GroupByDataTableReducer.java
##########
@@ -171,106 +160,81 @@ public void reduceAndSetResults(String tableName, DataSchema dataSchema,
*/
private void setSQLGroupByInResultTable(BrokerResponseNative brokerResponseNative, DataSchema dataSchema,
Collection<DataTable> dataTables) {
-
+ DataSchema resultTableSchema = getSQLResultTableSchema(dataSchema);
IndexedTable indexedTable = getIndexedTable(dataSchema, dataTables);
-
- int[] finalSchemaMapIdx = null;
- if (_sqlSelectionList != null) {
- finalSchemaMapIdx = getFinalSchemaMapIdx();
- }
- List<Object[]> rows = new ArrayList<>();
Iterator<Record> sortedIterator = indexedTable.iterator();
- int numRows = 0;
- while (numRows < _groupBy.getTopN() && sortedIterator.hasNext()) {
- Record nextRecord = sortedIterator.next();
- Object[] values = nextRecord.getValues();
-
- int index = _numGroupBy;
- int aggNum = 0;
- while (index < _numColumns) {
- values[index] = AggregationFunctionUtils
- .getSerializableValue(_aggregationFunctions[aggNum++].extractFinalResult(values[index]));
- index++;
+ int limit = _queryContext.getLimit();
+ List<Object[]> rows = new ArrayList<>(limit);
+
+ if (_sqlQuery) {
+ // NOTE: For SQL query, need to reorder the columns in the data table based on the select expressions.
+
+ int[] selectExpressionIndexMap = getSelectExpressionIndexMap();
+ int numSelectExpressions = selectExpressionIndexMap.length;
+ String[] columnNames = resultTableSchema.getColumnNames();
+ DataSchema.ColumnDataType[] columnDataTypes = resultTableSchema.getColumnDataTypes();
+ String[] reorderedColumnNames = new String[numSelectExpressions];
+ DataSchema.ColumnDataType[] reorderedColumnDataTypes = new DataSchema.ColumnDataType[numSelectExpressions];
+ resultTableSchema = new DataSchema(reorderedColumnNames, reorderedColumnDataTypes);
+ for (int i = 0; i < numSelectExpressions; i++) {
+ reorderedColumnNames[i] = columnNames[selectExpressionIndexMap[i]];
+ reorderedColumnDataTypes[i] = columnDataTypes[selectExpressionIndexMap[i]];
}
- if (_sqlSelectionList != null) {
Review comment:
I don't think so. The existing logic have the same if condition in 3 places (line 178, 195, 208), and this one is even in a while loop. Instead, the change makes it one time branching, which is much more clear IMO.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5605: [QueryContext] Use QueryContext in Operators and DataTableReducers
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5605:
URL: https://github.com/apache/incubator-pinot/pull/5605#discussion_r444466740
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/CombineGroupByOperator.java
##########
@@ -174,8 +173,8 @@ public void runJob() {
} catch (EarlyTerminationException e) {
// Early-terminated because query times out or is already satisfied
} catch (Exception e) {
- LOGGER.error("Exception processing CombineGroupBy for index {}, operator {}, brokerRequest {}", index,
- _operators.get(index).getClass().getName(), _brokerRequest, e);
+ LOGGER.error("Caught exception while processing group-by for index: {}, operator: {}, queryContext: {}",
Review comment:
Added
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #5605: [QueryContext] Use QueryContext in Operators and DataTableReducers
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #5605:
URL: https://github.com/apache/incubator-pinot/pull/5605#discussion_r445148299
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/reduce/GroupByDataTableReducer.java
##########
@@ -486,69 +447,44 @@ private void setGroupByResults(BrokerResponseNative brokerResponseNative, boolea
finalResultMaps[i] = finalResultMap;
}
- int aggregationNumsInFinalResult = 0;
- for (int i = 0; i < _numAggregationFunctions; i++) {
- if (aggregationFunctionsSelectStatus[i]) {
- aggregationNumsInFinalResult++;
- }
- }
-
- if (aggregationNumsInFinalResult > 0) {
Review comment:
Looks like this piece of code was redundant and that'w why you invoke trimming on already computed finalResultMaps in the new code?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #5605: [QueryContext] Use QueryContext in Operators and DataTableReducers
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #5605:
URL: https://github.com/apache/incubator-pinot/pull/5605#discussion_r445139005
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/reduce/GroupByDataTableReducer.java
##########
@@ -171,106 +160,81 @@ public void reduceAndSetResults(String tableName, DataSchema dataSchema,
*/
private void setSQLGroupByInResultTable(BrokerResponseNative brokerResponseNative, DataSchema dataSchema,
Collection<DataTable> dataTables) {
-
+ DataSchema resultTableSchema = getSQLResultTableSchema(dataSchema);
IndexedTable indexedTable = getIndexedTable(dataSchema, dataTables);
-
- int[] finalSchemaMapIdx = null;
- if (_sqlSelectionList != null) {
- finalSchemaMapIdx = getFinalSchemaMapIdx();
- }
- List<Object[]> rows = new ArrayList<>();
Iterator<Record> sortedIterator = indexedTable.iterator();
- int numRows = 0;
- while (numRows < _groupBy.getTopN() && sortedIterator.hasNext()) {
- Record nextRecord = sortedIterator.next();
- Object[] values = nextRecord.getValues();
-
- int index = _numGroupBy;
- int aggNum = 0;
- while (index < _numColumns) {
- values[index] = AggregationFunctionUtils
- .getSerializableValue(_aggregationFunctions[aggNum++].extractFinalResult(values[index]));
- index++;
+ int limit = _queryContext.getLimit();
+ List<Object[]> rows = new ArrayList<>(limit);
+
+ if (_sqlQuery) {
+ // NOTE: For SQL query, need to reorder the columns in the data table based on the select expressions.
+
+ int[] selectExpressionIndexMap = getSelectExpressionIndexMap();
+ int numSelectExpressions = selectExpressionIndexMap.length;
+ String[] columnNames = resultTableSchema.getColumnNames();
+ DataSchema.ColumnDataType[] columnDataTypes = resultTableSchema.getColumnDataTypes();
+ String[] reorderedColumnNames = new String[numSelectExpressions];
+ DataSchema.ColumnDataType[] reorderedColumnDataTypes = new DataSchema.ColumnDataType[numSelectExpressions];
+ resultTableSchema = new DataSchema(reorderedColumnNames, reorderedColumnDataTypes);
+ for (int i = 0; i < numSelectExpressions; i++) {
+ reorderedColumnNames[i] = columnNames[selectExpressionIndexMap[i]];
+ reorderedColumnDataTypes[i] = columnDataTypes[selectExpressionIndexMap[i]];
}
- if (_sqlSelectionList != null) {
Review comment:
This existing logic seems simpler
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #5605: [QueryContext] Use QueryContext in Operators and DataTableReducers
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #5605:
URL: https://github.com/apache/incubator-pinot/pull/5605#discussion_r445145735
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/reduce/GroupByDataTableReducer.java
##########
@@ -486,69 +447,44 @@ private void setGroupByResults(BrokerResponseNative brokerResponseNative, boolea
finalResultMaps[i] = finalResultMap;
}
- int aggregationNumsInFinalResult = 0;
- for (int i = 0; i < _numAggregationFunctions; i++) {
- if (aggregationFunctionsSelectStatus[i]) {
- aggregationNumsInFinalResult++;
- }
- }
-
- if (aggregationNumsInFinalResult > 0) {
- String[] finalColumnNames = new String[aggregationNumsInFinalResult];
- Map<String, Comparable>[] finalOutResultMaps = new Map[aggregationNumsInFinalResult];
- String[] finalResultTableAggNames = new String[aggregationNumsInFinalResult];
- AggregationFunction[] finalAggregationFunctions = new AggregationFunction[aggregationNumsInFinalResult];
- int count = 0;
- for (int i = 0; i < _numAggregationFunctions; i++) {
- if (aggregationFunctionsSelectStatus[i]) {
- finalColumnNames[count] = columnNames[i];
- finalOutResultMaps[count] = finalResultMaps[i];
- AggregationFunction aggregationFunction = _aggregationFunctions[i];
- finalResultTableAggNames[count] = aggregationFunction.getResultColumnName();
- finalAggregationFunctions[count] = aggregationFunction;
- count++;
+ // Trim the final result maps and set them into the broker response.
+ AggregationGroupByTrimmingService aggregationGroupByTrimmingService =
+ new AggregationGroupByTrimmingService(_aggregationFunctions, _queryContext.getLimit());
+ List<GroupByResult>[] groupByResultLists = aggregationGroupByTrimmingService.trimFinalResults(finalResultMaps);
+
+ if (_responseFormatSql) {
Review comment:
We should actually consider deleting this. We should not mix PQL with SQL. This code path is for PQL execution but SQL response format. We should not let users avail that. Ideally, it should be
SQL query -> SQL execution -> SQL response
PQL query -> PQL execution -> PQL response
The functions in this class are extraordinarily large in size and the code looks very confusing because of these branches (not related to this PR)
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #5605: [QueryContext] Use QueryContext in Operators and DataTableReducers
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #5605:
URL: https://github.com/apache/incubator-pinot/pull/5605#discussion_r445145735
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/reduce/GroupByDataTableReducer.java
##########
@@ -486,69 +447,44 @@ private void setGroupByResults(BrokerResponseNative brokerResponseNative, boolea
finalResultMaps[i] = finalResultMap;
}
- int aggregationNumsInFinalResult = 0;
- for (int i = 0; i < _numAggregationFunctions; i++) {
- if (aggregationFunctionsSelectStatus[i]) {
- aggregationNumsInFinalResult++;
- }
- }
-
- if (aggregationNumsInFinalResult > 0) {
- String[] finalColumnNames = new String[aggregationNumsInFinalResult];
- Map<String, Comparable>[] finalOutResultMaps = new Map[aggregationNumsInFinalResult];
- String[] finalResultTableAggNames = new String[aggregationNumsInFinalResult];
- AggregationFunction[] finalAggregationFunctions = new AggregationFunction[aggregationNumsInFinalResult];
- int count = 0;
- for (int i = 0; i < _numAggregationFunctions; i++) {
- if (aggregationFunctionsSelectStatus[i]) {
- finalColumnNames[count] = columnNames[i];
- finalOutResultMaps[count] = finalResultMaps[i];
- AggregationFunction aggregationFunction = _aggregationFunctions[i];
- finalResultTableAggNames[count] = aggregationFunction.getResultColumnName();
- finalAggregationFunctions[count] = aggregationFunction;
- count++;
+ // Trim the final result maps and set them into the broker response.
+ AggregationGroupByTrimmingService aggregationGroupByTrimmingService =
+ new AggregationGroupByTrimmingService(_aggregationFunctions, _queryContext.getLimit());
+ List<GroupByResult>[] groupByResultLists = aggregationGroupByTrimmingService.trimFinalResults(finalResultMaps);
+
+ if (_responseFormatSql) {
Review comment:
We should actually consider deleting this. We should not mix PQL with SQL. This code path is for PQL execution but SQL response format. We should not let users avail that. Ideally, it should be
SQL query -> SQL execution -> SQL response
PQL query -> PQL execution -> PQL response
The functions in this class are extraordinarily large in size and the code looks very confusing.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mayankshriv commented on a change in pull request #5605: [QueryContext] Use QueryContext in Operators and DataTableReducers
Posted by GitBox <gi...@apache.org>.
mayankshriv commented on a change in pull request #5605:
URL: https://github.com/apache/incubator-pinot/pull/5605#discussion_r444417005
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/CombineGroupByOperator.java
##########
@@ -174,8 +173,8 @@ public void runJob() {
} catch (EarlyTerminationException e) {
// Early-terminated because query times out or is already satisfied
} catch (Exception e) {
- LOGGER.error("Exception processing CombineGroupBy for index {}, operator {}, brokerRequest {}", index,
- _operators.get(index).getClass().getName(), _brokerRequest, e);
+ LOGGER.error("Caught exception while processing group-by for index: {}, operator: {}, queryContext: {}",
Review comment:
Perhaps add something to indicate that this is the combine phase? For example, `Caught exception in combine phase of group-by...`?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5605: [QueryContext] Use QueryContext in Operators and DataTableReducers
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5605:
URL: https://github.com/apache/incubator-pinot/pull/5605#discussion_r445180740
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/reduce/GroupByDataTableReducer.java
##########
@@ -171,106 +160,81 @@ public void reduceAndSetResults(String tableName, DataSchema dataSchema,
*/
private void setSQLGroupByInResultTable(BrokerResponseNative brokerResponseNative, DataSchema dataSchema,
Collection<DataTable> dataTables) {
-
+ DataSchema resultTableSchema = getSQLResultTableSchema(dataSchema);
IndexedTable indexedTable = getIndexedTable(dataSchema, dataTables);
-
- int[] finalSchemaMapIdx = null;
- if (_sqlSelectionList != null) {
- finalSchemaMapIdx = getFinalSchemaMapIdx();
- }
- List<Object[]> rows = new ArrayList<>();
Iterator<Record> sortedIterator = indexedTable.iterator();
- int numRows = 0;
- while (numRows < _groupBy.getTopN() && sortedIterator.hasNext()) {
- Record nextRecord = sortedIterator.next();
- Object[] values = nextRecord.getValues();
-
- int index = _numGroupBy;
- int aggNum = 0;
- while (index < _numColumns) {
- values[index] = AggregationFunctionUtils
- .getSerializableValue(_aggregationFunctions[aggNum++].extractFinalResult(values[index]));
- index++;
+ int limit = _queryContext.getLimit();
+ List<Object[]> rows = new ArrayList<>(limit);
+
+ if (_sqlQuery) {
Review comment:
`getFinalSchemaMapIdx` is renamed to `getSelectExpressionIndexMap` for readability.
The existing logic keeps the sql query check inside the loop, which is not efficient and not as readable IMO. The change makes the sql query check at the top level, and once we move to SQL completely, we can simply remove the `else` part.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5605: [QueryContext] Use QueryContext in Operators and DataTableReducers
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5605:
URL: https://github.com/apache/incubator-pinot/pull/5605#discussion_r445227178
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/reduce/GroupByDataTableReducer.java
##########
@@ -171,106 +160,81 @@ public void reduceAndSetResults(String tableName, DataSchema dataSchema,
*/
private void setSQLGroupByInResultTable(BrokerResponseNative brokerResponseNative, DataSchema dataSchema,
Collection<DataTable> dataTables) {
-
+ DataSchema resultTableSchema = getSQLResultTableSchema(dataSchema);
IndexedTable indexedTable = getIndexedTable(dataSchema, dataTables);
-
- int[] finalSchemaMapIdx = null;
- if (_sqlSelectionList != null) {
- finalSchemaMapIdx = getFinalSchemaMapIdx();
- }
- List<Object[]> rows = new ArrayList<>();
Iterator<Record> sortedIterator = indexedTable.iterator();
- int numRows = 0;
- while (numRows < _groupBy.getTopN() && sortedIterator.hasNext()) {
- Record nextRecord = sortedIterator.next();
- Object[] values = nextRecord.getValues();
-
- int index = _numGroupBy;
- int aggNum = 0;
- while (index < _numColumns) {
- values[index] = AggregationFunctionUtils
- .getSerializableValue(_aggregationFunctions[aggNum++].extractFinalResult(values[index]));
- index++;
+ int limit = _queryContext.getLimit();
+ List<Object[]> rows = new ArrayList<>(limit);
+
+ if (_sqlQuery) {
+ // NOTE: For SQL query, need to reorder the columns in the data table based on the select expressions.
+
+ int[] selectExpressionIndexMap = getSelectExpressionIndexMap();
+ int numSelectExpressions = selectExpressionIndexMap.length;
+ String[] columnNames = resultTableSchema.getColumnNames();
+ DataSchema.ColumnDataType[] columnDataTypes = resultTableSchema.getColumnDataTypes();
+ String[] reorderedColumnNames = new String[numSelectExpressions];
+ DataSchema.ColumnDataType[] reorderedColumnDataTypes = new DataSchema.ColumnDataType[numSelectExpressions];
+ resultTableSchema = new DataSchema(reorderedColumnNames, reorderedColumnDataTypes);
+ for (int i = 0; i < numSelectExpressions; i++) {
+ reorderedColumnNames[i] = columnNames[selectExpressionIndexMap[i]];
+ reorderedColumnDataTypes[i] = columnDataTypes[selectExpressionIndexMap[i]];
}
- if (_sqlSelectionList != null) {
- Object[] finalValues = new Object[_sqlSelectionList.size()];
- for (int i = 0; i < finalSchemaMapIdx.length; i++) {
- finalValues[i] = values[finalSchemaMapIdx[i]];
+ while (rows.size() < limit && sortedIterator.hasNext()) {
+ Record nextRecord = sortedIterator.next();
+ Object[] values = nextRecord.getValues();
+ for (int i = 0; i < _numAggregationFunctions; i++) {
+ int valueIndex = i + _numGroupByExpressions;
+ values[valueIndex] = AggregationFunctionUtils
+ .getSerializableValue(_aggregationFunctions[i].extractFinalResult(values[valueIndex]));
}
- rows.add(finalValues);
- } else {
- rows.add(values);
- }
- numRows++;
- }
- DataSchema finalDataSchema = getSQLResultTableSchema(dataSchema);
- if (_sqlSelectionList != null) {
- int columnSize = _sqlSelectionList.size();
- String[] columns = new String[columnSize];
- DataSchema.ColumnDataType[] columnDataTypes = new DataSchema.ColumnDataType[columnSize];
- for (int i = 0; i < columnSize; i++) {
- columns[i] = finalDataSchema.getColumnName(finalSchemaMapIdx[i]);
- columnDataTypes[i] = finalDataSchema.getColumnDataType(finalSchemaMapIdx[i]);
+ Object[] reorderedValues = new Object[numSelectExpressions];
+ for (int i = 0; i < numSelectExpressions; i++) {
+ reorderedValues[i] = values[selectExpressionIndexMap[i]];
+ }
+ rows.add(reorderedValues);
}
- finalDataSchema = new DataSchema(columns, columnDataTypes);
- }
- brokerResponseNative.setResultTable(new ResultTable(finalDataSchema, rows));
- }
-
- /**
- * Generate index mapping based on selection expression to DataTable schema, which is groupBy columns,
- * then aggregation functions.
- *
- * @return a mapping from final schema idx to corresponding idx in data table schema.
- */
- private int[] getFinalSchemaMapIdx() {
- int[] finalSchemaMapIdx = new int[_sqlSelectionList.size()];
- int nextAggregationIdx = _numGroupBy;
- for (int i = 0; i < _sqlSelectionList.size(); i++) {
- finalSchemaMapIdx[i] = getExpressionMapIdx(_sqlSelectionList.get(i), nextAggregationIdx);
- if (finalSchemaMapIdx[i] == nextAggregationIdx) {
- nextAggregationIdx++;
+ } else {
+ while (rows.size() < limit && sortedIterator.hasNext()) {
Review comment:
Added
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org