fx19880617 commented on a change in pull request #5513:
URL: https://github.com/apache/incubator-pinot/pull/5513#discussion_r436894620
##########
File path:
pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -676,14 +678,25 @@ protected static Expression
invokeCompileTimeFunctionExpression(Expression funcE
try {
FunctionInvoker invoker = new FunctionInvoker(functionInfo);
Object result = invoker.process(arguments);
- if (result instanceof String) {
- result = String.format("'%s'", result);
- }
return RequestUtils.getLiteralExpression(result);
} catch (Exception e) {
throw new SqlCompilationException(new
IllegalArgumentException("Unsupported function - " + funcName, e));
}
}
return funcExpr;
}
+
+ public static boolean isLiteralOnlyExpression(Expression e) {
+ if (e.getType() == ExpressionType.LITERAL) {
+ return true;
+ }
+ if (e.getType() == ExpressionType.FUNCTION) {
+ Function functionCall = e.getFunctionCall();
+ if (functionCall.getOperator().equalsIgnoreCase(SqlKind.AS.toString())) {
Review comment:
Yes, the CalciteSQLParser already evaluated the transform function with
literals and replace it with literal.
So the only left over is to handle here is `Literal` and `Function` `AS`.
##########
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) {
Review comment:
sure
##########
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) {
Review comment:
added
##########
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:
We only catch the behavior that process literal functions failure.
##########
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:
From sql standard, it's ok if it's null, the query should not fail if it
contains no literals.
##########
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:
Make sense!
##########
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:
Changed this log to debug level.
For the code from `376-405`, I feel it makes too many assumptions which
current broker query doesn't honor. I prefer to keep them separately.
##########
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:
Changed this log to debug level.
For the code from `376-405`, I feel it makes too many assumptions which
current broker query doesn't honor(e.g. table name is not a must here). I
prefer to keep them separately.
----------------------------------------------------------------
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
