Jackie-Jiang commented on a change in pull request #5513:
URL: https://github.com/apache/incubator-pinot/pull/5513#discussion_r437008430
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -184,6 +192,18 @@ public BrokerResponse handleRequest(JsonNode request,
@Nullable RequesterIdentit
requestStatistics.setErrorCode(QueryException.PQL_PARSING_ERROR_CODE);
return new
BrokerResponseNative(QueryException.getException(QueryException.PQL_PARSING_ERROR,
e));
}
+ if (isLiteralOnlyQuery(brokerRequest)) {
+ LOGGER.info("Request {} contains only Literal, skipping server query:
{}", requestId, query);
Review comment:
This can potentially flood the log. Can we extract line 376-405 in
existing code into a method and call that here?
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -435,6 +455,116 @@ static void handleQueryLimitOverride(BrokerRequest
brokerRequest, int queryLimit
}
}
+ /**
+ * Check if a SQL parsed BrokerRequest is a literal only query.
+ * @param brokerRequest
+ * @return true if this query selects only Literals
+ *
+ */
+ @VisibleForTesting
+ static boolean isLiteralOnlyQuery(BrokerRequest brokerRequest) {
+ if (brokerRequest.getPinotQuery() != null) {
+ for (Expression e: brokerRequest.getPinotQuery().getSelectList()) {
+ if (!CalciteSqlParser.isLiteralOnlyExpression(e)) {
+ return false;
+ }
+ }
+ return true;
+ }
+ return false;
+ }
+
+ /**
+ * Compute BrokerResponse for literal only query.
+ *
+ * @param brokerRequest
+ * @param compilationStartTimeNs
+ * @param requestStatistics
+ * @return BrokerResponse
+ */
+ private BrokerResponse processLiteralOnlyBrokerRequest(BrokerRequest
brokerRequest, long compilationStartTimeNs,
+ RequestStatistics requestStatistics)
+ throws IllegalStateException {
+ System.out.println("brokerRequest = " + brokerRequest.toString());
+ BrokerResponseNative brokerResponse = new BrokerResponseNative();
+ List<String> columnNames = new ArrayList<>();
+ List<DataSchema.ColumnDataType> columnTypes = new ArrayList<>();
+ List<Object> row = new ArrayList<>();
+ for (Expression e : brokerRequest.getPinotQuery().getSelectList()) {
+ computeResultsForExpression(e, columnNames, columnTypes, row);
+ }
+ DataSchema dataSchema =
+ new DataSchema(columnNames.toArray(new String[0]),
columnTypes.toArray(new DataSchema.ColumnDataType[0]));
+ List<Object[]> rows = new ArrayList<>();
+ rows.add(row.toArray());
+ ResultTable resultTable = new ResultTable(dataSchema, rows);
+ brokerResponse.setResultTable(resultTable);
+
+ long totalTimeMs = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() -
compilationStartTimeNs);
+ brokerResponse.setTimeUsedMs(totalTimeMs);
+ requestStatistics.setQueryProcessingTime(totalTimeMs);
+ requestStatistics.setStatistics(brokerResponse);
+ return brokerResponse;
+ }
+
+ private void computeResultsForExpression(Expression e, List<String>
columnNames, List<DataSchema.ColumnDataType> columnTypes,
+ List<Object> row) {
+ if (e.getType() == ExpressionType.LITERAL) {
+ computeResultsForLiteral(e.getLiteral(), columnNames, columnTypes, row);
+ }
+ if (e.getType() == ExpressionType.FUNCTION) {
+ if
(e.getFunctionCall().getOperator().equalsIgnoreCase(SqlKind.AS.toString())) {
+ String columnName =
e.getFunctionCall().getOperands().get(1).getIdentifier().getName();
+ computeResultsForExpression(e.getFunctionCall().getOperands().get(0),
columnNames, columnTypes, row);
+ columnNames.set(columnNames.size() - 1, columnName);
+ } else {
+ throw new IllegalStateException(
+ "No able to compute results for function - " +
e.getFunctionCall().getOperator());
+ }
+ }
+ }
+
+ private void computeResultsForLiteral(Literal literal, List<String>
columnNames, List<DataSchema.ColumnDataType> columnTypes,
+ List<Object> row) {
+ Object fieldValue = literal.getFieldValue();
+ columnNames.add(fieldValue.toString());
+ switch (literal.getSetField()) {
+ case SHORT_VALUE:
+ columnTypes.add(DataSchema.ColumnDataType.INT);
+ row.add((int) literal.getShortValue());
+ break;
+ case INT_VALUE:
+ columnTypes.add(DataSchema.ColumnDataType.INT);
+ row.add(literal.getIntValue());
+ break;
+ case LONG_VALUE:
+ columnTypes.add(DataSchema.ColumnDataType.LONG);
+ row.add(literal.getLongValue());
+ break;
+ case DOUBLE_VALUE:
+ columnTypes.add(DataSchema.ColumnDataType.DOUBLE);
+ row.add(literal.getDoubleValue());
+ break;
+ case BOOL_VALUE:
+ columnTypes.add(DataSchema.ColumnDataType.STRING);
+ row.add(new Boolean(literal.getBoolValue()).toString());
+ break;
+ case STRING_VALUE:
+ columnTypes.add(DataSchema.ColumnDataType.STRING);
+ System.out.println("literal.getStringValue() = " +
literal.getStringValue());
+ row.add(literal.getStringValue());
+ break;
+ case BINARY_VALUE:
+ columnTypes.add(DataSchema.ColumnDataType.BYTES);
+ row.add(BytesUtils.toHexString(literal.getBinaryValue()));
+ break;
+ case BYTE_VALUE:
Review comment:
BYTE should be modeled as INT
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -435,6 +455,116 @@ static void handleQueryLimitOverride(BrokerRequest
brokerRequest, int queryLimit
}
}
+ /**
+ * Check if a SQL parsed BrokerRequest is a literal only query.
+ * @param brokerRequest
+ * @return true if this query selects only Literals
+ *
+ */
+ @VisibleForTesting
+ static boolean isLiteralOnlyQuery(BrokerRequest brokerRequest) {
+ if (brokerRequest.getPinotQuery() != null) {
+ for (Expression e: brokerRequest.getPinotQuery().getSelectList()) {
+ if (!CalciteSqlParser.isLiteralOnlyExpression(e)) {
+ return false;
+ }
+ }
+ return true;
+ }
+ return false;
+ }
+
+ /**
+ * Compute BrokerResponse for literal only query.
+ *
+ * @param brokerRequest
+ * @param compilationStartTimeNs
+ * @param requestStatistics
+ * @return BrokerResponse
+ */
+ private BrokerResponse processLiteralOnlyBrokerRequest(BrokerRequest
brokerRequest, long compilationStartTimeNs,
+ RequestStatistics requestStatistics)
+ throws IllegalStateException {
+ System.out.println("brokerRequest = " + brokerRequest.toString());
Review comment:
Remove this
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -184,6 +192,18 @@ public BrokerResponse handleRequest(JsonNode request,
@Nullable RequesterIdentit
requestStatistics.setErrorCode(QueryException.PQL_PARSING_ERROR_CODE);
return new
BrokerResponseNative(QueryException.getException(QueryException.PQL_PARSING_ERROR,
e));
}
+ if (isLiteralOnlyQuery(brokerRequest)) {
+ LOGGER.info("Request {} contains only Literal, skipping server query:
{}", requestId, query);
+ try {
+ BrokerResponse brokerResponse =
+ processLiteralOnlyBrokerRequest(brokerRequest,
compilationStartTimeNs, requestStatistics);
+ return brokerResponse;
+ } catch (IllegalStateException e) {
Review comment:
Why only catching `IllegalStateException`?
##########
File path:
pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -278,7 +278,9 @@ private static PinotQuery
compileCalciteSqlToPinotQuery(String sql) {
pinotQuery.setOffset(Integer.valueOf(((SqlNumericLiteral)
selectSqlNode.getOffset()).toValue()));
}
DataSource dataSource = new DataSource();
- dataSource.setTableName(selectSqlNode.getFrom().toString());
+ if (selectSqlNode.getFrom() != null) {
Review comment:
Can this be null? We shouldn't allow query without table name right?
I would suggest always putting a dummy table name in the test so that
production code won't accidentally miss the table name
##########
File path:
pinot-common/src/main/java/org/apache/pinot/pql/parsers/PinotQuery2BrokerRequestConverter.java
##########
@@ -128,7 +128,7 @@ private void convertSelectList(PinotQuery pinotQuery,
BrokerRequest brokerReques
if (selection == null) {
selection = new Selection();
}
-
selection.addToSelectionColumns(expression.getLiteral().getFieldValue().toString());
+ selection.addToSelectionColumns(String.format("'%s'",
expression.getLiteral().getFieldValue().toString()));
Review comment:
For performance:
```suggestion
selection.addToSelectionColumns('\'' +
expression.getLiteral().getFieldValue().toString() + '\'');
```
----------------------------------------------------------------
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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
