Copilot commented on code in PR #7530:
URL: https://github.com/apache/kyuubi/pull/7530#discussion_r3892425641
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/operation/KyuubiOperation.scala:
##########
@@ -35,7 +35,15 @@ import org.apache.kyuubi.shaded.thrift.TException
import org.apache.kyuubi.shaded.thrift.transport.TTransportException
import org.apache.kyuubi.util.ThriftUtils
-abstract class KyuubiOperation(session: Session) extends
AbstractOperation(session) {
+// The handle is injected as a constructor paramaccessor `override protected
val handle` (mirroring
Review Comment:
Typo in the class comment: “paramaccessor” should be “parameter accessor”
for clarity.
##########
extensions/server/kyuubi-server-plugin/src/main/java/org/apache/kyuubi/plugin/StatementInterceptResult.java:
##########
@@ -0,0 +1,128 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kyuubi.plugin;
+
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * The decision returned by a {@link StatementInterceptor} for a single
statement: proceed with the
+ * current statement, rewrite it for the next interceptor and the engine, or
reject it.
+ */
+public final class StatementInterceptResult {
+
+ public enum Action {
+ PROCEED,
+ REWRITE,
+ REJECT
+ }
+
+ private final Action action;
+ private final String statement; // the rewritten statement when action is
REWRITE
+ private final String message; // the rejection reason when action is REJECT
+ private final Map<String, String> confOverlay;
+
+ private StatementInterceptResult(
+ Action action, String statement, String message, Map<String, String>
confOverlay) {
+ this.action = action;
+ this.statement = statement;
+ this.message = message;
+ this.confOverlay = immutableConfOverlay(confOverlay);
+ }
+
+ /** Keep the current statement and pass it to the next interceptor. */
+ public static StatementInterceptResult proceed() {
+ return proceed(Collections.emptyMap());
+ }
+
+ /**
+ * Keep the current statement and apply {@code confOverlay} to this
statement only. The entries
+ * are passed to subsequent interceptors and ultimately to the engine
operation.
+ */
+ public static StatementInterceptResult proceed(Map<String, String>
confOverlay) {
+ return new StatementInterceptResult(Action.PROCEED, null, null,
confOverlay);
+ }
+
+ /**
+ * Replace the current statement with {@code statement}. It is passed to the
next interceptor and
+ * ultimately to the engine. {@code statement} must not be null or blank.
+ */
+ public static StatementInterceptResult rewrite(String statement) {
+ return rewrite(statement, Collections.emptyMap());
+ }
+
+ /**
+ * Replace the current statement with {@code statement} and apply {@code
confOverlay} to this
+ * statement only. Both are passed to subsequent interceptors and ultimately
to the engine
+ * operation. {@code statement} must not be null or blank.
+ */
+ public static StatementInterceptResult rewrite(
+ String statement, Map<String, String> confOverlay) {
+ return new StatementInterceptResult(
+ Action.REWRITE, requireNonBlank(statement, "statement"), null,
confOverlay);
+ }
+
+ /**
+ * Reject the statement immediately; the chain stops and the client receives
an error. {@code
+ * message} must not be null or blank.
+ */
+ public static StatementInterceptResult reject(String message) {
+ return new StatementInterceptResult(
+ Action.REJECT, null, requireNonBlank(message, "message"),
Collections.emptyMap());
+ }
+
+ public Action action() {
+ return action;
+ }
+
+ public String statement() {
+ return statement;
+ }
+
+ public String message() {
+ return message;
+ }
+
+ /** The immutable per-statement configuration updates returned by this
interceptor. */
+ public Map<String, String> confOverlay() {
+ return confOverlay;
+ }
+
+ private static String requireNonBlank(String value, String name) {
+ if (value == null || value.trim().isEmpty()) {
+ throw new IllegalArgumentException(name + " must not be null or blank");
+ }
+ return value;
+ }
+
+ private static Map<String, String> immutableConfOverlay(Map<String, String>
confOverlay) {
+ if (confOverlay == null) {
+ throw new IllegalArgumentException("confOverlay must not be null");
+ }
+ Map<String, String> copy = new HashMap<>();
+ confOverlay.forEach(
+ (key, value) -> {
+ if (key == null || value == null) {
+ throw new IllegalArgumentException("confOverlay must not contain
null keys or values");
+ }
+ copy.put(key, value);
+ });
+ return Collections.unmodifiableMap(copy);
+ }
Review Comment:
`immutableConfOverlay` always allocates a new `HashMap` (and wraps it) even
when the input overlay is empty. Since `proceed()`/`rewrite()` commonly pass
`Collections.emptyMap()`, this adds avoidable per-statement allocation
overhead. Add an empty fast-path and pre-size the map for non-empty overlays.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
