[Storage] Add `byte_size` to `DataPointer`

Project: duckdb/duckdb | Language: Python | Concepts: testing-strategy, caching, error-handling | Difficulty: mid
Link: https://github.com/duckdb/duckdb/pull/25176

The problem

The regression test runner for DuckDB benchmarks did not properly handle error reporting or cache isolation between runs. This led to two main issues:

Errors like INCORRECT results from benchmarks were not surfaced properly; the script would try to cast these strings to floats and fail with a confusing error.
Benchmark runs for different code versions (old/new) reused the same cache directory, causing state leakage between runs and making results unreliable.

Why it's non-trivial

A naive fix would just catch conversion errors or clear the cache, but:

Error messages from benchmarks can be multi-line and may appear in either stdout or stderr, requiring careful parsing to provide useful diagnostics.
Some benchmarks require persistent data for performance or correctness, so always clearing the cache may not be feasible.
Isolating cache directories while still sharing large, read-only benchmark data (to avoid duplication) requires careful directory and symlink management.

The approach

1. Improved error handling in benchmark runner:

Instead of casting every timing value directly to float, the code now checks for error strings and surfaces detailed messages:

def benchmark_failure_message(
    label: str, benchmark: str, row: List[str], trailing_lines: List[str], stdout: str
) -> str:
    # ... constructs a detailed error message for display ...

This function builds a clear error message using the benchmark label, status, and any diagnostic lines that follow.

In the runner, error handling is now explicit:

-                if row:
-                    timings.append(float(row[2]))
+        for line_index, row in enumerate(rows, start=1):
+            if not row:
+                continue
+            try:
+                timings.append(float(row[2]))
+            except (IndexError, ValueError):
+                message = benchmark_failure_message(
+                    self.label, benchmark, row, stderr_lines[line_index + 1 :], process.stdout
+                )
+                print(f"Failed to run benchmark {benchmark}: {message}", flush=True)
+                return None, message

This change ensures that if a row contains INCORRECT or ERROR, it is reported with context, not as a conversion failure.

2. Isolating benchmark cache directories:

A helper finds the cache directory for a given runner:

def find_benchmark_cache_directory(runner_path: str) -> str:
    return os.path.abspath(os.path.join(os.path.dirname(runner_path), "..", "..", "..", "duckdb_benchmark_data"))

This computes the cache directory path relative to the runner binary.

When clearing caches, the same logic is used:

-def clear_benchmark_caches(runner_paths: List[str]):
-    cache_paths = {
-        os.path.abspath(os.path.join(os.path.dirname(runner_path), "..", "..", "..", "duckdb_benchmark_data"))
-        for runner_path in runner_paths
-    }
+def clear_benchmark_caches(runner_paths: List[str]):
+    cache_paths = {find_benchmark_cache_directory(runner_path) for runner_path in runner_paths}
     for cache_path in cache_paths:
         shutil.rmtree(cache_path, ignore_errors=True)

This ensures all cache directories used by any runner are cleared as needed.

For full isolation, a new function creates separate root directories for each runner and symlinks the shared data:

def create_isolated_benchmark_root(source_root: Path, target_root: Path):
    target_root.mkdir()
    for source_path in source_root.iterdir():
        if source_path.name == "duckdb_benchmark_data":
            continue
        target_path = target_root / source_path.name
        target_path.symlink_to(source_path, target_is_directory=source_path.is_dir())
    (target_root / "duckdb_benchmark_data").mkdir()

This creates a new directory for each runner, symlinks all data except the cache, and makes a fresh cache directory.

3. Tests for error reporting and cache isolation:

Several new tests were added to verify:

Error messages are surfaced when a benchmark returns INCORRECT or ERROR.
The correct cache directory is used.
Isolating cache directories prevents state leakage between runs.

Example test for error reporting:

def test_surfaces_incorrect_result_diagnostic(self):
    completed_process = subprocess.CompletedProcess(
        args=[],
        returncode=0,
        stdout="",
        stderr=(
            "name\trun\ttiming\n"
            "query.benchmark\t1\tINCORRECT\n"
            "INCORRECT RESULT: Invalid Input Error: attempted to read past the end of the segment\n"
        ),
    )
    with patch("scripts.regression.benchmark.subprocess.run", return_value=completed_process):
        timings, error = BenchmarkRunner("benchmark_runner", "current").run("query.benchmark", 1)
    self.assertIsNone(timings)
    self.assertEqual(
        error,
        "current benchmark runner reported INCORRECT for query.benchmark on run 1:\n"
        "INCORRECT RESULT: Invalid Input Error: attempted to read past the end of the segment",
    )

This verifies that an INCORRECT result is reported with the correct diagnostic.

From the review thread

Now it correctly reports:

1: benchmark/micro/logger/logging_overhead/duckdb_persistent_q1_with_default_logging.benchmark
Base:
 Base benchmark runner reported INCORRECT for benchmark/micro/logger/logging_overhead/duckdb_persistent_q1_with_default_logging.benchmark on run 1:
INCORRECT RESULT: TransactionContext Error: Failed to commit: Failed to deserialize: expected end of object, but found field id: 106
Comparison batch 2 ran PR immediately before Base. Both runners use the same benchmark cache, so files may have been last written by PR.
PR:
 No failure
- 52

@Tishj fyi some benchmarks require/omit clearing the benchmark cache dir. Preferably we clear it always (so we isolate bench runs better, as you said), but i'm not sure if/when a benchmark requires clearing?

Hmm, seeing as the only one where that's the case is realnest, my guess is that that's just because of the size of the benchmark data. I would think we just want to make two separate benchmark directories for the two runners and symlink the required data of a benchmark into the directories ?

Takeaways

Always surface and propagate diagnostic information from test failures, rather than letting generic exceptions mask the real issue.
Isolate stateful artifacts (such as benchmark caches) between test runs to prevent cross-contamination and ensure reproducibility.
When sharing large, read-only data between test runs, use symlinks or similar mechanisms to avoid unnecessary duplication while still isolating mutable state.

[Storage] Add byte_size to DataPointer