Jackie-Jiang commented on code in PR #9239:
URL: https://github.com/apache/pinot/pull/9239#discussion_r955278255
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/request/RequestUtils.java:
##########
@@ -30,13 +33,67 @@
import org.apache.pinot.common.request.Literal;
import org.apache.pinot.common.request.PinotQuery;
import org.apache.pinot.spi.utils.BytesUtils;
+import org.apache.pinot.spi.utils.CommonConstants;
import org.apache.pinot.sql.FilterKind;
+import org.apache.pinot.sql.parsers.CalciteSqlParser;
+import org.apache.pinot.sql.parsers.SqlCompilationException;
+import org.apache.pinot.sql.parsers.SqlNodeAndOptions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public class RequestUtils {
+ private static final Logger LOGGER =
LoggerFactory.getLogger(RequestUtils.class);
+
private RequestUtils() {
}
+ public static SqlNodeAndOptions parseQuery(String query, JsonNode request)
+ throws SqlCompilationException {
+ long parserStartTimeNs = System.nanoTime();
+ SqlNodeAndOptions sqlNodeAndOptions =
CalciteSqlParser.compileToSqlNodeAndOptions(query);
+ setOptions(sqlNodeAndOptions, request);
+ sqlNodeAndOptions.setParseTimeNs(System.nanoTime() - parserStartTimeNs);
+ return sqlNodeAndOptions;
+ }
+
+ /**
+ * Sets extra options for the given query.
+ */
+ @VisibleForTesting
+ public static void setOptions(SqlNodeAndOptions sqlNodeAndOptions, JsonNode
jsonRequest) {
+ Map<String, String> queryOptions = new HashMap<>();
+ if (jsonRequest.has(CommonConstants.Broker.Request.DEBUG_OPTIONS)) {
+ Map<String, String> debugOptions =
RequestUtils.getOptionsFromJson(jsonRequest,
+ CommonConstants.Broker.Request.DEBUG_OPTIONS);
+ // TODO: remove debug options after releasing 0.11.0.
+ if (!debugOptions.isEmpty()) {
+ // NOTE: Debug options are deprecated. Put all debug options into
query options for backward compatibility.
Review Comment:
This is not enough. During version upgrade, server doesn't know the debug
options can be read from the query options
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java:
##########
@@ -137,7 +137,8 @@ public static SqlNodeAndOptions
compileToSqlNodeAndOptions(String sql)
}
}
- public static SqlNodeAndOptions extractSqlNodeAndOptions(SqlNodeList
sqlNodeList) {
+ @VisibleForTesting
+ static SqlNodeAndOptions extractSqlNodeAndOptions(String sql, SqlNodeList
sqlNodeList) {
Review Comment:
The new added `sql` seems unused?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -133,16 +133,12 @@ private BrokerResponseNative handleRequest(long
requestId, String query,
throws Exception {
LOGGER.debug("SQL query for request {}: {}", requestId, query);
+ // Parse the request
+ sqlNodeAndOptions = sqlNodeAndOptions != null ? sqlNodeAndOptions :
RequestUtils.parseQuery(query, request);
Review Comment:
This needs to be moved into the try-catch
--
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
