github-actions[bot] commented on code in PR #67966:
URL: https://github.com/apache/doris/pull/67966#discussion_r4010615498
##########
fe/fe-core/src/main/java/org/apache/doris/arrowflight/DorisFlightSqlProducer.java:
##########
@@ -632,20 +638,72 @@ public void
getStreamCrossReference(CommandGetCrossReference command, CallContex
throw
CallStatus.UNIMPLEMENTED.withDescription("getStreamCrossReference
unimplemented").toRuntimeException();
}
+ /**
+ * Sets session options: the current catalog, the current database and
session variables, see
+ * {@link FlightSessionOptions}. The ADBC Flight SQL driver sends these
for the connection's
+ * {@code adbc.connection.catalog} / {@code adbc.connection.db_schema} and
for its
+ * {@code adbc.flight.sql.session.option.*} options; the Flight SQL JDBC
driver for its
+ * {@code catalog} property. Each option is set on its own and answered on
its own: the result
+ * names the ones that could not be set and why, and the action itself
only fails when the
+ * session cannot be reached.
+ */
+ @Override
+ public void setSessionOptions(final SetSessionOptionsRequest request,
final CallContext context,
+ final StreamListener<SetSessionOptionsResult> listener) {
+ try {
+ ConnectContext connectContext =
flightSessionsManager.getConnectContext(context.peerIdentity());
+ Map<String, SetSessionOptionsResult.Error> errors =
FlightProtocolAdapter.of(connectContext)
Review Comment:
[P1] Bound prior-request resources independently of option traffic
Neither new option action calls beginRequest, so the preceding request's
cached FE result/endpoints remain and any deferred external-query executor
stays registered. Accepted SET already refreshes startTime, and fixing GET's
missing activity refresh would make GET do so too; periodic option actions can
therefore retain old buffers plus the coordinator, query registration, and
query-queue slot indefinitely unless resource age is separate. Please either
release prior-request state at a safe boundary or age/reap it independently of
option activity, and test both actions after cached-local and deferred queries.
##########
fe/fe-core/src/main/java/org/apache/doris/arrowflight/DorisFlightSqlProducer.java:
##########
@@ -632,20 +638,72 @@ public void
getStreamCrossReference(CommandGetCrossReference command, CallContex
throw
CallStatus.UNIMPLEMENTED.withDescription("getStreamCrossReference
unimplemented").toRuntimeException();
}
+ /**
+ * Sets session options: the current catalog, the current database and
session variables, see
+ * {@link FlightSessionOptions}. The ADBC Flight SQL driver sends these
for the connection's
+ * {@code adbc.connection.catalog} / {@code adbc.connection.db_schema} and
for its
+ * {@code adbc.flight.sql.session.option.*} options; the Flight SQL JDBC
driver for its
+ * {@code catalog} property. Each option is set on its own and answered on
its own: the result
+ * names the ones that could not be set and why, and the action itself
only fails when the
+ * session cannot be reached.
+ */
+ @Override
+ public void setSessionOptions(final SetSessionOptionsRequest request,
final CallContext context,
Review Comment:
[P2] Advertise the newly supported session actions
This class still inherits Arrow Java 19's FlightSqlProducer.listActions,
whose FLIGHT_SQL_ACTIONS list omits SetSessionOptions, GetSessionOptions, and
CloseSession even though this PR implements all three FlightConstants actions.
Capability-discovering clients are therefore told these actions are unavailable
while direct calls happen to work. Please override listActions to emit the
inherited SQL actions plus these three constants, and add a discovery assertion.
##########
fe/fe-core/src/main/java/org/apache/doris/arrowflight/DorisFlightSqlProducer.java:
##########
@@ -632,20 +638,72 @@ public void
getStreamCrossReference(CommandGetCrossReference command, CallContex
throw
CallStatus.UNIMPLEMENTED.withDescription("getStreamCrossReference
unimplemented").toRuntimeException();
}
+ /**
+ * Sets session options: the current catalog, the current database and
session variables, see
+ * {@link FlightSessionOptions}. The ADBC Flight SQL driver sends these
for the connection's
+ * {@code adbc.connection.catalog} / {@code adbc.connection.db_schema} and
for its
+ * {@code adbc.flight.sql.session.option.*} options; the Flight SQL JDBC
driver for its
+ * {@code catalog} property. Each option is set on its own and answered on
its own: the result
+ * names the ones that could not be set and why, and the action itself
only fails when the
+ * session cannot be reached.
+ */
+ @Override
+ public void setSessionOptions(final SetSessionOptionsRequest request,
final CallContext context,
+ final StreamListener<SetSessionOptionsResult> listener) {
+ try {
+ ConnectContext connectContext =
flightSessionsManager.getConnectContext(context.peerIdentity());
+ Map<String, SetSessionOptionsResult.Error> errors =
FlightProtocolAdapter.of(connectContext)
+ .callCommand(connectContext,
+ () -> FlightSessionOptions.set(connectContext,
request.getSessionOptions()));
+ listener.onNext(new SetSessionOptionsResult(errors));
+ listener.onCompleted();
+ } catch (FlightRuntimeException e) {
+ // Same as in getFlightInfoStatement: keep the status the
session's command lock chose.
+ LOG.error("set session options failed", e);
+ listener.onError(e);
+ } catch (Throwable e) {
+ String errMsg = "set session options failed, " + e.getMessage();
+ LOG.error(errMsg, e);
+
listener.onError(CallStatus.INTERNAL.withDescription(errMsg).withCause(e).toRuntimeException());
+ }
+ }
+
+ /** The session's options, see {@link FlightSessionOptions#get}. */
+ @Override
+ public void getSessionOptions(final GetSessionOptionsRequest request,
final CallContext context,
+ final StreamListener<GetSessionOptionsResult> listener) {
+ try {
+ ConnectContext connectContext =
flightSessionsManager.getConnectContext(context.peerIdentity());
+ Map<String, SessionOptionValue> options =
FlightProtocolAdapter.of(connectContext)
Review Comment:
[P1] Refresh session activity for option-only commands
This GET runs only through callCommand, which does not set COM_QUERY or
refresh ConnectContext.startTime; empty or wholly rejected SET requests have
the same path because they never reach runStatement. Therefore a client can
keep successfully issuing these session actions and still be reaped at
wait_timeout based on the context's old timestamp. Once the context is
unregistered, the same authenticated token cannot recreate it because
createdSession is already true. Please give every session-option action the
normal active/sleep activity lifecycle and cover repeated GET plus
empty/invalid-only SET across the timeout boundary.
##########
fe/fe-core/src/main/java/org/apache/doris/arrowflight/FlightSessionOptions.java:
##########
@@ -0,0 +1,282 @@
+// 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.doris.arrowflight;
+
+import org.apache.doris.analysis.SetType;
+import org.apache.doris.common.ErrorCode;
+import org.apache.doris.nereids.util.SqlLiteralUtils;
+import org.apache.doris.qe.ConnectContext;
+import org.apache.doris.qe.QueryState.MysqlStateType;
+import org.apache.doris.qe.SqlModeHelper;
+import org.apache.doris.qe.VarAttrDef;
+import org.apache.doris.qe.VariableMgr;
+import org.apache.doris.qe.VariableMgr.VarContext;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.arrow.flight.SessionOptionValue;
+import org.apache.arrow.flight.SessionOptionValueFactory;
+import org.apache.arrow.flight.SessionOptionValueVisitor;
+import org.apache.arrow.flight.SetSessionOptionsResult;
+import org.apache.arrow.flight.SetSessionOptionsResult.ErrorValue;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.regex.Pattern;
+
+/**
+ * The session options of an Arrow Flight SQL session, as the
SetSessionOptions and GetSessionOptions
+ * actions see them.
+ *
+ * <p>An option stands for the statement of the session that sets it: {@code
catalog} is the current
+ * catalog ({@code SWITCH}), {@code schema} the current database ({@code
USE}), and any other name is
+ * the session variable of that name ({@code SET SESSION}). Those two names
are what the ADBC Flight
+ * SQL driver puts on the wire for {@code adbc.connection.catalog} and
+ * {@code adbc.connection.db_schema}, and what the Flight SQL JDBC driver
sends for its
+ * {@code catalog} property; every other option name the drivers pass through
as given. Setting an
+ * option runs its statement as a command of the session, so it is checked,
audited and takes effect
+ * exactly as if the client had sent the statement. Reading the options back
gives what
+ * {@code SHOW VARIABLES} shows, every value as a string: the one
representation {@code SET} accepts
+ * back, whatever the Java type of the variable behind it.
+ *
+ * <p>The result of setting an option is one of the three {@link ErrorValue}s
per name and nothing
+ * else, so the reason a value was refused only reaches the frontend log.
+ */
+public final class FlightSessionOptions {
+ private static final Logger LOG =
LogManager.getLogger(FlightSessionOptions.class);
+
+ /** The current catalog; set with {@code SWITCH}. */
+ public static final String CATALOG = "catalog";
+ /** The current database; set with {@code USE}. */
+ public static final String SCHEMA = "schema";
+
+ // What a session variable is called in SET: VariableMgr looks the name up
case-insensitively,
+ // and only a name made of these characters is one the parser reads as the
variable's identifier.
+ private static final Pattern VARIABLE_NAME =
Pattern.compile("[A-Za-z_][A-Za-z0-9_]*");
+
+ private FlightSessionOptions() {
+ }
+
+ /**
+ * Sets the options of one request, each on its own, and returns the error
of every option that
+ * could not be set; an option absent from the result was set. {@code
catalog} goes first and
+ * {@code schema} second, since {@code USE} names a database of the
current catalog; the others
+ * follow in name order, so that a request applies the same way every
time. Runs as a command of
+ * the session, under its command lock.
+ */
+ public static Map<String, SetSessionOptionsResult.Error>
set(ConnectContext ctx,
+ Map<String, SessionOptionValue> options) {
+ List<String> names = new ArrayList<>(options.keySet());
+ Collections.sort(names);
+ names.remove(SCHEMA);
+ names.remove(CATALOG);
+ if (options.containsKey(SCHEMA)) {
+ names.add(0, SCHEMA);
+ }
+ if (options.containsKey(CATALOG)) {
+ names.add(0, CATALOG);
+ }
+ Map<String, SetSessionOptionsResult.Error> errors = new
LinkedHashMap<>();
+ for (String name : names) {
+ ErrorValue error = setOne(ctx, name, options.get(name));
+ if (error != null) {
+ errors.put(name, new SetSessionOptionsResult.Error(error));
+ }
+ }
+ return errors;
+ }
+
+ /** Sets one option and returns why it could not be, or null when it was
set. */
+ @VisibleForTesting
+ static ErrorValue setOne(ConnectContext ctx, String name,
SessionOptionValue value) {
+ if (CATALOG.equals(name)) {
+ return switchCatalog(ctx, value);
+ }
+ if (SCHEMA.equals(name)) {
+ return useDatabase(ctx, value);
+ }
+ return setVariable(ctx, name, value);
+ }
+
+ /**
+ * The options of the session: the current catalog, the current database
(an empty string when
+ * none has been chosen) and every session variable {@code SHOW VARIABLES}
would list, in its text.
+ */
+ public static Map<String, SessionOptionValue> get(ConnectContext ctx) {
+ Map<String, SessionOptionValue> options = new LinkedHashMap<>();
+ options.put(CATALOG,
SessionOptionValueFactory.makeSessionOptionValue(ctx.getDefaultCatalog()));
+ String database = ctx.getDatabase();
+ options.put(SCHEMA,
SessionOptionValueFactory.makeSessionOptionValue(database == null ? "" :
database));
+ for (List<String> row : VariableMgr.dump(SetType.SESSION,
ctx.getSessionVariable(), null)) {
+ // A row is name, value, default value, changed.
+ options.put(row.get(0),
SessionOptionValueFactory.makeSessionOptionValue(row.get(1)));
+ }
+ return options;
+ }
+
+ private static ErrorValue switchCatalog(ConnectContext ctx,
SessionOptionValue value) {
+ String catalog = value.acceptVisitor(STRING_VALUE);
+ if (catalog == null || catalog.isEmpty()) {
+ return ErrorValue.INVALID_VALUE;
+ }
+ return runStatement(ctx, CATALOG, "SWITCH " +
quoteIdentifier(catalog), ErrorCode.ERR_UNKNOWN_CATALOG);
+ }
+
+ private static ErrorValue useDatabase(ConnectContext ctx,
SessionOptionValue value) {
+ String database = value.acceptVisitor(STRING_VALUE);
+ if (database == null || database.isEmpty()) {
+ return ErrorValue.INVALID_VALUE;
+ }
+ return runStatement(ctx, SCHEMA, "USE " + quoteIdentifier(database),
ErrorCode.ERR_BAD_DB_ERROR);
+ }
+
+ private static ErrorValue setVariable(ConnectContext ctx, String name,
SessionOptionValue value) {
+ // Only a session variable is a session option. A name SET quietly
ignores for the sake of
+ // MySQL clients (a removed variable, the MySQL compatibility
whitelist) is no variable of
+ // this session either: setting it would set nothing, and reading the
options back would not
+ // list it.
+ if (!VARIABLE_NAME.matcher(name).matches()) {
+ return ErrorValue.INVALID_NAME;
+ }
+ VarContext varCtx = VariableMgr.getVarContext(name);
Review Comment:
[P1] Restrict SET to GetSessionOptions' key namespace
getVarContext accepts names that GET never exposes unchanged: retained
enable_nereids_dml (REMOVED) and enable_local_exchange (INVISIBLE) are omitted
entirely; Query_Timeout comes back only as query_timeout; and bare
enable_shared_scan comes back as experimental_enable_shared_scan. SET returns
no per-name error in each case, but the ADBC getter performs an exact lookup of
the key the client requested and therefore reports it missing. Please derive
accepted option keys from the same visible/display namespace as GET (or
canonicalize both sides consistently), and cover retained hidden variables plus
case/prefix spellings; use_v2_rollup only covers a context removed entirely.
##########
fe/fe-core/src/main/java/org/apache/doris/arrowflight/FlightSessionOptions.java:
##########
@@ -0,0 +1,282 @@
+// 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.doris.arrowflight;
+
+import org.apache.doris.analysis.SetType;
+import org.apache.doris.common.ErrorCode;
+import org.apache.doris.nereids.util.SqlLiteralUtils;
+import org.apache.doris.qe.ConnectContext;
+import org.apache.doris.qe.QueryState.MysqlStateType;
+import org.apache.doris.qe.SqlModeHelper;
+import org.apache.doris.qe.VarAttrDef;
+import org.apache.doris.qe.VariableMgr;
+import org.apache.doris.qe.VariableMgr.VarContext;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.arrow.flight.SessionOptionValue;
+import org.apache.arrow.flight.SessionOptionValueFactory;
+import org.apache.arrow.flight.SessionOptionValueVisitor;
+import org.apache.arrow.flight.SetSessionOptionsResult;
+import org.apache.arrow.flight.SetSessionOptionsResult.ErrorValue;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.regex.Pattern;
+
+/**
+ * The session options of an Arrow Flight SQL session, as the
SetSessionOptions and GetSessionOptions
+ * actions see them.
+ *
+ * <p>An option stands for the statement of the session that sets it: {@code
catalog} is the current
+ * catalog ({@code SWITCH}), {@code schema} the current database ({@code
USE}), and any other name is
+ * the session variable of that name ({@code SET SESSION}). Those two names
are what the ADBC Flight
+ * SQL driver puts on the wire for {@code adbc.connection.catalog} and
+ * {@code adbc.connection.db_schema}, and what the Flight SQL JDBC driver
sends for its
+ * {@code catalog} property; every other option name the drivers pass through
as given. Setting an
+ * option runs its statement as a command of the session, so it is checked,
audited and takes effect
+ * exactly as if the client had sent the statement. Reading the options back
gives what
+ * {@code SHOW VARIABLES} shows, every value as a string: the one
representation {@code SET} accepts
+ * back, whatever the Java type of the variable behind it.
+ *
+ * <p>The result of setting an option is one of the three {@link ErrorValue}s
per name and nothing
+ * else, so the reason a value was refused only reaches the frontend log.
+ */
+public final class FlightSessionOptions {
+ private static final Logger LOG =
LogManager.getLogger(FlightSessionOptions.class);
+
+ /** The current catalog; set with {@code SWITCH}. */
+ public static final String CATALOG = "catalog";
+ /** The current database; set with {@code USE}. */
+ public static final String SCHEMA = "schema";
+
+ // What a session variable is called in SET: VariableMgr looks the name up
case-insensitively,
+ // and only a name made of these characters is one the parser reads as the
variable's identifier.
+ private static final Pattern VARIABLE_NAME =
Pattern.compile("[A-Za-z_][A-Za-z0-9_]*");
+
+ private FlightSessionOptions() {
+ }
+
+ /**
+ * Sets the options of one request, each on its own, and returns the error
of every option that
+ * could not be set; an option absent from the result was set. {@code
catalog} goes first and
+ * {@code schema} second, since {@code USE} names a database of the
current catalog; the others
+ * follow in name order, so that a request applies the same way every
time. Runs as a command of
+ * the session, under its command lock.
+ */
+ public static Map<String, SetSessionOptionsResult.Error>
set(ConnectContext ctx,
+ Map<String, SessionOptionValue> options) {
+ List<String> names = new ArrayList<>(options.keySet());
+ Collections.sort(names);
+ names.remove(SCHEMA);
+ names.remove(CATALOG);
+ if (options.containsKey(SCHEMA)) {
+ names.add(0, SCHEMA);
+ }
+ if (options.containsKey(CATALOG)) {
+ names.add(0, CATALOG);
+ }
+ Map<String, SetSessionOptionsResult.Error> errors = new
LinkedHashMap<>();
+ for (String name : names) {
+ ErrorValue error = setOne(ctx, name, options.get(name));
+ if (error != null) {
+ errors.put(name, new SetSessionOptionsResult.Error(error));
+ }
+ }
+ return errors;
+ }
+
+ /** Sets one option and returns why it could not be, or null when it was
set. */
+ @VisibleForTesting
+ static ErrorValue setOne(ConnectContext ctx, String name,
SessionOptionValue value) {
+ if (CATALOG.equals(name)) {
+ return switchCatalog(ctx, value);
+ }
+ if (SCHEMA.equals(name)) {
+ return useDatabase(ctx, value);
+ }
+ return setVariable(ctx, name, value);
+ }
+
+ /**
+ * The options of the session: the current catalog, the current database
(an empty string when
+ * none has been chosen) and every session variable {@code SHOW VARIABLES}
would list, in its text.
+ */
+ public static Map<String, SessionOptionValue> get(ConnectContext ctx) {
+ Map<String, SessionOptionValue> options = new LinkedHashMap<>();
+ options.put(CATALOG,
SessionOptionValueFactory.makeSessionOptionValue(ctx.getDefaultCatalog()));
+ String database = ctx.getDatabase();
+ options.put(SCHEMA,
SessionOptionValueFactory.makeSessionOptionValue(database == null ? "" :
database));
+ for (List<String> row : VariableMgr.dump(SetType.SESSION,
ctx.getSessionVariable(), null)) {
+ // A row is name, value, default value, changed.
+ options.put(row.get(0),
SessionOptionValueFactory.makeSessionOptionValue(row.get(1)));
+ }
+ return options;
+ }
+
+ private static ErrorValue switchCatalog(ConnectContext ctx,
SessionOptionValue value) {
+ String catalog = value.acceptVisitor(STRING_VALUE);
+ if (catalog == null || catalog.isEmpty()) {
+ return ErrorValue.INVALID_VALUE;
+ }
+ return runStatement(ctx, CATALOG, "SWITCH " +
quoteIdentifier(catalog), ErrorCode.ERR_UNKNOWN_CATALOG);
+ }
+
+ private static ErrorValue useDatabase(ConnectContext ctx,
SessionOptionValue value) {
+ String database = value.acceptVisitor(STRING_VALUE);
+ if (database == null || database.isEmpty()) {
+ return ErrorValue.INVALID_VALUE;
+ }
+ return runStatement(ctx, SCHEMA, "USE " + quoteIdentifier(database),
ErrorCode.ERR_BAD_DB_ERROR);
+ }
+
+ private static ErrorValue setVariable(ConnectContext ctx, String name,
SessionOptionValue value) {
+ // Only a session variable is a session option. A name SET quietly
ignores for the sake of
+ // MySQL clients (a removed variable, the MySQL compatibility
whitelist) is no variable of
+ // this session either: setting it would set nothing, and reading the
options back would not
+ // list it.
+ if (!VARIABLE_NAME.matcher(name).matches()) {
+ return ErrorValue.INVALID_NAME;
+ }
+ VarContext varCtx = VariableMgr.getVarContext(name);
+ if (varCtx == null) {
+ return ErrorValue.INVALID_NAME;
+ }
+ // SET SESSION refuses a read-only variable and one that exists once
per frontend rather than
+ // per session (SET GLOBAL sets that one); neither can be set as a
session option.
+ if ((varCtx.getFlag() & (VarAttrDef.READ_ONLY | VarAttrDef.GLOBAL)) !=
0) {
+ return ErrorValue.ERROR;
+ }
+ // A string is quoted for the session's sql_mode, the mode the
statement is then parsed under.
+ String literal =
SqlModeHelper.withSqlMode(ctx.getSessionVariable().getSqlMode(),
+ () -> value.acceptVisitor(SET_LITERAL));
+ if (literal == null) {
+ return ErrorValue.INVALID_VALUE;
+ }
+ // The name and the scope were checked above, so what is left for SET
to refuse is the value:
+ // its type, its range, or what a variable's own checker makes of it.
+ return runStatement(ctx, name, "SET SESSION " + name + " = " +
literal, null);
+ }
+
+ /**
+ * Runs the statement an option stands for as a command of the session. A
failure is
+ * {@code INVALID_VALUE} when the value was refused and {@code ERROR}
otherwise. For SWITCH and
+ * USE the two are told apart by the error code the statement left on the
session: an unknown
+ * catalog or database is written there by the command itself, while a
statement that fails in
+ * validation, e.g. for lack of privilege, leaves the executor's generic
code. For SET every
+ * failure is the value's (see {@link #setVariable}), so {@code
refusedValueCode} is null there.
+ */
+ private static ErrorValue runStatement(ConnectContext ctx, String option,
String statement,
+ ErrorCode refusedValueCode) {
+ try (FlightSqlConnectProcessor processor = new
FlightSqlConnectProcessor(ctx)) {
+ processor.handleQuery(statement);
+ if (ctx.getState().getStateType() != MysqlStateType.ERR) {
+ return null;
+ }
+ LOG.warn("session option {} of Arrow Flight SQL connection {}
could not be set, statement: {}, "
+ + "error code: {}, error message: {}", option,
ctx.getConnectionId(), statement,
+ ctx.getState().getErrorCode(),
ctx.getState().getErrorMessage());
+ if (refusedValueCode == null || refusedValueCode ==
ctx.getState().getErrorCode()) {
Review Comment:
[P2] Preserve non-value SET failures as ERROR
refusedValueCode is always null for variables, so every ERR from handleQuery
is returned as INVALID_VALUE even when the name and value are valid. For
example, a non-root cloud session in OVERDUE state rejects SET SESSION
query_timeout = 10 before execution; transaction mode and block_sql_ast_names
can similarly reject SetOptionsCommand. Those are server/session failures, for
which the Flight enum provides ERROR, not invalid option values. Please
classify only known conversion/checker failures as INVALID_VALUE and return
ERROR for the other execution paths, with a non-value failure test.
##########
fe/fe-core/src/main/java/org/apache/doris/arrowflight/FlightSessionOptions.java:
##########
@@ -0,0 +1,282 @@
+// 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.doris.arrowflight;
+
+import org.apache.doris.analysis.SetType;
+import org.apache.doris.common.ErrorCode;
+import org.apache.doris.nereids.util.SqlLiteralUtils;
+import org.apache.doris.qe.ConnectContext;
+import org.apache.doris.qe.QueryState.MysqlStateType;
+import org.apache.doris.qe.SqlModeHelper;
+import org.apache.doris.qe.VarAttrDef;
+import org.apache.doris.qe.VariableMgr;
+import org.apache.doris.qe.VariableMgr.VarContext;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.arrow.flight.SessionOptionValue;
+import org.apache.arrow.flight.SessionOptionValueFactory;
+import org.apache.arrow.flight.SessionOptionValueVisitor;
+import org.apache.arrow.flight.SetSessionOptionsResult;
+import org.apache.arrow.flight.SetSessionOptionsResult.ErrorValue;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.regex.Pattern;
+
+/**
+ * The session options of an Arrow Flight SQL session, as the
SetSessionOptions and GetSessionOptions
+ * actions see them.
+ *
+ * <p>An option stands for the statement of the session that sets it: {@code
catalog} is the current
+ * catalog ({@code SWITCH}), {@code schema} the current database ({@code
USE}), and any other name is
+ * the session variable of that name ({@code SET SESSION}). Those two names
are what the ADBC Flight
+ * SQL driver puts on the wire for {@code adbc.connection.catalog} and
+ * {@code adbc.connection.db_schema}, and what the Flight SQL JDBC driver
sends for its
+ * {@code catalog} property; every other option name the drivers pass through
as given. Setting an
+ * option runs its statement as a command of the session, so it is checked,
audited and takes effect
+ * exactly as if the client had sent the statement. Reading the options back
gives what
+ * {@code SHOW VARIABLES} shows, every value as a string: the one
representation {@code SET} accepts
+ * back, whatever the Java type of the variable behind it.
+ *
+ * <p>The result of setting an option is one of the three {@link ErrorValue}s
per name and nothing
+ * else, so the reason a value was refused only reaches the frontend log.
+ */
+public final class FlightSessionOptions {
+ private static final Logger LOG =
LogManager.getLogger(FlightSessionOptions.class);
+
+ /** The current catalog; set with {@code SWITCH}. */
+ public static final String CATALOG = "catalog";
+ /** The current database; set with {@code USE}. */
+ public static final String SCHEMA = "schema";
+
+ // What a session variable is called in SET: VariableMgr looks the name up
case-insensitively,
+ // and only a name made of these characters is one the parser reads as the
variable's identifier.
+ private static final Pattern VARIABLE_NAME =
Pattern.compile("[A-Za-z_][A-Za-z0-9_]*");
+
+ private FlightSessionOptions() {
+ }
+
+ /**
+ * Sets the options of one request, each on its own, and returns the error
of every option that
+ * could not be set; an option absent from the result was set. {@code
catalog} goes first and
+ * {@code schema} second, since {@code USE} names a database of the
current catalog; the others
+ * follow in name order, so that a request applies the same way every
time. Runs as a command of
+ * the session, under its command lock.
+ */
+ public static Map<String, SetSessionOptionsResult.Error>
set(ConnectContext ctx,
+ Map<String, SessionOptionValue> options) {
+ List<String> names = new ArrayList<>(options.keySet());
+ Collections.sort(names);
+ names.remove(SCHEMA);
+ names.remove(CATALOG);
+ if (options.containsKey(SCHEMA)) {
+ names.add(0, SCHEMA);
+ }
+ if (options.containsKey(CATALOG)) {
+ names.add(0, CATALOG);
+ }
+ Map<String, SetSessionOptionsResult.Error> errors = new
LinkedHashMap<>();
+ for (String name : names) {
+ ErrorValue error = setOne(ctx, name, options.get(name));
+ if (error != null) {
+ errors.put(name, new SetSessionOptionsResult.Error(error));
+ }
+ }
+ return errors;
+ }
+
+ /** Sets one option and returns why it could not be, or null when it was
set. */
+ @VisibleForTesting
+ static ErrorValue setOne(ConnectContext ctx, String name,
SessionOptionValue value) {
+ if (CATALOG.equals(name)) {
+ return switchCatalog(ctx, value);
+ }
+ if (SCHEMA.equals(name)) {
+ return useDatabase(ctx, value);
+ }
+ return setVariable(ctx, name, value);
+ }
+
+ /**
+ * The options of the session: the current catalog, the current database
(an empty string when
+ * none has been chosen) and every session variable {@code SHOW VARIABLES}
would list, in its text.
+ */
+ public static Map<String, SessionOptionValue> get(ConnectContext ctx) {
+ Map<String, SessionOptionValue> options = new LinkedHashMap<>();
+ options.put(CATALOG,
SessionOptionValueFactory.makeSessionOptionValue(ctx.getDefaultCatalog()));
+ String database = ctx.getDatabase();
+ options.put(SCHEMA,
SessionOptionValueFactory.makeSessionOptionValue(database == null ? "" :
database));
+ for (List<String> row : VariableMgr.dump(SetType.SESSION,
ctx.getSessionVariable(), null)) {
+ // A row is name, value, default value, changed.
+ options.put(row.get(0),
SessionOptionValueFactory.makeSessionOptionValue(row.get(1)));
+ }
+ return options;
+ }
+
+ private static ErrorValue switchCatalog(ConnectContext ctx,
SessionOptionValue value) {
+ String catalog = value.acceptVisitor(STRING_VALUE);
+ if (catalog == null || catalog.isEmpty()) {
+ return ErrorValue.INVALID_VALUE;
+ }
+ return runStatement(ctx, CATALOG, "SWITCH " +
quoteIdentifier(catalog), ErrorCode.ERR_UNKNOWN_CATALOG);
Review Comment:
[P2] Classify malformed catalog names as INVALID_VALUE
A catalog value such as bad.name parses safely as one quoted identifier but
fails SwitchCommand's checkCatalogAllRules with ERR_WRONG_NAME_FORMAT. Because
this call recognizes only ERR_UNKNOWN_CATALOG as a refused value, the result is
generic ERROR, even though the failure is entirely caused by the supplied
catalog option value. Please include catalog name-format validation in the
INVALID_VALUE path and cover a malformed name alongside a well-formed
nonexistent catalog.
##########
regression-test/suites/arrow_flight_sql_p0/test_session_options.groovy:
##########
@@ -0,0 +1,194 @@
+// 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.
+
+import java.sql.DriverManager
+import java.sql.SQLException
+
+// The Flight SQL JDBC driver on the classpath shades Arrow Flight; its
FlightSqlClient is the
+// same client the ADBC and JDBC drivers speak the session actions with.
+import
org.apache.arrow.driver.jdbc.shaded.org.apache.arrow.flight.CloseSessionRequest
+import org.apache.arrow.driver.jdbc.shaded.org.apache.arrow.flight.FlightClient
+import
org.apache.arrow.driver.jdbc.shaded.org.apache.arrow.flight.FlightRuntimeException
+import
org.apache.arrow.driver.jdbc.shaded.org.apache.arrow.flight.FlightStatusCode
+import
org.apache.arrow.driver.jdbc.shaded.org.apache.arrow.flight.GetSessionOptionsRequest
+import org.apache.arrow.driver.jdbc.shaded.org.apache.arrow.flight.Location
+import
org.apache.arrow.driver.jdbc.shaded.org.apache.arrow.flight.NoOpSessionOptionValueVisitor
+import
org.apache.arrow.driver.jdbc.shaded.org.apache.arrow.flight.SessionOptionValueFactory
+import
org.apache.arrow.driver.jdbc.shaded.org.apache.arrow.flight.SetSessionOptionsRequest
+import
org.apache.arrow.driver.jdbc.shaded.org.apache.arrow.flight.sql.FlightSqlClient
+import
org.apache.arrow.driver.jdbc.shaded.org.apache.arrow.memory.RootAllocator
+
+// The session actions of Arrow Flight SQL on a Doris session:
SetSessionOptions sets the current
+// catalog (`catalog`, what the ADBC driver sends for adbc.connection.catalog
and the JDBC driver for
+// its catalog property), the current database (`schema`,
adbc.connection.db_schema) and session
+// variables (any other name), each answered on its own with INVALID_NAME /
INVALID_VALUE / ERROR;
+// GetSessionOptions reads them back as SHOW VARIABLES text; CloseSession
invalidates the bearer
+// token at once.
+//
+// Not in the 'arrow_flight_sql' group on purpose: `sql` stays the MySQL
control connection, and the
+// session under test is a raw Flight SQL client of its own.
+suite("test_session_options") {
+ String host = context.config.otherConfigs.get("extArrowFlightSqlHost")
+ int port = context.config.otherConfigs.get("extArrowFlightSqlPort") as int
+ String user = context.config.otherConfigs.get("extArrowFlightSqlUser")
+ String password =
context.config.otherConfigs.get("extArrowFlightSqlPassword")
+ String tableName = "session_options_tbl"
Review Comment:
[P2] Keep the regression table available for debugging
This is an ordinary single-table case, so the repository test contract
requires hardcoding session_options_tbl in the SQL rather than routing it
through tableName; it also requires dropping before setup, not after the test,
so the final state remains available when debugging a failure. Please remove
this variable/interpolation and the trailing DROP, keeping only the pre-test
cleanup.
--
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]
