github-actions[bot] commented on code in PR #64366:
URL: https://github.com/apache/doris/pull/64366#discussion_r3607321509
##########
fe/fe-core/src/main/java/org/apache/doris/qe/ConnectProcessor.java:
##########
@@ -336,6 +336,9 @@ public void executeQuery(String originStmt) throws
Exception {
StatementBase parsedStmt = stmts.get(i);
parsedStmt.setOrigStmt(new OriginStatement(auditStmt,
usingOrigSingleStmt ? 0 : i));
parsedStmt.setUserInfo(ctx.getCurrentUserIdentity());
+ if (parsedStmt instanceof LogicalPlanAdapter) {
+ ((LogicalPlanAdapter)
parsedStmt).getStatementContext().resetStatementStartTime();
Review Comment:
[P1] Isolate later-statement hints before refreshing this context
`executeQuery()` parses the whole packet before this loop, and
`LogicalPlanBuilder.withHints()` calls `setVarOnceInSql()` while parsing each
statement. A future statement can therefore leave the shared `SessionVariable`
modified when this reset processes an earlier statement. With base `+08:00`,
`select now(); select /*+ SET_VAR(time_zone='+00:00') */ 1` reaches this line
with UTC installed, so the first statement is refreshed from the second
statement's hint; its executor then reverts that value before the hinted
statement runs. Restore/isolate parse-time hints per statement or defer
applying them, and add this two-statement regression.
##########
be/src/service/point_query_executor.cpp:
##########
@@ -308,6 +331,8 @@ Status PointQueryExecutor::init(const
PTabletKeyLookupRequest* request,
_tablet = DORIS_TRY(ExecEnv::get_tablet(request->tablet_id()));
if (cache_handle != nullptr) {
_reusable = cache_handle;
+ _reusable_execution_lock = _reusable->acquire_execution_lock();
Review Comment:
[P1] Do not block light-pool workers behind the execution lease
`tablet_fetch_data()` runs on the bounded shared `_light_work_pool`, whose
own contract says handlers must not lock or access disk. A hit now blocks here
and holds the lease through storage lookup, output, and profiling. FE timeout
cancels only its Future, while the BE lambda never checks
`controller->IsCanceled()`, so later executions of that UUID can occupy more
light workers waiting, time out themselves, and still execute obsolete lookups
serially. Enough sessions can exhaust the pool and stall unrelated light RPCs.
Use execution-local state or nonblocking/rescheduled admission with
cancellation checks, and cover real executor/RPC overlap rather than only
`try_lock()`.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/load/NereidsBrokerLoadTask.java:
##########
@@ -25,12 +25,15 @@
import org.apache.doris.thrift.TFileFormatType;
import org.apache.doris.thrift.TFileType;
+import java.time.Instant;
import java.util.List;
/**
* Nereids Broker Load Task
*/
public class NereidsBrokerLoadTask implements NereidsLoadTaskInfo {
+ private final Instant statementStartTime = Instant.now();
Review Comment:
[P1] Share one instant across the broker-load job
This initializer runs once per `NereidsBrokerLoadTask`, while
`BrokerLoadJob.createLoadingTask()` splits one submitted job into one
sequentially planned `LoadLoadingTask` per `FileGroupAggKey`. A multi-table or
disjoint-partition broker load therefore gives sibling fragments different
instants, so two `SET(ts=now(6))` data descriptions can commit different
timestamps. The earlier broker-load threads align planning and execution within
one sibling; they do not align siblings of the same job. Capture one instant
before the key loop, pass it through both shared-nothing and cloud task
creation, and assert equality in a two-key test.
##########
be/src/service/point_query_executor.cpp:
##########
@@ -216,6 +214,28 @@ void Reusable::return_block(std::unique_ptr<Block>& block)
{
}
}
+void Reusable::_update_runtime_state(const PTabletKeyLookupRequest& request) {
+ if (request.has_time_zone() && !request.time_zone().empty()) {
+ _runtime_state->set_timezone(request.time_zone());
+ }
+ if (request.has_timestamp_ms()) {
+ _runtime_state->set_query_time(request.timestamp_ms(),
request.nano_seconds());
+ }
+}
+
+Status Reusable::_rebuild_output_exprs() {
+ RETURN_IF_ERROR(VExpr::create_expr_trees(_output_exprs,
_output_exprs_ctxs));
+ RowDescriptor row_desc(tuple_desc());
+ RETURN_IF_ERROR(VExpr::prepare(_output_exprs_ctxs, _runtime_state.get(),
row_desc));
+ return VExpr::open(_output_exprs_ctxs, _runtime_state.get());
+}
+
+Status Reusable::refresh(const PTabletKeyLookupRequest& request) {
Review Comment:
[P2] Preserve the cache-hit fast path for request-independent expressions
Every hit unconditionally clears all contexts and then recreates every saved
Thrift expression tree, runs `prepare()`, and runs `open()`. Slot-only
projections and other request-global-independent outputs therefore pay the full
allocation/tree-walk cost on the latency-sensitive lookup-cache path, and this
work also lengthens the new execution critical section. Refreshing stale
constant-derived function state is necessary, but it does not require
rebuilding request-independent trees. Keep an immutable prepared shape with
execution-local mutable state, or gate rebuilding on global dependency, and add
a representative cache-hit benchmark/counter guard.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -227,6 +229,22 @@ public class StmtExecutor {
// this constructor is mainly for proxy
public StmtExecutor(ConnectContext context, OriginStatement originStmt,
boolean isProxy) {
+ this(context, originStmt, isProxy, Instant.now());
Review Comment:
[P1] Keep one StatementContext through raw parsing
This constructor retains context A in the executor, but `parseByNereids()`
invokes the normal parser, whose multi-statement visitor creates and installs
context B in the same `ConnectContext`; it stores only the parsed statement and
never updates the executor field. `new NereidsPlanner(statementContext)` then
folds with A, while `CoordinatorContext.createQueryGlobals()` reaches B through
the connection. A proxy query such as `now(6), now(kint)` can consequently use
different FE and BE instants, and explicit-time internal constructors lose
their supplied instant on the BE side. Reuse/copy one context through parsing
and add a parse-to-coordinator globals test.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/ConnectProcessor.java:
##########
@@ -336,6 +336,9 @@ public void executeQuery(String originStmt) throws
Exception {
StatementBase parsedStmt = stmts.get(i);
parsedStmt.setOrigStmt(new OriginStatement(auditStmt,
usingOrigSingleStmt ? 0 : i));
parsedStmt.setUserInfo(ctx.getCurrentUserIdentity());
+ if (parsedStmt instanceof LogicalPlanAdapter) {
+ ((LogicalPlanAdapter)
parsedStmt).getStatementContext().resetStatementStartTime();
Review Comment:
[P1] Keep cache validation on the execution timestamp
`parseFromSqlCache()` creates this adapter's `StatementContext` at T0, and
`nondeterministicFunctionChanged()` validates cached `now()`/`current_date()`
folds against T0 before the remaining policy checks and possible
`SqlCache.getCacheData()` RPC. This reset happens only afterward and changes
the same context to T1. If lookup crosses a second boundary (or local
midnight), a cached T0 `now(0)` row is accepted and returned for a statement
whose execution context is T1. Establish the per-statement instant before cache
validation, or preserve the validation instant on a hit, and cover the boundary
with an injected clock.
--
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]
