refactor: Explain mode to access mode

[johnmzz]

Description

Move ExplainMode analysis from execution-time correction to access-mode analysis phase.
This ensures all EXPLAIN queries are classified as read-only before execution begins,
improving architectural clarity and enabling EXPLAIN to work on read-only databases.

Changes

• analyzeQuery(): EXPLAIN queries now return access_mode=kRead immediately
• executeCore(): EXPLAIN CHECKPOINT routed correctly through execute_on_storage path
• Tests: Updated with comprehensive EXPLAIN vs PROFILE coverage

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/main/execution_slot.cc:89

• This refactor introduces a second source of truth for explain behavior (explain_mode parameter) alongside execution::CacheValue still appearing to carry explain_mode (as implied by the replacement in this diff). To prevent future divergence bugs (e.g., cached/plan-time mode differing from analysis-time mode), consider either (a) removing/stop populating the CacheValue::explain_mode field entirely, or (b) adding a debug assertion where both are available to enforce they match.

void markPlanningChangedIfNeeded(InPlaceWriteScope& write_scope,
                                 physical::ExplainMode explain_mode,
                                 const execution::CacheValue* prepared_query,
                                 const Status& execution_status) {

src/main/execution_slot.cc:94

• This refactor introduces a second source of truth for explain behavior (explain_mode parameter) alongside execution::CacheValue still appearing to carry explain_mode (as implied by the replacement in this diff). To prevent future divergence bugs (e.g., cached/plan-time mode differing from analysis-time mode), consider either (a) removing/stop populating the CacheValue::explain_mode field entirely, or (b) adding a debug assertion where both are available to enforce they match.

  if (!execution_status.ok() ||
      explain_mode == physical::ExplainMode::EXPLAIN) {

src/main/execution_slot.cc:104

• This refactor introduces a second source of truth for explain behavior (explain_mode parameter) alongside execution::CacheValue still appearing to carry explain_mode (as implied by the replacement in this diff). To prevent future divergence bugs (e.g., cached/plan-time mode differing from analysis-time mode), consider either (a) removing/stop populating the CacheValue::explain_mode field entirely, or (b) adding a debug assertion where both are available to enforce they match.

Status executePreparedQuery(execution::CacheValue& prepared_query,
                            physical::ExplainMode explain_mode,

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (2)

proto/physical.proto:447

• PhysicalPlan removed field number 5 (explain_mode). In protobuf, removed field numbers should be marked reserved to prevent accidental reuse (which can break backward/forward compatibility for persisted/cached plans).

  int32 plan_id = 1;
  // to be deprecated
  repeated PhysicalOpr plan = 3;
  ExecutionFlag flag = 4;
}

src/main/execution_slot.cc:342

• PR description says analyzeQuery() now returns access_mode = kRead for all EXPLAIN queries. With the current analyzer implementation, access_mode is still inferred from tokens even when explain_mode == EXPLAIN (e.g., EXPLAIN ... SET ... remains kUpdate, and EXPLAIN CHECKPOINT remains kUpdate). If the intent is that EXPLAIN is always classified read-only at analysis time, the analyzer should override analysis.access_mode when explain_mode == EXPLAIN; otherwise the PR description should be updated to reflect that EXPLAIN is enforced as read-only via the executeCore() execution path instead of via QueryAnalysis.access_mode.

    RETURN_IF_NOT_OK(
        executePreparedQuery(*prepared_query, analysis.explain_mode,
                             parsed_parameters.value(), storage, response));
    return Status::OK();
  };

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (3)

proto/physical.proto:447

• The PhysicalPlan message removed field number 5 (explain_mode) without reserving the tag/name. In protobuf, removed field numbers/names should be reserved to prevent accidental reuse and wire-compatibility issues in future schema edits.

  int32 plan_id = 1;
  // to be deprecated
  repeated PhysicalOpr plan = 3;
  ExecutionFlag flag = 4;
}

tests/compiler/explain_test.cpp:37

• The updated tests only assert analysis.explain_mode and no longer validate the PR’s stated behavior that EXPLAIN works on read-only databases / is treated as non-mutating before execution. Consider adding coverage that runs an EXPLAIN of a mutating statement (e.g., EXPLAIN ... SET ...) against a read-only DB config and asserts it succeeds (and does not require a write transaction).

  auto analysis = planner_.analyzeQuery("MATCH (n:person) RETURN n.name");
  EXPECT_EQ(analysis.explain_mode, physical::ExplainMode::NONE);
}

src/main/execution_slot.cc:341

• PR description says analyzeQuery() returns access_mode=kRead for EXPLAIN queries, but the execution flow here only threads analysis.explain_mode into execution; access-mode inference still uses analysis.access_mode elsewhere (and GOptPlanner::analyzeQuery()/existing tests classify e.g. EXPLAIN ... SET and EXPLAIN CHECKPOINT as kUpdate). Either update the PR description to reflect the intended semantics (EXPLAIN forces read execution while keeping underlying access_mode), or change the analyzer/flow so EXPLAIN queries are reported as read-only at analysis time.

    RETURN_IF_NOT_OK(
        executePreparedQuery(*prepared_query, analysis.explain_mode,
                             parsed_parameters.value(), storage, response));
    return Status::OK();

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

Suppressed comments (2)

tests/compiler/explain_test.cpp:92

• PR description says analyzeQuery() classifies all EXPLAIN queries as read-only (access_mode = kRead) before execution. The updated tests only assert explain_mode, even for EXPLAIN ... SET ... and EXPLAIN CREATE ..., so they don’t verify the key read-only classification behavior (and currently GOptPlanner::analyzeQuery() still returns kUpdate/kSchema for those patterns). Please align implementation + tests with the stated contract (either enforce kRead for EXPLAIN-prefixed queries, or adjust the PR description if that’s not intended).

// Test 8: All query types with EXPLAIN
TEST_F(ExplainTest, AllQueryTypesWithExplain) {
  std::vector<std::string> queries = {
      "EXPLAIN MATCH (n:person) RETURN n.name",
      "EXPLAIN MATCH (n:person)-[e:knows]->(m:person) RETURN n.name",
      "EXPLAIN MATCH (n:person) SET n.age = 30",
      "EXPLAIN CREATE NODE TABLE person(id INT64, PRIMARY KEY(id))",
  };

  for (const auto& query : queries) {
    auto analysis = planner_.analyzeQuery(query);
    EXPECT_EQ(analysis.explain_mode, physical::ExplainMode::EXPLAIN)
        << "Query: " << query;
  }

proto/physical.proto:447

• PhysicalPlan.explain_mode was removed, but the protobuf field number/name aren’t reserved. Without reserving, a future field could reuse tag 5 (or the name), breaking wire-compatibility with older serialized plans that still contain explain_mode. Reserve the removed field number and name inside PhysicalPlan.

  int32 plan_id = 1;
  // to be deprecated
  repeated PhysicalOpr plan = 3;
  ExecutionFlag flag = 4;
}