matanper opened a new issue, #70451: URL: https://github.com/apache/doris/issues/70451
### Search before asking - [x] I had searched in the [issues](https://github.com/apache/doris/issues?q=is%3Aissue) and found no similar issues. ### Version 4.1.4 (cloud mode). master (`9fb3e3a2a13`) has the same code. ### What's Wrong? A statement that throws an exception **before** `StmtExecutor.execute()` is reported to the client as a success. Nothing records an error or writes an audit log entry, and a non-`UserException` isn't logged at all. Over Arrow Flight SQL this is a wrong result: a `SELECT` is answered with the synthesized one-row `StatusResult = "0"` table from `FlightSqlChannel.addOKResult` instead of its rows. A client that reads rows by position or name, and sees no error, takes that row (or "no matching rows") as the query's answer. ### Why `ConnectProcessor.handleQuery` (`fe/fe-core/src/main/java/org/apache/doris/qe/ConnectProcessor.java:242-249` on master) catches everything `executeQuery` throws: ```java } catch (ConnectionException exception) { throw exception; } catch (UserException exception) { LOG.warn("execute query exception", exception); } catch (Exception ignored) { // saved use handleQueryException } ``` The comment assumes `handleQueryException` has already run, which is only true inside the per-statement `catch (Throwable)` at `:386-387`. That catch wraps `executor.execute()` (`:366`) alone. Code that runs earlier for each statement throws straight to `handleQuery` with the state still OK from `ctx.getState().reset()` (`:325`). That code is the cloud request metrics block (`:256-270`), `convertSqlByDialect` (`:274`), and, inside the loop, `resolveWorkloadGroupName()` (`:335`), `new StmtExecutor(...)` (`:340`) and the profile and executor setup up to `ctx.setExecutor` (`:357`). On the Arrow Flight path, `DorisFlightSqlProducer.executeQueryStatement` (`fe/fe-core/src/main/java/org/apache/doris/arrowflight/DorisFlightSqlProducer.java:251-262`) then finds: - a non-ERR state; - `isReturnResultFromLocal()` still true, because only `StmtExecutor` clears it, once a query is running; - an empty channel. So it returns `addOKResult`, which is meant for `SET`/`USE`. The `carryForwardedOutcome` comment in `FlightProtocolAdapter` describes the same fake-OK outcome for forwarded statements. This is the same outcome reached through a different path. ### What You Expected? A statement that fails before it executes should be reported as failed. It should get an error state, an audit log entry with that error, and a WARN with the stack trace. For Arrow Flight SQL that means the call fails, instead of returning a `StatusResult` row for a `SELECT`. ### How to Reproduce? 1. Make any call in that span throw for one statement. For example, in a unit test, subclass `ConnectProcessor` and override `resolveWorkloadGroupName()` to throw a `RuntimeException`. 2. Call `handleQuery("SELECT 1")`. 3. `ctx.getState().getStateType()` is `OK`, the error message is empty, `auditAfterExec` isn't called, and nothing is logged. 4. Over Arrow Flight SQL, the same failure gives a `FlightInfo` whose only endpoint returns one row, `StatusResult: "0"`. On a busy cloud-mode cluster with many Arrow Flight SQL sessions, we see occasional `SELECT`s answered like this. They have no audit log entry and no FE log line. ### Anything Else? Proposed fix, in `handleQuery`: - If the state is already ERR, the statement failed while executing and was already reported, so keep the current behaviour. - Otherwise, log the exception at WARN and pass it to `handleQueryException(exception, originStmt, null, null)`. That sets the error and writes one audit entry, and the Flight producer's existing ERR check then fails the call. A regression test that throws from `resolveWorkloadGroupName()` fails without the fix (`expected: <ERR> but was: <OK>`) and passes with it. A failure during execution keeps its own error and is audited once. We have a patch and can open a PR. ### Are you willing to submit PR? - [x] Yes I am willing to submit a PR! ### Code of Conduct - [x] I agree to follow this project's [Code of Conduct](https://www.apache.org/foundation/policies/conduct) -- 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]
