From 8a8ba8c43220070b4fd3f7c0c1ffbe3280a3dba1 Mon Sep 17 00:00:00 2001 From: VibeSWE <338779468+VibeSWE@users.noreply.github.com> Date: Tue, 10 Nov 2026 20:27:47 -0600 Subject: [PATCH 1/5] perf: validate cached small parquet reads and streamline snapshot planning --- .../performance/validated-small-file-cache.md | 132 +++ .../metastores/object_store_metastore.rs | 131 ++- src/query/mod.rs | 2 + src/query/small_file_cache.rs | 751 ++++++++++++++++++ src/query/stream_schema_provider.rs | 143 +++- src/storage/mod.rs | 2 +- 6 files changed, 1107 insertions(+), 54 deletions(-) create mode 100644 docs/performance/validated-small-file-cache.md create mode 100644 src/query/small_file_cache.rs diff --git a/docs/performance/validated-small-file-cache.md b/docs/performance/validated-small-file-cache.md new file mode 100644 index 000000000..a7280b676 --- /dev/null +++ b/docs/performance/validated-small-file-cache.md @@ -0,0 +1,132 @@ +# Compatible query read caching + +This implements a conservative read optimization against d6a155f2. It preserves +bucket paths and formats, and adds no required configuration or TTL. + +## Storage and freshness findings + +`upload_single_parquet_file` uses unconditional multipart upload, followed by a +size check and manifest publication. Staging conversion normally incorporates a +ULID (`src/parseable/staging/writer.rs`), but crash recovery can retry a remote +path. The storage API doesn't enforce immutable parquet objects. Retention also +deletes them. A pathname and manifest size alone therefore aren't a safe cache +identity. The metadata conditional-write API doesn't make parquet uploads, stream +snapshots or manifests immutable; those writers still use ordinary writes. + +`stream.json` embeds the snapshot. Manifests are updated under existing paths. +Query and Prism nodes must see other processes' updates. Standalone mode alone +isn't an enforceable guarantee that another process won't write the same bucket. +A process-local write-through metadata cache would miss those writes. An ordinary +in-flight fetch started before a new query is likewise insufficient for strict +next-query freshness. A TTL knowingly changes that contract. None is introduced. + +PR #130's resident-stream and schema-refresh behavior is preserved. This patch +changes the scan's snapshot ordering without restoring per-query stream reloads: +lease staging before the first settings/snapshot GET. If remote settings show +that staging is needed when resident settings predicted otherwise, lease it and +fetch a fresh snapshot. Upload publishes before staging is unlinked, so either +the lease or remote manifest retains the file. Historical scans don't need leases. + +## Small parquet files + +DataFusion 55 already has `CachedParquetFileReaderFactory`, a byte-limited file +metadata cache, and a metadata-size hint. Query runtime creation and ParquetFormat +factory installation matter: ParquetFormat replaces a factory supplied earlier +on ParquetSource. The patch installs its reader after that step and retains the +original factory for larger files, preserving its options and metadata cache. + +For a manifest file size at most 256 KiB, a reader fetches the complete object in +one bounded GET. It holds those bytes for its lifetime; metadata, page indexes, +and column reads then use slices of the same body. On a subsequent scan, a +conditional GET validates the cached ETag. A 304 reuses the bytes without a body; +a changed object replaces them. A 404/error invalidates the entry and propagates. +No ETag means no cross-reader caching. Growth past the limit uses the original +reader with the newly observed size. The bounded probe prevents an unexpected +large replacement from being read into memory in full. + +Keys use the process's singleton storage-provider identity and full object path, +including tenant prefixes. Per-query object-store clients differ, so their pointer +identity would prevent cross-query hits. The configured provider lives for the +process lifetime. A physical scan also has an 8 MiB LRU view shared between +DataFusion metadata and data readers, avoiding a second validation of the same +body within that scan. New scans never inherit that view. LRU storage is capped at 64 MiB and 4,096 entries. Active readers +can retain evicted bodies; as with existing scan buffers, their memory also depends +on query concurrency. This limit is a cache residency limit, not a process RSS cap. + +Stream/tenant removal and retention need no cached catalog invalidation because +catalog objects aren't cached across requests. They stop selecting deleted files. +Direct reads of a removed file validate and fail; bytes left unreferenced in the +cache are reclaimed by bounded eviction. Recreating the same path validates its +new ETag. There is no negative cache to hide newly uploaded files. + +This deliberately retains one network validation per small file per scan while +the bounded scan view retains it; eviction or concurrent first opens can require +additional validations. Avoiding +that last round trip requires a stronger immutable/versioned publication contract. +Whole-body reads trade extra cold bytes for fewer round trips; the limit keeps +that trade confined to small objects. Large-file metadata behavior is unchanged. + +## Other read costs + +Dataset discovery retains its existing storage-based membership checks, now with +up to 16 metadata-prefix LISTs in flight. It makes the same number of LIST calls. +In-memory membership could hide streams created/deleted by other nodes. + +The S3 pool idle timeout remains 15 seconds. The code constructs clients when +creating query runtimes; a timeout change alone does not establish useful reuse +across dashboard refreshes. No TLS handshake benefit has been measured here. + +## Reproduction + +Build locally with `PATH=$HOME/.cargo/bin:$PATH`, `P_BUILD_NO_UI=1`, and +`CARGO_BUILD_JOBS=4`. Run one Cargo command at a time: + +``` +cargo test --lib query::small_file_cache -- --nocapture +P_CACHE_TEST_MINIO_ENDPOINT=http://127.0.0.1:18390 cargo test --lib repeated_sixty_file_queries -- --nocapture +cargo test --lib snapshot_capture_tests +cargo test --lib next_metadata_read_sees_external_writer_changes +cargo test --lib staging_lease +cargo clippy --all-targets +``` + +The measurement uses MetricLayer and StorageMetricWindow, hence the same +`parseable_storage_request_response_time_count` samples exposed by `/api/v1/metrics`. +It generates 60 one-minute parquet files, opens a fresh DataFusion query session +for each run, compares returned rows, and evaluates a PromQL rate range expression +from the scanned samples. It isolates parquet I/O: HTTP handlers, schema refresh, +`stream.json` and manifest reads are outside the measurement. Do not interpret +its table as a reproduction of the supplied live-instance endpoint totals. + +## Measured MinIO results + +Local MinIO, 60 files containing 28,800 samples over one hour, 894,420 bytes total. +Both runs returned identical results; these are the repeated run's metric deltas. +The PromQL case scans samples and evaluates `sum(rate(requests{job="api"}[5m]))` +over a range with 60-second steps. The fixture is intentionally narrower than a +real OTLP dataset, and excludes endpoint/catalog overhead as described above. + +| Parquet workload | Before GET | Before GET_RANGES | After GET | After GET_RANGES | Calls before → after | +| --- | ---: | ---: | ---: | ---: | ---: | +| COUNT(*) | 0 | 60 | 60 | 0 | 60 → 60 | +| Filtered GROUP BY | 1 | 180 | 60 | 0 | 181 → 60 | +| PromQL scan + range evaluation | 60 | 240 | 60 | 0 | 300 → 60 | + +COUNT's file-request count is unchanged. Its usual standalone catalog preamble +loses one stream.json GET through the capture-order change, but that reduction is +code-derived, not included in this measured table. Dataset discovery also keeps +its LIST count while allowing concurrent probes. No local wall-time claim is made. + +The existing MetricLayer byte counter charges requested bounded-range lengths, +including unsuccessful/304 requests. This harness deliberately reports request +counts, not inferred transferred bytes from that counter. + +Validation: the full library suite passed (1,382 passed, 4 ignored). The final +MinIO run passed all four cache/measurement tests, including an assertion that +each warm query receives 60 HTTP 304 responses. Fresh-cache and repeated-query +request counts were identical; warm responses carry no parquet body. +Plain `cargo clippy --all-targets` passed with existing warnings only; no warning +points to the changed cache, scan, or metastore code. No full HTTP dashboard +benchmark was performed: catalog, schema-refresh and handler overhead remain an +explicit validation gap. MinIO and in-memory storage were exercised; Azure/GCS +were checked through the shared object_store API and were not emulator-tested. diff --git a/src/metastore/metastores/object_store_metastore.rs b/src/metastore/metastores/object_store_metastore.rs index 4a90a0ef1..1b1dff443 100644 --- a/src/metastore/metastores/object_store_metastore.rs +++ b/src/metastore/metastores/object_store_metastore.rs @@ -1552,7 +1552,6 @@ impl Metastore for ObjectStoreMetastore { .map_err(MetastoreError::ObjectStorageError) } else { // not local-disk, object storage - let mut result_file_list = HashSet::new(); let root = tenant_id .as_ref() .map(|tenant| object_store::path::Path::from_iter([tenant.clone()])); @@ -1572,26 +1571,37 @@ impl Metastore for ObjectStoreMetastore { && name != TOMBSTONE_ROOT_DIRECTORY }) .collect::>(); - for stream in streams { - let stream_path = if let Some(root) = root.as_ref() { - object_store::path::Path::from_iter([ - root.as_ref(), - &stream, - STREAM_ROOT_DIRECTORY, - ]) - } else { - object_store::path::Path::from(format!("{}/{}", stream, STREAM_ROOT_DIRECTORY)) - }; - let resp = self.storage.list_with_delimiter(Some(stream_path)).await?; - if resp - .objects - .iter() - .any(|name| name.location.filename().unwrap().ends_with("stream.json")) - { - result_file_list.insert(stream); - } - } - Ok(result_file_list) + let results = + futures::stream::iter(streams) + .map(|stream| { + let root = root.clone(); + async move { + let stream_path = if let Some(root) = root.as_ref() { + object_store::path::Path::from_iter([ + root.as_ref(), + &stream, + STREAM_ROOT_DIRECTORY, + ]) + } else { + object_store::path::Path::from(format!( + "{}/{}", + stream, STREAM_ROOT_DIRECTORY + )) + }; + let resp = self.storage.list_with_delimiter(Some(stream_path)).await?; + if resp.objects.iter().any(|name| { + name.location.filename().unwrap().ends_with("stream.json") + }) { + Ok::<_, MetastoreError>(Some(stream)) + } else { + Ok(None) + } + } + }) + .buffer_unordered(16) + .try_collect::>() + .await?; + Ok(results.into_iter().flatten().collect()) } } } @@ -1601,6 +1611,85 @@ mod tests { use super::*; use crate::storage::LocalFS; + #[tokio::test] + async fn next_metadata_read_sees_external_writer_changes() { + for tenant in [None, Some("tenant".to_owned())] { + let dir = tempfile::tempdir_in(".").unwrap(); + let storage = Arc::new(LocalFS::new(dir.path().to_owned())); + let reader = ObjectStoreMetastore { + storage: storage.clone(), + }; + let stream_path = RelativePathBuf::from_iter([ + tenant.as_deref().unwrap_or(""), + "metrics", + STREAM_ROOT_DIRECTORY, + STREAM_METADATA_FILE_NAME, + ]); + let manifest_path = RelativePathBuf::from_iter([ + tenant.as_deref().unwrap_or(""), + "metrics", + "day", + "manifest.json", + ]); + for generation in ["first", "external-update"] { + // Write directly through storage, bypassing this reader's API. + let format = crate::storage::ObjectStoreFormat { + created_at: generation.into(), + ..Default::default() + }; + storage + .put_object(&stream_path, to_bytes(&format), &tenant) + .await + .unwrap(); + let loaded: crate::storage::ObjectStoreFormat = serde_json::from_slice( + &reader + .get_stream_json("metrics", true, &tenant, false) + .await + .unwrap(), + ) + .unwrap(); + assert_eq!(loaded.created_at, generation); + let manifest = Manifest { + version: generation.into(), + ..Default::default() + }; + storage + .put_object(&manifest_path, encode_manifest(&manifest).unwrap(), &tenant) + .await + .unwrap(); + let loaded = reader + .get_manifest( + "metrics", + Utc::now(), + Utc::now(), + Some(manifest_path.to_string()), + &tenant, + ) + .await + .unwrap() + .unwrap(); + assert_eq!(loaded.version, generation); + } + storage + .delete_object(&manifest_path, &tenant) + .await + .unwrap(); + assert!( + reader + .get_manifest( + "metrics", + Utc::now(), + Utc::now(), + Some(manifest_path.to_string()), + &tenant + ) + .await + .unwrap() + .is_none() + ); + } + } + #[tokio::test] async fn remote_schema_refresh_publishes_only_changes() { use crate::{event::commit_schema_to_metadata, metadata::LogStreamMetadata}; diff --git a/src/query/mod.rs b/src/query/mod.rs index 059cd6a40..784686324 100644 --- a/src/query/mod.rs +++ b/src/query/mod.rs @@ -1713,3 +1713,5 @@ mod tests { assert_eq!(val, out); } } + +mod small_file_cache; diff --git a/src/query/small_file_cache.rs b/src/query/small_file_cache.rs new file mode 100644 index 000000000..600caef02 --- /dev/null +++ b/src/query/small_file_cache.rs @@ -0,0 +1,751 @@ +/* + * Parseable Server (C) 2022 - 2025 Parseable, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +//! Validated small-file bodies. Each physical scan validates before serving +//! bytes, so remote overwrites and deletes are visible without a TTL. Cache +//! scope identifies the configured provider across per-query runtime instances. +use bytes::Bytes; +use datafusion::{ + datasource::{ + listing::PartitionedFile, + physical_plan::parquet::{DefaultParquetFileReaderFactory, ParquetFileReaderFactory}, + }, + physical_plan::metrics::ExecutionPlanMetricsSet, +}; +use futures_util::{FutureExt, future::BoxFuture}; +use object_store::{GetOptions, GetRange, ObjectStore, path::Path}; +use parquet::{ + arrow::{arrow_reader::ArrowReaderOptions, async_reader::AsyncFileReader}, + errors::{ParquetError, Result}, + file::metadata::{ParquetMetaData, ParquetMetaDataReader}, +}; +use std::{ + collections::VecDeque, + ops::Range, + sync::{Arc, LazyLock, Mutex}, +}; + +const MAX_FILE: u64 = 256 * 1024; +const MAX_BYTES: usize = 64 * 1024 * 1024; +const SCAN_BYTES: usize = 8 * 1024 * 1024; +const MAX_ENTRIES: usize = 4096; +#[derive(Debug, Clone)] +struct Entry { + scope: usize, + path: Path, + etag: String, + bytes: Bytes, +} +#[derive(Debug, Default)] +struct Cache { + entries: VecDeque, + bytes: usize, +} +impl Cache { + fn remove(&mut self, scope: usize, path: &Path) -> Option { + let i = self + .entries + .iter() + .position(|e| e.scope == scope && &e.path == path)?; + let entry = self.entries.remove(i)?; + self.bytes -= entry.bytes.len(); + Some(entry) + } + fn insert(&mut self, entry: Entry, budget: usize) { + self.remove(entry.scope, &entry.path); + if entry.bytes.len() > budget { + return; + } + while self.bytes + entry.bytes.len() > budget || self.entries.len() >= MAX_ENTRIES { + let old = self.entries.pop_front().expect("nonempty cache"); + self.bytes -= old.bytes.len(); + } + self.bytes += entry.bytes.len(); + self.entries.push_back(entry); + } +} +static CACHE: LazyLock> = LazyLock::new(Mutex::default); + +async fn load( + cache: &Mutex, + scope: usize, + store: &Arc, + path: &Path, +) -> object_store::Result<(u64, Option)> { + // Keep a private reference during validation; eviction cannot lose the body + // needed for a 304. Do not hold the cache lock across network I/O. + let old = { + let mut cache = cache.lock().unwrap(); + let old = cache.remove(scope, path); + if let Some(entry) = old.clone() { + cache.insert(entry, MAX_BYTES); + } + old + }; + let response = store + .get_opts( + path, + GetOptions { + if_none_match: old.as_ref().map(|e| e.etag.clone()), + range: Some(GetRange::Bounded(0..MAX_FILE + 1)), + ..Default::default() + }, + ) + .await; + match response { + Err(object_store::Error::NotModified { .. }) if old.is_some() => { + let bytes = old.unwrap().bytes; + Ok((bytes.len() as u64, Some(bytes))) + } + Ok(response) => { + let size = response.meta.size; + let etag = response.meta.e_tag.clone(); + if size > MAX_FILE { + cache.lock().unwrap().remove(scope, path); + return Ok((size, None)); + } + let bytes = response.bytes().await?; + let mut cache = cache.lock().unwrap(); + cache.remove(scope, path); + if let Some(etag) = etag { + cache.insert( + Entry { + scope, + path: path.clone(), + etag, + bytes: bytes.clone(), + }, + MAX_BYTES, + ); + } + Ok((size, Some(bytes))) + } + Err(error) => { + cache.lock().unwrap().remove(scope, path); + Err(error) + } + } +} + +// Metadata and data readers of one physical scan share a bounded view. A new +// physical scan always validates against storage; it cannot inherit this view. +async fn load_for_scan( + view: &Mutex, + scope: usize, + store: &Arc, + path: &Path, +) -> object_store::Result<(u64, Option)> { + { + let mut view = view.lock().unwrap(); + if let Some(entry) = view.remove(scope, path) { + let bytes = entry.bytes.clone(); + view.insert(entry, SCAN_BYTES); + return Ok((bytes.len() as u64, Some(bytes))); + } + } + let result = load(&CACHE, scope, store, path).await?; + if let Some(bytes) = &result.1 { + view.lock().unwrap().insert( + Entry { + scope, + path: path.clone(), + etag: String::new(), + bytes: bytes.clone(), + }, + SCAN_BYTES, + ); + } + Ok(result) +} + +pub(super) fn install( + plan: Arc, + store: Arc, + scope: usize, +) -> datafusion::common::Result> { + use datafusion::datasource::{ + physical_plan::{FileScanConfigBuilder, ParquetSource}, + source::DataSourceExec, + }; + let exec = plan + .downcast_ref::() + .expect("ParquetFormat returns DataSourceExec"); + let (conf, source) = exec + .downcast_to_file_source::() + .expect("parquet source"); + let fallback = source + .parquet_file_reader_factory() + .cloned() + .unwrap_or_else(|| Arc::new(DefaultParquetFileReaderFactory::new(store.clone()))); + let source = + source + .clone() + .with_parquet_file_reader_factory(Arc::new(SmallFileReaderFactory { + store, + fallback, + scope, + view: Arc::new(Mutex::default()), + })); + Ok(Arc::new( + exec.clone().with_data_source(Arc::new( + FileScanConfigBuilder::from(conf.clone()) + .with_source(Arc::new(source)) + .build(), + )), + )) +} + +#[derive(Debug)] +pub(super) struct SmallFileReaderFactory { + view: Arc>, + scope: usize, + pub store: Arc, + fallback: Arc, +} +impl ParquetFileReaderFactory for SmallFileReaderFactory { + fn create_reader( + &self, + partition: usize, + file: PartitionedFile, + hint: Option, + metrics: &ExecutionPlanMetricsSet, + ) -> datafusion::common::Result> { + let fallback = self + .fallback + .create_reader(partition, file.clone(), hint, metrics)?; + if file.object_meta.size > MAX_FILE { + return Ok(fallback); + } + let file_metrics = datafusion::datasource::physical_plan::ParquetFileMetrics::new( + partition, + file.object_meta.location.as_ref(), + metrics, + ); + Ok(Box::new(Reader { + file_metrics, + hint, + view: self.view.clone(), + store: self.store.clone(), + scope: self.scope, + file, + partition, + metrics: metrics.clone(), + fallback, + factory: self.fallback.clone(), + initialized: false, + bytes: None, + })) + } +} +struct Reader { + hint: Option, + view: Arc>, + file_metrics: datafusion::datasource::physical_plan::ParquetFileMetrics, + scope: usize, + factory: Arc, + store: Arc, + file: PartitionedFile, + partition: usize, + metrics: ExecutionPlanMetricsSet, + fallback: Box, + initialized: bool, + bytes: Option, +} +impl Reader { + async fn init(&mut self) -> Result<()> { + if !self.initialized { + let (size, bytes) = load_for_scan( + &self.view, + self.scope, + &self.store, + &self.file.object_meta.location, + ) + .await + .map_err(|e| ParquetError::External(Box::new(e)))?; + if bytes.is_none() { + self.file.object_meta.size = size; + self.fallback = self + .factory + .create_reader(self.partition, self.file.clone(), self.hint, &self.metrics) + .map_err(|e| ParquetError::External(Box::new(e)))?; + } + self.bytes = bytes; + self.initialized = true; + } + Ok(()) + } +} +impl AsyncFileReader for Reader { + fn get_bytes(&mut self, range: Range) -> BoxFuture<'_, Result> { + async move { + self.init().await?; + if let Some(bytes) = &self.bytes { + if range.start > range.end || range.end > bytes.len() as u64 { + return Err(ParquetError::General("Invalid parquet byte range".into())); + } + self.file_metrics + .bytes_scanned + .add((range.end - range.start) as usize); + Ok(bytes.slice(range.start as usize..range.end as usize)) + } else { + self.fallback.get_bytes(range).await + } + } + .boxed() + } + fn get_byte_ranges(&mut self, ranges: Vec>) -> BoxFuture<'_, Result>> { + async move { + self.init().await?; + if self.bytes.is_none() { + return self.fallback.get_byte_ranges(ranges).await; + } + let mut out = Vec::with_capacity(ranges.len()); + for range in ranges { + out.push(self.get_bytes(range).await?); + } + Ok(out) + } + .boxed() + } + fn get_metadata<'a>( + &'a mut self, + options: Option<&'a ArrowReaderOptions>, + ) -> BoxFuture<'a, Result>> { + async move { + self.init().await?; + if let Some(bytes) = &self.bytes { + let size = bytes.len() as u64; + Ok(Arc::new( + ParquetMetaDataReader::new() + .with_arrow_reader_options(options) + .load_and_finish(self, size) + .await?, + )) + } else { + self.fallback.get_metadata(options).await + } + } + .boxed() + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::storage::{StorageMetricWindow, metrics_layer::MetricLayer}; + use object_store::{ObjectStoreExt, memory::InMemory}; + + #[tokio::test] + async fn hit_miss_overwrite_delete_recreate_and_store_isolation() { + let store: Arc = + Arc::new(MetricLayer::new(InMemory::new(), "small-cache-test")); + let cache = Mutex::new(Cache::default()); + let path = Path::from("tenant/stream/file.parquet"); + store + .put(&path, Bytes::from_static(b"old").into()) + .await + .unwrap(); + let window = StorageMetricWindow::start("small-cache-test"); + assert_eq!( + load(&cache, 1, &store, &path).await.unwrap().1.unwrap(), + "old" + ); + assert_eq!( + load(&cache, 1, &store, &path).await.unwrap().1.unwrap(), + "old" + ); + assert_eq!(window.delta().get_requests, 2, "one validation per reader"); + assert_eq!( + crate::metrics::STORAGE_REQUEST_RESPONSE_TIME + .with_label_values(&["small-cache-test", "GET", "304"]) + .get_sample_count(), + 1 + ); + assert_eq!(cache.lock().unwrap().entries.len(), 1); + store + .put(&path, Bytes::from_static(b"new").into()) + .await + .unwrap(); + assert_eq!( + load(&cache, 1, &store, &path).await.unwrap().1.unwrap(), + "new" + ); + store.delete(&path).await.unwrap(); + assert!(load(&cache, 1, &store, &path).await.is_err()); + assert!(cache.lock().unwrap().entries.is_empty()); + store + .put(&path, Bytes::from_static(b"reborn").into()) + .await + .unwrap(); + assert_eq!( + load(&cache, 1, &store, &path).await.unwrap().1.unwrap(), + "reborn" + ); + let other: Arc = Arc::new(InMemory::new()); + assert!(load(&cache, 2, &other, &path).await.is_err()); + store + .put(&path, Bytes::from(vec![0; MAX_FILE as usize + 1]).into()) + .await + .unwrap(); + assert!(load(&cache, 1, &store, &path).await.unwrap().1.is_none()); + assert!(cache.lock().unwrap().entries.is_empty()); + } + + #[tokio::test] + async fn scan_view_is_bounded_and_new_scan_validates_changes() { + let store: Arc = Arc::new(InMemory::new()); + let scope = 99881; + let path = Path::from("view/file.parquet"); + let view = Mutex::new(Cache::default()); + store + .put(&path, Bytes::from_static(b"one").into()) + .await + .unwrap(); + assert_eq!( + load_for_scan(&view, scope, &store, &path) + .await + .unwrap() + .1 + .unwrap(), + "one" + ); + store + .put(&path, Bytes::from_static(b"two").into()) + .await + .unwrap(); + assert_eq!( + load_for_scan(&view, scope, &store, &path) + .await + .unwrap() + .1 + .unwrap(), + "one", + "one scan retains its validated body" + ); + let next = Mutex::new(Cache::default()); + assert_eq!( + load_for_scan(&next, scope, &store, &path) + .await + .unwrap() + .1 + .unwrap(), + "two", + "new scan must validate" + ); + store.delete(&path).await.unwrap(); + let next = Mutex::new(Cache::default()); + assert!(load_for_scan(&next, scope, &store, &path).await.is_err()); + let mut view = view.lock().unwrap(); + for i in 0..40 { + view.insert( + Entry { + scope, + path: Path::from(format!("{i}")), + etag: String::new(), + bytes: Bytes::from(vec![0; MAX_FILE as usize]), + }, + SCAN_BYTES, + ); + } + assert!(view.bytes <= SCAN_BYTES); + assert!(view.remove(scope, &path).is_none()); + } + + #[test] + fn bounds_eviction_and_replacement() { + let mut cache = Cache::default(); + let entry = |name: &str, len| Entry { + scope: 1, + path: Path::from(name), + etag: "v1".into(), + bytes: Bytes::from(vec![0; len]), + }; + cache.insert(entry("a", 3), 5); + cache.insert(entry("b", 3), 5); + assert!(cache.remove(1, &Path::from("a")).is_none()); + assert_eq!(cache.bytes, 3); + cache.insert(entry("b", 2), 5); + assert_eq!(cache.bytes, 2); + cache.insert(entry("huge", 6), 5); + assert_eq!(cache.bytes, 2); + for i in 0..MAX_ENTRIES + 1 { + cache.insert(entry(&i.to_string(), 0), 5); + } + assert_eq!(cache.entries.len(), MAX_ENTRIES); + assert!(cache.remove(1, &Path::from("0")).is_none()); + } +} + +#[cfg(test)] +mod measurement { + use super::*; + use crate::{ + promql::{self, QueryPlan, Sample, Series}, + query::stream_schema_provider::build_parquet_scan_components, + storage::{StorageMetricWindow, metrics_layer::MetricLayer}, + }; + use arrow_array::{Array, Float64Array, RecordBatch, StringArray, TimestampMillisecondArray}; + use arrow_schema::{DataType, Field, Schema, SchemaRef, TimeUnit}; + use datafusion::{ + catalog::Session, + datasource::{ + TableProvider, + file_format::FileFormat, + physical_plan::{FileGroup, FileScanConfigBuilder}, + }, + execution::object_store::ObjectStoreUrl, + logical_expr::TableType, + physical_plan::ExecutionPlan, + prelude::{Expr, SessionConfig, SessionContext}, + }; + use object_store::{ObjectStoreExt, memory::InMemory}; + use parquet::arrow::ArrowWriter; + + #[derive(Debug)] + struct Table { + schema: SchemaRef, + files: Vec, + cached: bool, + } + #[async_trait::async_trait] + impl TableProvider for Table { + fn schema(&self) -> SchemaRef { + self.schema.clone() + } + fn table_type(&self) -> TableType { + TableType::Base + } + async fn scan( + &self, + state: &dyn Session, + projection: Option<&Vec>, + filters: &[Expr], + limit: Option, + ) -> datafusion::common::Result> { + let url = ObjectStoreUrl::parse("s3://cache-measurement").unwrap(); + let (format, source) = + build_parquet_scan_components(self.schema.clone(), filters, state)?; + let mut builder = FileScanConfigBuilder::new(url.clone(), source.into()) + .with_file_groups( + self.files + .chunks(15) + .map(|files| FileGroup::new(files.to_vec())) + .collect(), + ) + .with_limit(limit); + if let Some(p) = projection { + builder = builder.with_projection_indices(Some(p.clone()))?; + } + let plan = format.create_physical_plan(state, builder.build()).await?; + if self.cached { + install(plan, state.runtime_env().object_store(url)?, 12345) + } else { + Ok(plan) + } + } + } + + // This uses the production ParquetFormat, scan components, metrics layer, + // and PromQL evaluator. It isolates file I/O from HTTP admission and mutable + // catalog reads; those costs must not be presented as measured here. + #[tokio::test] + async fn repeated_sixty_file_queries() { + let schema = Arc::new(Schema::new(vec![ + Field::new( + "p_timestamp", + DataType::Timestamp(TimeUnit::Millisecond, None), + false, + ), + Field::new( + "time_unix_nano", + DataType::Timestamp(TimeUnit::Millisecond, None), + false, + ), + Field::new("metric_name", DataType::Utf8, false), + Field::new("job", DataType::Utf8, false), + Field::new("data_point_value", DataType::Float64, false), + ])); + let backend: Arc = + if let Ok(endpoint) = std::env::var("P_CACHE_TEST_MINIO_ENDPOINT") { + Arc::new( + object_store::aws::AmazonS3Builder::new() + .with_endpoint(endpoint) + .with_allow_http(true) + .with_region("us-east-1") + .with_bucket_name("cache-measurement") + .with_access_key_id("minioadmin") + .with_secret_access_key("minioadmin") + .build() + .unwrap(), + ) + } else { + Arc::new(InMemory::new()) + }; + let store: Arc = Arc::new(MetricLayer::new(backend, "cache-measurement")); + let mut files = Vec::new(); + for minute in 0..60 { + let times: Vec = (0..480).map(|i| minute * 60_000 + i * 125).collect(); + let batch = RecordBatch::try_new( + schema.clone(), + vec![ + Arc::new(TimestampMillisecondArray::from(times.clone())), + Arc::new(TimestampMillisecondArray::from(times)), + Arc::new(StringArray::from(vec!["requests"; 480])), + Arc::new(StringArray::from( + (0..480) + .map(|i| if i % 2 == 0 { "api" } else { "worker" }) + .collect::>(), + )), + Arc::new(Float64Array::from( + (0..480) + .map(|i| (minute * 480 + i) as f64) + .collect::>(), + )), + ], + ) + .unwrap(); + let mut body = Vec::new(); + let mut writer = ArrowWriter::try_new(&mut body, schema.clone(), None).unwrap(); + writer.write(&batch).unwrap(); + writer.close().unwrap(); + let path = format!("metrics/{minute}.parquet"); + files.push(PartitionedFile::new(path.clone(), body.len() as u64)); + store + .put(&Path::from(path), Bytes::from(body).into()) + .await + .unwrap(); + } + println!( + "CACHE_MEASUREMENT stored_bytes={} files={}", + files.iter().map(|file| file.object_meta.size).sum::(), + files.len() + ); + let queries = [ + ("count", "SELECT COUNT(*) FROM metrics"), + ( + "group", + "SELECT job, SUM(data_point_value) FROM metrics WHERE data_point_value > 100 GROUP BY job ORDER BY job", + ), + ( + "promql", + "SELECT time_unix_nano, data_point_value FROM metrics WHERE metric_name = 'requests' AND job = 'api' ORDER BY time_unix_nano", + ), + ]; + for (name, sql) in queries { + let mut reference = None; + for cached in [false, true] { + if cached { + let mut cache = CACHE.lock().unwrap(); + for file in &files { + cache.remove(12345, &file.object_meta.location); + } + } + for run in 0..2 { + // Production builds a new query session; the process cache + // survives these sessions, DataFusion's session cache doesn't. + let mut config = SessionConfig::new().with_target_partitions(4); + config.options_mut().execution.parquet.metadata_size_hint = + Some(2 * 1024 * 1024); + config.options_mut().execution.parquet.enable_page_index = true; + config.options_mut().execution.parquet.pushdown_filters = true; + let ctx = SessionContext::new_with_config(config); + ctx.runtime_env().register_object_store( + &url::Url::parse("s3://cache-measurement").unwrap(), + store.clone(), + ); + ctx.register_table( + "metrics", + Arc::new(Table { + schema: schema.clone(), + files: files.clone(), + cached, + }), + ) + .unwrap(); + let not_modified = || { + crate::metrics::STORAGE_REQUEST_RESPONSE_TIME + .with_label_values(&["cache-measurement", "GET", "304"]) + .get_sample_count() + }; + let before_304 = not_modified(); + let window = StorageMetricWindow::start("cache-measurement"); + let batches = ctx.sql(sql).await.unwrap().collect().await.unwrap(); + let result = arrow::util::pretty::pretty_format_batches(&batches) + .unwrap() + .to_string(); + if let Some(reference) = &reference { + assert_eq!(&result, reference); + } else { + reference = Some(result); + } + if name == "promql" { + let mut samples = Vec::new(); + for batch in &batches { + let times = batch + .column(0) + .as_any() + .downcast_ref::() + .unwrap(); + let values = batch + .column(1) + .as_any() + .downcast_ref::() + .unwrap(); + for i in 0..times.len() { + samples.push(Sample { + timestamp_ms: times.value(i), + value: values.value(i), + }); + } + } + let series = Series { + labels: [ + ("__name__".into(), "requests".into()), + ("job".into(), "api".into()), + ] + .into(), + samples, + }; + let plan = + QueryPlan::parse("sum(rate(requests{job=\"api\"}[5m]))").unwrap(); + let result = + promql::evaluate_range(&plan, &[series], 300_000, 3_540_000, 60_000) + .unwrap(); + assert!(!result.into_json().to_string().is_empty()); + } + let delta = window.delta(); + println!( + "CACHE_MEASUREMENT {name} cached={cached} run={run} GET={} GET_RANGES={}", + delta.get_requests, delta.get_ranges_requests + ); + if cached { + if run == 1 { + assert_eq!( + not_modified() - before_304, + 60, + "warm scans validate with 304" + ); + } + assert_eq!(delta.get_ranges_requests, 0); + assert_eq!(delta.get_requests, 60); + } + } + } + } + } +} diff --git a/src/query/stream_schema_provider.rs b/src/query/stream_schema_provider.rs index c4baf009d..a5184add4 100644 --- a/src/query/stream_schema_provider.rs +++ b/src/query/stream_schema_provider.rs @@ -364,6 +364,11 @@ async fn parquet_scan_plan( batch_size: usize, ) -> Result, DataFusionError> { let (file_format, file_source) = build_parquet_scan_components(schema, filters, state)?; + let cache_store = if object_store_url.as_str() != "file:///" { + Some(state.runtime_env().object_store(&object_store_url)?) + } else { + None + }; // create file groups from vec file partitions let file_groups = partitions.into_iter().map(FileGroup::new).collect_vec(); @@ -387,13 +392,45 @@ async fn parquet_scan_plan( let conf = conf_builder.build(); // create the execution plan - file_format + let plan = file_format .create_physical_plan( state, // .as_any().downcast_ref::().unwrap(), // Remove this when ParquetFormat catches up conf, ) - .await + .await?; + if let Some(store) = cache_store { + super::small_file_cache::install( + plan, + store, + Arc::as_ptr(&PARSEABLE.storage) as *const () as usize, + ) + } else { + Ok(plan) + } +} + +// A snapshot used with staging must be fetched after staging is leased. If +// remote settings newly require staging, discard the first snapshot and retry. +async fn capture_before_snapshot( + predicted: bool, + mut capture: C, + mut load: L, + includes_staging: I, +) -> DataFusionResult<(M, Option)> +where + C: FnMut() -> DataFusionResult, + L: FnMut(bool) -> F, + F: std::future::Future>, + I: Fn(&M) -> bool, +{ + let mut captured = if predicted { Some(capture()?) } else { None }; + let mut metadata = load(false).await?; + if !predicted && includes_staging(&metadata) { + captured = Some(capture()?); + metadata = load(true).await?; + } + Ok((metadata, captured)) } impl StandardTableProvider { @@ -882,22 +919,13 @@ impl TableProvider for StandardTableProvider { .map_err(|e| DataFusionError::Plan(e.to_string())) }; - let object_store_format = if let Some(cache) = cache { - cache.format.get_or_try_init(load_format).await?.clone() - } else { - load_format().await? - }; - - let time_partition = object_store_format.time_partition; - let mut time_filters = extract_primary_filter(filters, &time_partition); - let include_staging = is_within_staging_window(&time_filters); - // Capture before reading the snapshot used by this scan. Upload publishes - // the manifest before unlinking staging: either source will then survive. - let (staged_records, staged_files) = if include_staging { + // Capture before the first remote read. Resident settings normally match + // remote settings; if they disagree about staging, capture and re-read. + // Publication always precedes staging unlink, so this order cannot hide + // a file uploaded while the query is being planned. + let capture = || -> DataFusionResult<_> { match PARSEABLE.get_stream(&self.stream, &self.tenant_id) { Ok(staging) => { - // Capture memory and leased files under the publication lock, - // before reading the refreshed remote snapshot. let (records, files) = staging .capture_staging(&self.schema, capture_staging_parquet) .map_err(|error| { @@ -905,13 +933,29 @@ impl TableProvider for StandardTableProvider { "Cannot capture staging record batches: {error}" )) })?; - (records, files?) + Ok((records, files?)) } - Err(_) => (Vec::new(), Vec::new()), + Err(_) => Ok((Vec::new(), Vec::new())), } - } else { - (Vec::new(), Vec::new()) }; + let captured_before_read = + is_within_staging_window(&extract_primary_filter(filters, &self.time_partition)); + let read_format = |refresh: bool| async move { + if let Some(cache) = cache.filter(|_| !refresh) { + cache.format.get_or_try_init(load_format).await.cloned() + } else { + load_format().await + } + }; + let (object_store_format, captured) = + capture_before_snapshot(captured_before_read, capture, read_format, |format| { + is_within_staging_window(&extract_primary_filter(filters, &format.time_partition)) + }) + .await?; + let (staged_records, staged_files) = captured.unwrap_or_default(); + let time_partition = object_store_format.time_partition; + let mut time_filters = extract_primary_filter(filters, &time_partition); + let include_staging = is_within_staging_window(&time_filters); let mut merged_snapshot = Snapshot::default(); if PARSEABLE.options.mode == Mode::Query || PARSEABLE.options.mode == Mode::Prism { let objects = PARSEABLE @@ -926,18 +970,6 @@ impl TableProvider for StandardTableProvider { .manifest_list .extend(format.snapshot.manifest_list); } - } else if include_staging { - // The first read supplies stream settings only; its snapshot may - // predate the staging capture and must not be used for this scan. - let refreshed: ObjectStoreFormat = serde_json::from_slice( - &PARSEABLE - .metastore - .get_stream_json(&self.stream, false, &self.tenant_id, false) - .await - .map_err(|e| DataFusionError::Plan(e.to_string()))?, - ) - .map_err(|e| DataFusionError::Plan(e.to_string()))?; - merged_snapshot = refreshed.snapshot; } else { merged_snapshot = object_store_format.snapshot; } @@ -2425,3 +2457,50 @@ mod ordering_tests; #[cfg(test)] mod exact_filter_tests; + +#[cfg(test)] +mod snapshot_capture_tests { + use super::*; + use std::sync::atomic::{AtomicUsize, Ordering}; + + #[tokio::test] + async fn publication_between_capture_and_read_cannot_hide_a_file() { + for predicted in [true, false] { + let published = AtomicUsize::new(0); + let reads = AtomicUsize::new(0); + let (snapshot, staging) = capture_before_snapshot( + predicted, + || { + // Capture a lease, then simulate publication and unlink. + let lease = "just-uploaded.parquet"; + published.store(1, Ordering::SeqCst); + Ok(lease) + }, + |_| async { + reads.fetch_add(1, Ordering::SeqCst); + Ok(published.load(Ordering::SeqCst)) + }, + |_| true, + ) + .await + .unwrap(); + assert_eq!(snapshot, 1); + assert_eq!(staging, Some("just-uploaded.parquet")); + assert_eq!(reads.load(Ordering::SeqCst), if predicted { 1 } else { 2 }); + } + } + + #[tokio::test] + async fn historical_queries_do_not_capture_staging() { + let (snapshot, staging) = capture_before_snapshot( + false, + || -> DataFusionResult<()> { panic!("historical scan must not lease staging") }, + |_| async { Ok(42) }, + |_| false, + ) + .await + .unwrap(); + assert_eq!(snapshot, 42); + assert_eq!(staging, None); + } +} diff --git a/src/storage/mod.rs b/src/storage/mod.rs index c376a487f..647723762 100644 --- a/src/storage/mod.rs +++ b/src/storage/mod.rs @@ -45,7 +45,7 @@ mod gcs; mod localfs; #[cfg(test)] pub(crate) use localfs::LocalFS; -mod metrics_layer; +pub(crate) mod metrics_layer; pub use metrics_layer::{StorageMetricDelta, StorageMetricWindow, storage_metrics_provider_label}; pub mod object_storage; pub mod retention; From 7301bbfcd8e47ddffceb25680a51315b7c692fa9 Mon Sep 17 00:00:00 2001 From: VibeSWE <338779468+VibeSWE@users.noreply.github.com> Date: Tue, 10 Nov 2026 23:01:52 -0600 Subject: [PATCH 2/5] fix: preserve query accounting and harden validated small-file cache --- .../performance/validated-small-file-cache.md | 101 ++- src/cli.rs | 11 + .../metastores/object_store_metastore.rs | 99 ++- src/metrics/mod.rs | 2 +- src/query/cache_http_proxy.py | 54 ++ src/query/small_file_cache.rs | 593 ++++++++++++++++-- src/query/stream_schema_provider.rs | 28 +- src/storage/metrics_layer.rs | 125 +++- 8 files changed, 868 insertions(+), 145 deletions(-) create mode 100644 src/query/cache_http_proxy.py diff --git a/docs/performance/validated-small-file-cache.md b/docs/performance/validated-small-file-cache.md index a7280b676..92b7f7f9c 100644 --- a/docs/performance/validated-small-file-cache.md +++ b/docs/performance/validated-small-file-cache.md @@ -7,8 +7,10 @@ bucket paths and formats, and adds no required configuration or TTL. `upload_single_parquet_file` uses unconditional multipart upload, followed by a size check and manifest publication. Staging conversion normally incorporates a -ULID (`src/parseable/staging/writer.rs`), but crash recovery can retry a remote -path. The storage API doesn't enforce immutable parquet objects. Retention also +ULID (`src/parseable/staging/writer.rs`). Crash recovery retries the preserved +file; it is not evidence of different bytes at the same path. Uploads are +unconditional, however, so any writer with bucket access can overwrite a path. +The storage API does not enforce immutable parquet objects. Retention also deletes them. A pathname and manifest size alone therefore aren't a safe cache identity. The metadata conditional-write API doesn't make parquet uploads, stream snapshots or manifests immutable; those writers still use ordinary writes. @@ -25,7 +27,8 @@ changes the scan's snapshot ordering without restoring per-query stream reloads: lease staging before the first settings/snapshot GET. If remote settings show that staging is needed when resident settings predicted otherwise, lease it and fetch a fresh snapshot. Upload publishes before staging is unlinked, so either -the lease or remote manifest retains the file. Historical scans don't need leases. +the lease or remote manifest retains the file. Historical scans don't need leases. A speculative capture error is held until +remote settings confirm staging is needed; otherwise it is discarded. ## Small parquet files @@ -40,16 +43,17 @@ one bounded GET. It holds those bytes for its lifetime; metadata, page indexes, and column reads then use slices of the same body. On a subsequent scan, a conditional GET validates the cached ETag. A 304 reuses the bytes without a body; a changed object replaces them. A 404/error invalidates the entry and propagates. -No ETag means no cross-reader caching. Growth past the limit uses the original +Absent or weak (`W/`) ETags mean no cross-scan body reuse. Growth past the limit uses the original reader with the newly observed size. The bounded probe prevents an unexpected large replacement from being read into memory in full. -Keys use the process's singleton storage-provider identity and full object path, +Keys use the process's singleton storage-provider identity, object-store URL, and full object path, including tenant prefixes. Per-query object-store clients differ, so their pointer identity would prevent cross-query hits. The configured provider lives for the process lifetime. A physical scan also has an 8 MiB LRU view shared between DataFusion metadata and data readers, avoiding a second validation of the same -body within that scan. New scans never inherit that view. LRU storage is capped at 64 MiB and 4,096 entries. Active readers +body within that scan. New scans never inherit that view. The keyed linked-list LRU performs lookup, promotion, and removal in O(1), +without holding its mutex across I/O. Storage defaults to 64 MiB and 4,096 entries. Active readers can retain evicted bodies; as with existing scan buffers, their memory also depends on query concurrency. This limit is a cache residency limit, not a process RSS cap. @@ -83,7 +87,7 @@ Build locally with `PATH=$HOME/.cargo/bin:$PATH`, `P_BUILD_NO_UI=1`, and ``` cargo test --lib query::small_file_cache -- --nocapture -P_CACHE_TEST_MINIO_ENDPOINT=http://127.0.0.1:18390 cargo test --lib repeated_sixty_file_queries -- --nocapture +P_CACHE_TEST_MINIO_ENDPOINT=http://127.0.0.1:18390 cargo test --lib query::small_file_cache -- --ignored --nocapture cargo test --lib snapshot_capture_tests cargo test --lib next_metadata_read_sees_external_writer_changes cargo test --lib staging_lease @@ -117,16 +121,73 @@ loses one stream.json GET through the capture-order change, but that reduction i code-derived, not included in this measured table. Dataset discovery also keeps its LIST count while allowing concurrent probes. No local wall-time claim is made. -The existing MetricLayer byte counter charges requested bounded-range lengths, -including unsuccessful/304 requests. This harness deliberately reports request -counts, not inferred transferred bytes from that counter. - -Validation: the full library suite passed (1,382 passed, 4 ignored). The final -MinIO run passed all four cache/measurement tests, including an assertion that -each warm query receives 60 HTTP 304 responses. Fresh-cache and repeated-query -request counts were identical; warm responses carry no parquet body. -Plain `cargo clippy --all-targets` passed with existing warnings only; no warning -points to the changed cache, scan, or metastore code. No full HTTP dashboard -benchmark was performed: catalog, schema-refresh and handler overhead remain an -explicit validation gap. MinIO and in-memory storage were exercised; Azure/GCS -were checked through the shared object_store API and were not emulator-tested. +The MetricLayer byte counter now counts successful response body chunks and +successful multi-range result lengths, rather than requested range lengths. +304 responses have their own `not_modified_requests` delta and do not count as +failures or transferred bytes. Query `bytes_scanned` continues to count data +ranges only; footer/page-index loading does not add to this billing-style total. + +## Configuration and admission + +`--small-file-cache-enabled` / `P_SMALL_FILE_CACHE_ENABLED` defaults to `true`. +Setting it to `false` skips reader installation at `parquet_scan_plan`, retaining +the original DataFusion reader and its request pattern. +`--small-file-cache-size` / `P_SMALL_FILE_CACHE_SIZE` defaults to `67108864` bytes +(64 MiB); zero also disables installation. The per-scan view stays at 8 MiB and +the process cache entry cap stays at 4,096. These are residency limits, not RSS +limits. No new required configuration or dependency was added. + +A scan whose selected small-file manifest sizes exceed the configured byte limit, +or whose small-file count exceeds 4,096, bypasses installation entirely. It cannot +cycle through and evict the useful process working set. This conservative policy +uses manifest predictions; unexpectedly changed sizes are still validated. +Large-file reads retain the original reader. If metadata reading fails and HEAD +confirms the manifest size changed, the reader is rebuilt using the current size; +this covers shrinkage under a stale large manifest without adding requests to +successful large-file reads. A non-partial answer to the bounded cache probe +invalidates residency and falls back to the original reader after HEAD. + +## Test boundaries + +`repeated_sixty_file_queries` and `http_mutations_and_faults` require a real HTTP +S3-compatible endpoint in `P_CACHE_TEST_MINIO_ENDPOINT`, bucket +`cache-measurement`, credentials `minioadmin` / `minioadmin`. They are explicitly marked ignored, so a normal library run visibly reports +them as skipped with the HTTP requirement. Run them with `--ignored --nocapture`; +explicit execution without an endpoint fails instead of silently passing. The other cache, +storage metric, snapshot ordering, and bounded LIST tests need no HTTP service. +The HTTP mutation test additionally uses Python 3's standard-library fault proxy +on an ephemeral loopback port; the child is killed and waited on at test exit. +It covers exact parquet query rows after overwrite, deletion/recreation, growth +and shrinkage with current/stale sizes, 503 validation, interrupted bodies, weak +and missing ETags, and a 200 response to the bounded cache probe. The measurement +test compares original and cached query rows and `bytes_scanned`, cold and warm, +checks actual storage bytes and warm 304s, and checks evaluated PromQL values. +It does not exercise Azure/GCS emulators or the live dashboard service. + +## Fix validation (2026-11-10) + +Real MinIO HTTP on isolated loopback ports, plus the Python response fault proxy: +all six cache tests passed, including both explicitly enabled HTTP tests. The full +library suite passed: 1,385 passed, six ignored (including the two HTTP tests run +separately). The 60-file fixture again contained 894,420 bytes. Every cold cached +scan counted exactly that many storage body bytes; every warm cached scan counted +zero, with 60 not-modified outcomes and no storage failures. Billing-style +`bytes_scanned` matched the original reader for every query, cold and warm. +PromQL returned 55 evaluated points at 60-second intervals, each with rate 8. + +| Workload | Original reader GET / GET_RANGES | Reviewed cache (recorded) | Fixed cache cold | Fixed cache warm | Disabled cache GET / GET_RANGES | +| --- | ---: | ---: | ---: | ---: | ---: | +| COUNT(*) | 0 / 60 | 60 / 0 | 60 / 0 | 60 / 0 | 0 / 60 | +| Filtered GROUP BY | 1 / 180 | 60 / 0 | 60 / 0 | 60 / 0 | 1 / 180 | +| PromQL scan + range evaluation | 60 / 240 | 60 / 0 | 60 / 0 | 60 / 0 | 60 / 240 | + +The reviewed-cache column is the prior design document's measurement; the +original, fixed, and disabled columns were measured again during this fix. The +fixes add no round trips to these query shapes. Stale-size recovery and a store +rejecting the bounded probe can require fallback requests, as described above. + +Final plain `cargo clippy --all-targets` passed with existing warnings only +(21 library-test warnings and the existing staging API compatibility warning). +No warnings point to changed code. The temporary MinIO container and its volumes +were removed; the live side-by-side service, configuration, and bucket were not +used. No push or PR was performed. diff --git a/src/cli.rs b/src/cli.rs index 417a2a11a..2f1ea395d 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -496,6 +496,17 @@ pub struct Options { )] pub parquet_metadata_cache_size: usize, + #[arg(long, help = "Validate and cache small remote Parquet bodies", env = "P_SMALL_FILE_CACHE_ENABLED", default_value = "true", action = clap::ArgAction::Set)] + pub small_file_cache_enabled: bool, + + #[arg( + long, + help = "Maximum resident small-file body bytes (zero disables caching)", + env = "P_SMALL_FILE_CACHE_SIZE", + default_value = "67108864" + )] + pub small_file_cache_size: usize, + // reduced the max row group size from 1048576 // smaller row groups help in faster query performance in multi threaded query #[arg( diff --git a/src/metastore/metastores/object_store_metastore.rs b/src/metastore/metastores/object_store_metastore.rs index 1b1dff443..1f84868a1 100644 --- a/src/metastore/metastores/object_store_metastore.rs +++ b/src/metastore/metastores/object_store_metastore.rs @@ -1571,46 +1571,85 @@ impl Metastore for ObjectStoreMetastore { && name != TOMBSTONE_ROOT_DIRECTORY }) .collect::>(); - let results = - futures::stream::iter(streams) - .map(|stream| { - let root = root.clone(); - async move { - let stream_path = if let Some(root) = root.as_ref() { - object_store::path::Path::from_iter([ - root.as_ref(), - &stream, - STREAM_ROOT_DIRECTORY, - ]) - } else { - object_store::path::Path::from(format!( - "{}/{}", - stream, STREAM_ROOT_DIRECTORY - )) - }; - let resp = self.storage.list_with_delimiter(Some(stream_path)).await?; - if resp.objects.iter().any(|name| { - name.location.filename().unwrap().ends_with("stream.json") - }) { - Ok::<_, MetastoreError>(Some(stream)) - } else { - Ok(None) - } - } - }) - .buffer_unordered(16) - .try_collect::>() - .await?; + let results = collect_dataset_probes(streams.into_iter().map(|stream| { + let root = root.clone(); + async move { + let stream_path = if let Some(root) = root.as_ref() { + object_store::path::Path::from_iter([ + root.as_ref(), + &stream, + STREAM_ROOT_DIRECTORY, + ]) + } else { + object_store::path::Path::from(format!( + "{}/{}", + stream, STREAM_ROOT_DIRECTORY + )) + }; + let resp = self.storage.list_with_delimiter(Some(stream_path)).await?; + if resp + .objects + .iter() + .any(|name| name.location.filename().unwrap().ends_with("stream.json")) + { + Ok::<_, MetastoreError>(Some(stream)) + } else { + Ok(None) + } + } + })) + .await?; Ok(results.into_iter().flatten().collect()) } } } +// All-or-error collection: a failed probe cannot publish a partial inventory. +async fn collect_dataset_probes( + probes: impl IntoIterator>>, +) -> Result, E> { + futures::stream::iter(probes) + .buffer_unordered(16) + .try_collect() + .await +} + #[cfg(test)] mod tests { use super::*; use crate::storage::LocalFS; + #[tokio::test] + async fn dataset_probes_are_bounded_and_fail_atomically() { + use std::sync::atomic::{AtomicUsize, Ordering}; + for fail in [false, true] { + let active = AtomicUsize::new(0); + let peak = AtomicUsize::new(0); + let result = collect_dataset_probes((0..64).map(|i| { + let active = &active; + let peak = &peak; + async move { + let n = active.fetch_add(1, Ordering::SeqCst) + 1; + peak.fetch_max(n, Ordering::SeqCst); + tokio::time::sleep(std::time::Duration::from_millis(5)).await; + active.fetch_sub(1, Ordering::SeqCst); + if fail && i == 25 { + Err("failed probe") + } else { + Ok(i) + } + } + })) + .await; + assert_eq!(peak.load(Ordering::SeqCst), 16); + if fail { + assert_eq!(result, Err("failed probe")); + } else { + assert_eq!(result.unwrap().len(), 64); + } + } + } + #[tokio::test] async fn next_metadata_read_sees_external_writer_changes() { for tenant in [None, Some("tenant".to_owned())] { diff --git a/src/metrics/mod.rs b/src/metrics/mod.rs index e2848c33f..d1a8fc2cc 100644 --- a/src/metrics/mod.rs +++ b/src/metrics/mod.rs @@ -631,7 +631,7 @@ pub static STORAGE_READ_BYTES_TOTAL: Lazy = Lazy::new(|| { IntCounterVec::new( Opts::new( "storage_read_bytes_total", - "Logical bytes requested from an object store", + "Body bytes returned successfully by an object store", ) .namespace(METRICS_NAMESPACE), &["provider"], diff --git a/src/query/cache_http_proxy.py b/src/query/cache_http_proxy.py new file mode 100644 index 000000000..5f80801f0 --- /dev/null +++ b/src/query/cache_http_proxy.py @@ -0,0 +1,54 @@ +"""Test-only HTTP fault proxy; writes bypass this proxy and go directly to MinIO.""" +import http.client +import http.server +import pathlib +import sys +import urllib.parse + +upstream = urllib.parse.urlsplit(sys.argv[1]) +mode_file = pathlib.Path(sys.argv[2]) + +class Handler(http.server.BaseHTTPRequestHandler): + protocol_version = 'HTTP/1.1' + + def log_message(self, *_): + pass + + def do_HEAD(self): + self.do_GET() + + def do_GET(self): + mode = mode_file.read_text() + with mode_file.with_suffix('.requests').open('a') as log: + log.write(str(bool(self.headers.get('If-None-Match'))) + '\n') + if mode == 'fail' and self.headers.get('If-None-Match'): + self.send_response(503) + self.send_header('Content-Length', '0') + self.end_headers() + return + headers = {key.lower(): value for key, value in self.headers.items()} + full = mode == 'full' and self.headers.get('Range') == 'bytes=0-262144' + conn = http.client.HTTPConnection(upstream.hostname, upstream.port, timeout=10) + conn.request(self.command, self.path, headers=headers) + response = conn.getresponse() + body = response.read() + self.send_response(200 if full and response.status == 206 else response.status) + for key, value in response.getheaders(): + if key.lower() in ('connection', 'transfer-encoding') or (full and key.lower() == 'content-range'): + continue + if key.lower() == 'etag': + if mode == 'absent': + continue + if mode == 'weak': + value = 'W/' + value + self.send_header(key, value) + self.end_headers() + if self.command != 'HEAD': + self.wfile.write(body[:len(body)//2] if mode == 'broken' else body) + if mode == 'broken': + self.close_connection = True + conn.close() + +server = http.server.ThreadingHTTPServer(('127.0.0.1', 0), Handler) +print(server.server_port, flush=True) +server.serve_forever() diff --git a/src/query/small_file_cache.rs b/src/query/small_file_cache.rs index 600caef02..8586e1631 100644 --- a/src/query/small_file_cache.rs +++ b/src/query/small_file_cache.rs @@ -28,19 +28,20 @@ use datafusion::{ physical_plan::metrics::ExecutionPlanMetricsSet, }; use futures_util::{FutureExt, future::BoxFuture}; -use object_store::{GetOptions, GetRange, ObjectStore, path::Path}; +use object_store::{GetOptions, GetRange, ObjectStore, ObjectStoreExt, path::Path}; use parquet::{ arrow::{arrow_reader::ArrowReaderOptions, async_reader::AsyncFileReader}, errors::{ParquetError, Result}, file::metadata::{ParquetMetaData, ParquetMetaDataReader}, }; use std::{ - collections::VecDeque, + collections::HashMap, ops::Range, sync::{Arc, LazyLock, Mutex}, }; const MAX_FILE: u64 = 256 * 1024; +#[cfg(test)] const MAX_BYTES: usize = 64 * 1024 * 1024; const SCAN_BYTES: usize = 8 * 1024 * 1024; const MAX_ENTRIES: usize = 4096; @@ -51,20 +52,35 @@ struct Entry { etag: String, bytes: Bytes, } +type Key = (usize, Path); +#[derive(Debug)] +struct Node { + entry: Entry, + prev: Option, + next: Option, +} #[derive(Debug, Default)] struct Cache { - entries: VecDeque, + entries: HashMap, + first: Option, + last: Option, bytes: usize, } impl Cache { fn remove(&mut self, scope: usize, path: &Path) -> Option { - let i = self - .entries - .iter() - .position(|e| e.scope == scope && &e.path == path)?; - let entry = self.entries.remove(i)?; - self.bytes -= entry.bytes.len(); - Some(entry) + let node = self.entries.remove(&(scope, path.clone()))?; + if let Some(prev) = &node.prev { + self.entries.get_mut(prev).unwrap().next = node.next.clone(); + } else { + self.first = node.next.clone(); + } + if let Some(next) = &node.next { + self.entries.get_mut(next).unwrap().prev = node.prev.clone(); + } else { + self.last = node.prev.clone(); + } + self.bytes -= node.entry.bytes.len(); + Some(node.entry) } fn insert(&mut self, entry: Entry, budget: usize) { self.remove(entry.scope, &entry.path); @@ -72,11 +88,25 @@ impl Cache { return; } while self.bytes + entry.bytes.len() > budget || self.entries.len() >= MAX_ENTRIES { - let old = self.entries.pop_front().expect("nonempty cache"); - self.bytes -= old.bytes.len(); + let (scope, path) = self.first.clone().expect("nonempty cache"); + self.remove(scope, &path); + } + let key = (entry.scope, entry.path.clone()); + if let Some(last) = &self.last { + self.entries.get_mut(last).unwrap().next = Some(key.clone()); + } else { + self.first = Some(key.clone()); } self.bytes += entry.bytes.len(); - self.entries.push_back(entry); + self.entries.insert( + key.clone(), + Node { + entry, + prev: self.last.take(), + next: None, + }, + ); + self.last = Some(key); } } static CACHE: LazyLock> = LazyLock::new(Mutex::default); @@ -86,14 +116,17 @@ async fn load( scope: usize, store: &Arc, path: &Path, + namespace: &str, + budget: usize, ) -> object_store::Result<(u64, Option)> { + let key = Path::from(format!("{namespace}{}", path.as_ref())); // Keep a private reference during validation; eviction cannot lose the body // needed for a 304. Do not hold the cache lock across network I/O. let old = { let mut cache = cache.lock().unwrap(); - let old = cache.remove(scope, path); + let old = cache.remove(scope, &key); if let Some(entry) = old.clone() { - cache.insert(entry, MAX_BYTES); + cache.insert(entry, budget); } old }; @@ -116,32 +149,57 @@ async fn load( let size = response.meta.size; let etag = response.meta.e_tag.clone(); if size > MAX_FILE { - cache.lock().unwrap().remove(scope, path); + cache.lock().unwrap().remove(scope, &key); return Ok((size, None)); } - let bytes = response.bytes().await?; + let bytes = match response.bytes().await { + Ok(bytes) => bytes, + Err(error) => { + cache.lock().unwrap().remove(scope, &key); + return Err(error); + } + }; let mut cache = cache.lock().unwrap(); - cache.remove(scope, path); - if let Some(etag) = etag { + cache.remove(scope, &key); + if let Some(etag) = etag.filter(|tag| !tag.starts_with("W/")) { cache.insert( Entry { scope, - path: path.clone(), + path: key.clone(), etag, bytes: bytes.clone(), }, - MAX_BYTES, + budget, ); } Ok((size, Some(bytes))) } + Err(error) if non_partial(&error) => { + cache.lock().unwrap().remove(scope, &key); + // HEAD obtains the current size without trusting a stale manifest. + Ok((store.head(path).await?.size, None)) + } Err(error) => { - cache.lock().unwrap().remove(scope, path); + cache.lock().unwrap().remove(scope, &key); Err(error) } } } +fn non_partial(error: &object_store::Error) -> bool { + use std::error::Error; + let mut cause: &dyn Error = error; + loop { + if cause.to_string() == "Received non-partial response when range requested" { + return true; + } + match cause.source() { + Some(source) => cause = source, + None => return false, + } + } +} + // Metadata and data readers of one physical scan share a bounded view. A new // physical scan always validates against storage; it cannot inherit this view. async fn load_for_scan( @@ -149,21 +207,24 @@ async fn load_for_scan( scope: usize, store: &Arc, path: &Path, + namespace: &str, + budget: usize, ) -> object_store::Result<(u64, Option)> { + let key = Path::from(format!("{namespace}{}", path.as_ref())); { let mut view = view.lock().unwrap(); - if let Some(entry) = view.remove(scope, path) { + if let Some(entry) = view.remove(scope, &key) { let bytes = entry.bytes.clone(); view.insert(entry, SCAN_BYTES); return Ok((bytes.len() as u64, Some(bytes))); } } - let result = load(&CACHE, scope, store, path).await?; + let result = load(&CACHE, scope, store, path, namespace, budget).await?; if let Some(bytes) = &result.1 { view.lock().unwrap().insert( Entry { scope, - path: path.clone(), + path: key, etag: String::new(), bytes: bytes.clone(), }, @@ -177,7 +238,12 @@ pub(super) fn install( plan: Arc, store: Arc, scope: usize, + budget: usize, + enabled: bool, ) -> datafusion::common::Result> { + if !enabled || budget == 0 { + return Ok(plan); + } use datafusion::datasource::{ physical_plan::{FileScanConfigBuilder, ParquetSource}, source::DataSourceExec, @@ -188,6 +254,23 @@ pub(super) fn install( let (conf, source) = exec .downcast_to_file_source::() .expect("parquet source"); + // Bypass scans whose predicted working set cannot survive one traversal. + let small = conf + .file_groups + .iter() + .flat_map(|group| group.iter()) + .filter(|file| file.object_meta.size <= MAX_FILE) + .collect::>(); + if small.len() > MAX_ENTRIES + || small + .iter() + .map(|file| file.object_meta.size as u128) + .sum::() + > budget as u128 + { + return Ok(plan); + } + let namespace = conf.object_store_url.to_string(); let fallback = source .parquet_file_reader_factory() .cloned() @@ -199,6 +282,8 @@ pub(super) fn install( store, fallback, scope, + namespace, + budget, view: Arc::new(Mutex::default()), })); Ok(Arc::new( @@ -214,6 +299,8 @@ pub(super) fn install( pub(super) struct SmallFileReaderFactory { view: Arc>, scope: usize, + namespace: String, + budget: usize, pub store: Arc, fallback: Arc, } @@ -228,9 +315,6 @@ impl ParquetFileReaderFactory for SmallFileReaderFactory { let fallback = self .fallback .create_reader(partition, file.clone(), hint, metrics)?; - if file.object_meta.size > MAX_FILE { - return Ok(fallback); - } let file_metrics = datafusion::datasource::physical_plan::ParquetFileMetrics::new( partition, file.object_meta.location.as_ref(), @@ -242,6 +326,8 @@ impl ParquetFileReaderFactory for SmallFileReaderFactory { view: self.view.clone(), store: self.store.clone(), scope: self.scope, + namespace: self.namespace.clone(), + budget: self.budget, file, partition, metrics: metrics.clone(), @@ -257,6 +343,8 @@ struct Reader { view: Arc>, file_metrics: datafusion::datasource::physical_plan::ParquetFileMetrics, scope: usize, + namespace: String, + budget: usize, factory: Arc, store: Arc, file: PartitionedFile, @@ -269,11 +357,17 @@ struct Reader { impl Reader { async fn init(&mut self) -> Result<()> { if !self.initialized { + if self.file.object_meta.size > MAX_FILE { + self.initialized = true; + return Ok(()); + } let (size, bytes) = load_for_scan( &self.view, self.scope, &self.store, &self.file.object_meta.location, + &self.namespace, + self.budget, ) .await .map_err(|e| ParquetError::External(Box::new(e)))?; @@ -328,12 +422,42 @@ impl AsyncFileReader for Reader { ) -> BoxFuture<'a, Result>> { async move { self.init().await?; + if self.bytes.is_none() { + match self.fallback.get_metadata(options).await { + Ok(metadata) => return Ok(metadata), + Err(error) => { + // A formerly large object may have shrunk while the + // manifest still describes its old footer position. + let current = self + .store + .head(&self.file.object_meta.location) + .await + .map_err(|e| ParquetError::External(Box::new(e)))?; + if current.size == self.file.object_meta.size { + return Err(error); + } + self.file.object_meta = current; + self.fallback = self + .factory + .create_reader( + self.partition, + self.file.clone(), + self.hint, + &self.metrics, + ) + .map_err(|e| ParquetError::External(Box::new(e)))?; + self.initialized = false; + self.init().await?; + } + } + } if let Some(bytes) = &self.bytes { let size = bytes.len() as u64; + let mut reader = std::io::Cursor::new(bytes.clone()); Ok(Arc::new( ParquetMetaDataReader::new() .with_arrow_reader_options(options) - .load_and_finish(self, size) + .load_and_finish(&mut reader, size) .await?, )) } else { @@ -362,14 +486,25 @@ mod tests { .unwrap(); let window = StorageMetricWindow::start("small-cache-test"); assert_eq!( - load(&cache, 1, &store, &path).await.unwrap().1.unwrap(), + load(&cache, 1, &store, &path, "", MAX_BYTES) + .await + .unwrap() + .1 + .unwrap(), "old" ); assert_eq!( - load(&cache, 1, &store, &path).await.unwrap().1.unwrap(), + load(&cache, 1, &store, &path, "", MAX_BYTES) + .await + .unwrap() + .1 + .unwrap(), "old" ); assert_eq!(window.delta().get_requests, 2, "one validation per reader"); + assert_eq!(window.delta().read_bytes, 3); + assert_eq!(window.delta().not_modified_requests, 1); + assert_eq!(window.delta().non_2xx_requests, 0); assert_eq!( crate::metrics::STORAGE_REQUEST_RESPONSE_TIME .with_label_values(&["small-cache-test", "GET", "304"]) @@ -382,28 +517,63 @@ mod tests { .await .unwrap(); assert_eq!( - load(&cache, 1, &store, &path).await.unwrap().1.unwrap(), + load(&cache, 1, &store, &path, "", MAX_BYTES) + .await + .unwrap() + .1 + .unwrap(), "new" ); store.delete(&path).await.unwrap(); - assert!(load(&cache, 1, &store, &path).await.is_err()); + assert!(load(&cache, 1, &store, &path, "", MAX_BYTES).await.is_err()); assert!(cache.lock().unwrap().entries.is_empty()); store .put(&path, Bytes::from_static(b"reborn").into()) .await .unwrap(); assert_eq!( - load(&cache, 1, &store, &path).await.unwrap().1.unwrap(), + load(&cache, 1, &store, &path, "", MAX_BYTES) + .await + .unwrap() + .1 + .unwrap(), "reborn" ); let other: Arc = Arc::new(InMemory::new()); - assert!(load(&cache, 2, &other, &path).await.is_err()); + other + .put(&path, Bytes::from_static(b"separate").into()) + .await + .unwrap(); + assert_eq!( + load(&cache, 1, &other, &path, "s3://other/", MAX_BYTES) + .await + .unwrap() + .1 + .unwrap(), + "separate" + ); + assert_eq!( + load(&cache, 1, &store, &path, "s3://original/", MAX_BYTES) + .await + .unwrap() + .1 + .unwrap(), + "reborn" + ); + other.delete(&path).await.unwrap(); + assert!(load(&cache, 2, &other, &path, "", MAX_BYTES).await.is_err()); store .put(&path, Bytes::from(vec![0; MAX_FILE as usize + 1]).into()) .await .unwrap(); - assert!(load(&cache, 1, &store, &path).await.unwrap().1.is_none()); - assert!(cache.lock().unwrap().entries.is_empty()); + assert!( + load(&cache, 1, &store, &path, "", MAX_BYTES) + .await + .unwrap() + .1 + .is_none() + ); + assert!(cache.lock().unwrap().remove(1, &path).is_none()); } #[tokio::test] @@ -417,7 +587,7 @@ mod tests { .await .unwrap(); assert_eq!( - load_for_scan(&view, scope, &store, &path) + load_for_scan(&view, scope, &store, &path, "", MAX_BYTES) .await .unwrap() .1 @@ -429,7 +599,7 @@ mod tests { .await .unwrap(); assert_eq!( - load_for_scan(&view, scope, &store, &path) + load_for_scan(&view, scope, &store, &path, "", MAX_BYTES) .await .unwrap() .1 @@ -439,7 +609,7 @@ mod tests { ); let next = Mutex::new(Cache::default()); assert_eq!( - load_for_scan(&next, scope, &store, &path) + load_for_scan(&next, scope, &store, &path, "", MAX_BYTES) .await .unwrap() .1 @@ -449,7 +619,11 @@ mod tests { ); store.delete(&path).await.unwrap(); let next = Mutex::new(Cache::default()); - assert!(load_for_scan(&next, scope, &store, &path).await.is_err()); + assert!( + load_for_scan(&next, scope, &store, &path, "", MAX_BYTES) + .await + .is_err() + ); let mut view = view.lock().unwrap(); for i in 0..40 { view.insert( @@ -513,7 +687,7 @@ mod measurement { physical_plan::ExecutionPlan, prelude::{Expr, SessionConfig, SessionContext}, }; - use object_store::{ObjectStoreExt, memory::InMemory}; + use object_store::ObjectStoreExt; use parquet::arrow::ArrowWriter; #[derive(Debug)] @@ -521,6 +695,7 @@ mod measurement { schema: SchemaRef, files: Vec, cached: bool, + disabled: bool, } #[async_trait::async_trait] impl TableProvider for Table { @@ -553,7 +728,21 @@ mod measurement { } let plan = format.create_physical_plan(state, builder.build()).await?; if self.cached { - install(plan, state.runtime_env().object_store(url)?, 12345) + install( + plan, + state.runtime_env().object_store(url)?, + 12345, + MAX_BYTES, + true, + ) + } else if self.disabled { + install( + plan, + state.runtime_env().object_store(url)?, + 12345, + MAX_BYTES, + false, + ) } else { Ok(plan) } @@ -564,7 +753,10 @@ mod measurement { // and PromQL evaluator. It isolates file I/O from HTTP admission and mutable // catalog reads; those costs must not be presented as measured here. #[tokio::test] + #[ignore = "requires real MinIO; set P_CACHE_TEST_MINIO_ENDPOINT and run --ignored --nocapture"] async fn repeated_sixty_file_queries() { + let endpoint = std::env::var("P_CACHE_TEST_MINIO_ENDPOINT") + .expect("HTTP test requires P_CACHE_TEST_MINIO_ENDPOINT"); let schema = Arc::new(Schema::new(vec![ Field::new( "p_timestamp", @@ -580,22 +772,7 @@ mod measurement { Field::new("job", DataType::Utf8, false), Field::new("data_point_value", DataType::Float64, false), ])); - let backend: Arc = - if let Ok(endpoint) = std::env::var("P_CACHE_TEST_MINIO_ENDPOINT") { - Arc::new( - object_store::aws::AmazonS3Builder::new() - .with_endpoint(endpoint) - .with_allow_http(true) - .with_region("us-east-1") - .with_bucket_name("cache-measurement") - .with_access_key_id("minioadmin") - .with_secret_access_key("minioadmin") - .build() - .unwrap(), - ) - } else { - Arc::new(InMemory::new()) - }; + let backend: Arc = s3(&endpoint); let store: Arc = Arc::new(MetricLayer::new(backend, "cache-measurement")); let mut files = Vec::new(); for minute in 0..60 { @@ -648,11 +825,19 @@ mod measurement { ]; for (name, sql) in queries { let mut reference = None; - for cached in [false, true] { + let mut reference_bytes = None; + let mut reference_requests = None; + for (cached, disabled) in [(false, false), (false, true), (true, false)] { if cached { let mut cache = CACHE.lock().unwrap(); for file in &files { - cache.remove(12345, &file.object_meta.location); + cache.remove( + 12345, + &Path::from(format!( + "s3://cache-measurement/{}", + file.object_meta.location + )), + ); } } for run in 0..2 { @@ -674,6 +859,7 @@ mod measurement { schema: schema.clone(), files: files.clone(), cached, + disabled, }), ) .unwrap(); @@ -684,7 +870,18 @@ mod measurement { }; let before_304 = not_modified(); let window = StorageMetricWindow::start("cache-measurement"); - let batches = ctx.sql(sql).await.unwrap().collect().await.unwrap(); + let df = ctx.sql(sql).await.unwrap(); + let task = ctx.task_ctx(); + let plan = df.create_physical_plan().await.unwrap(); + let batches = datafusion::physical_plan::collect(plan.clone(), task) + .await + .unwrap(); + let scanned = scanned_bytes(&plan); + if let Some(expected) = reference_bytes { + assert_eq!(scanned, expected, "{name} cached={cached} run={run}"); + } else { + reference_bytes = Some(scanned); + } let result = arrow::util::pretty::pretty_format_batches(&batches) .unwrap() .to_string(); @@ -726,14 +923,40 @@ mod measurement { let result = promql::evaluate_range(&plan, &[series], 300_000, 3_540_000, 60_000) .unwrap(); - assert!(!result.into_json().to_string().is_empty()); + let json = result.into_json(); + let values = json["result"][0]["values"].as_array().unwrap(); + assert_eq!(values.len(), 55); + for (i, point) in values.iter().enumerate() { + assert_eq!(point[0].as_f64().unwrap(), 300.0 + i as f64 * 60.0); + let value: f64 = point[1].as_str().unwrap().parse().unwrap(); + assert!((value - 8.0).abs() < 1e-9, "{point}"); + } } let delta = window.delta(); + let requests = (delta.get_requests, delta.get_ranges_requests); + if disabled { + assert_eq!( + Some(requests), + reference_requests, + "kill switch must preserve original HTTP request pattern" + ); + } else if !cached { + reference_requests = Some(requests); + } println!( - "CACHE_MEASUREMENT {name} cached={cached} run={run} GET={} GET_RANGES={}", + "CACHE_MEASUREMENT {name} cached={cached} disabled={disabled} run={run} GET={} GET_RANGES={}", delta.get_requests, delta.get_ranges_requests ); if cached { + assert_eq!(delta.non_2xx_requests, 0); + assert_eq!( + delta.read_bytes, + if run == 0 { + files.iter().map(|f| f.object_meta.size).sum() + } else { + 0 + } + ); if run == 1 { assert_eq!( not_modified() - before_304, @@ -748,4 +971,246 @@ mod measurement { } } } + #[tokio::test] + async fn disabled_and_oversized_install_preserve_original_plan() { + let ctx = SessionContext::new(); + let store: Arc = Arc::new(object_store::memory::InMemory::new()); + ctx.runtime_env().register_object_store( + &url::Url::parse("s3://cache-measurement").unwrap(), + store.clone(), + ); + for count in [1, MAX_ENTRIES + 1] { + let table = Table { + schema: Arc::new(Schema::new(vec![Field::new( + "value", + DataType::Int64, + false, + )])), + files: (0..count) + .map(|i| PartitionedFile::new(format!("{i}.parquet"), 10)) + .collect(), + cached: false, + disabled: false, + }; + let plan = table.scan(&ctx.state(), None, &[], None).await.unwrap(); + for (enabled, budget) in [(false, MAX_BYTES), (true, 0), (true, 9)] { + let actual = install(plan.clone(), store.clone(), 0, budget, enabled).unwrap(); + assert!(Arc::ptr_eq(&actual, &plan)); + } + if count > MAX_ENTRIES { + assert!(Arc::ptr_eq( + &install(plan.clone(), store.clone(), 0, MAX_BYTES, true).unwrap(), + &plan + )); + } + } + } + + fn s3(endpoint: &str) -> Arc { + Arc::new( + object_store::aws::AmazonS3Builder::new() + .with_retry(object_store::RetryConfig { + max_retries: 0, + ..Default::default() + }) + .with_endpoint(endpoint) + .with_allow_http(true) + .with_region("us-east-1") + .with_bucket_name("cache-measurement") + .with_access_key_id("minioadmin") + .with_secret_access_key("minioadmin") + .build() + .unwrap(), + ) + } + + fn scanned_bytes(plan: &Arc) -> usize { + plan.metrics() + .and_then(|m| m.sum_by_name("bytes_scanned")) + .map(|m| m.as_usize()) + .unwrap_or(0) + + plan + .children() + .iter() + .map(|p| scanned_bytes(p)) + .sum::() + } + + struct Proxy(std::process::Child); + impl Drop for Proxy { + fn drop(&mut self) { + let _ = self.0.kill(); + let _ = self.0.wait(); + } + } + + async fn query( + store: Arc, + size: u64, + cached: bool, + ) -> datafusion::common::Result> { + let ctx = SessionContext::new(); + ctx.runtime_env() + .register_object_store(&url::Url::parse("s3://cache-measurement").unwrap(), store); + ctx.register_table( + "metrics", + Arc::new(Table { + schema: Arc::new(Schema::new(vec![Field::new( + "value", + DataType::Int64, + false, + )])), + files: vec![PartitionedFile::new("mutations/data.parquet", size)], + cached, + disabled: false, + }), + )?; + let batches = ctx + .sql("SELECT value FROM metrics ORDER BY value") + .await? + .collect() + .await?; + Ok(batches + .iter() + .flat_map(|b| { + b.column(0) + .as_any() + .downcast_ref::() + .unwrap() + .values() + .to_vec() + }) + .collect()) + } + + fn parquet(values: Vec) -> Bytes { + let batch = RecordBatch::try_from_iter(vec![( + "value", + Arc::new(arrow_array::Int64Array::from(values)) as arrow_array::ArrayRef, + )]) + .unwrap(); + let mut body = Vec::new(); + let mut writer = ArrowWriter::try_new(&mut body, batch.schema(), None).unwrap(); + writer.write(&batch).unwrap(); + writer.close().unwrap(); + Bytes::from(body) + } + + #[tokio::test] + #[ignore = "requires real MinIO; set P_CACHE_TEST_MINIO_ENDPOINT and run --ignored --nocapture"] + async fn http_mutations_and_faults() { + use std::io::{BufRead, BufReader}; + let endpoint = std::env::var("P_CACHE_TEST_MINIO_ENDPOINT") + .expect("HTTP test requires P_CACHE_TEST_MINIO_ENDPOINT"); + let dir = tempfile::tempdir_in(".").unwrap(); + let mode = dir.path().join("mode"); + std::fs::write(&mode, "normal").unwrap(); + let mut proxy = Proxy( + std::process::Command::new("python3") + .args([ + "-u", + "src/query/cache_http_proxy.py", + &endpoint, + mode.to_str().unwrap(), + ]) + .stdout(std::process::Stdio::piped()) + .spawn() + .unwrap(), + ); + let mut port = String::new(); + BufReader::new(proxy.0.stdout.take().unwrap()) + .read_line(&mut port) + .unwrap(); + let backend = s3(&endpoint); + let store: Arc = Arc::new(MetricLayer::new( + s3(&format!("http://127.0.0.1:{}", port.trim())), + "http-mutations", + )); + let path = Path::from("mutations/data.parquet"); + let old = parquet(vec![1, 2, 3]); + let new = parquet(vec![4, 5, 6]); + assert_eq!(old.len(), new.len()); + backend.put(&path, old.clone().into()).await.unwrap(); + for _ in 0..2 { + assert_eq!( + query(store.clone(), old.len() as u64, true).await.unwrap(), + [1, 2, 3] + ); + } + backend.put(&path, new.clone().into()).await.unwrap(); + assert_eq!( + query(store.clone(), old.len() as u64, true).await.unwrap(), + [4, 5, 6] + ); + backend.delete(&path).await.unwrap(); + assert!(query(store.clone(), old.len() as u64, false).await.is_err()); + assert!(query(store.clone(), old.len() as u64, true).await.is_err()); + backend.put(&path, old.clone().into()).await.unwrap(); + assert_eq!( + query(store.clone(), old.len() as u64, true).await.unwrap(), + [1, 2, 3] + ); + std::fs::write(&mode, "fail").unwrap(); + assert!(query(store.clone(), old.len() as u64, true).await.is_err()); + std::fs::write(&mode, "normal").unwrap(); + assert_eq!( + query(store.clone(), old.len() as u64, true).await.unwrap(), + [1, 2, 3] + ); + backend.put(&path, new.clone().into()).await.unwrap(); + std::fs::write(&mode, "broken").unwrap(); + assert!(query(store.clone(), old.len() as u64, true).await.is_err()); + assert!(!CACHE.lock().unwrap().entries.contains_key(&( + 12345, + Path::from("s3://cache-measurement/mutations/data.parquet") + ))); + std::fs::write(&mode, "normal").unwrap(); + assert_eq!( + query(store.clone(), new.len() as u64, true).await.unwrap(), + [4, 5, 6] + ); + for fault in ["absent", "weak", "full"] { + // Remove a prior strong validator, then exercise two independent scans. + backend.delete(&path).await.unwrap(); + assert!(query(store.clone(), old.len() as u64, true).await.is_err()); + backend.put(&path, new.clone().into()).await.unwrap(); + std::fs::write(&mode, fault).unwrap(); + std::fs::write(mode.with_extension("requests"), "").unwrap(); + let window = StorageMetricWindow::start("http-mutations"); + for _ in 0..2 { + assert_eq!( + query(store.clone(), new.len() as u64, true).await.unwrap(), + [4, 5, 6] + ); + } + assert_eq!(window.delta().not_modified_requests, 0, "{fault}"); + if fault != "full" { + assert_eq!( + std::fs::read_to_string(mode.with_extension("requests")).unwrap(), + "False\nFalse\n", + "{fault}" + ); + } + std::fs::write(&mode, "normal").unwrap(); + } + // Unique integers defeat dictionary compression, producing a real large parquet file. + let values = (0..100_000).collect::>(); + let large = parquet(values.clone()); + assert!(large.len() as u64 > MAX_FILE); + backend.put(&path, large.clone().into()).await.unwrap(); + for size in [old.len(), large.len()] { + assert_eq!( + query(store.clone(), size as u64, true).await.unwrap(), + values + ); + } + backend.put(&path, old.clone().into()).await.unwrap(); + for size in [large.len(), old.len()] { + assert_eq!( + query(store.clone(), size as u64, true).await.unwrap(), + [1, 2, 3] + ); + } + backend.delete(&path).await.unwrap(); + } } diff --git a/src/query/stream_schema_provider.rs b/src/query/stream_schema_provider.rs index a5184add4..31019c43d 100644 --- a/src/query/stream_schema_provider.rs +++ b/src/query/stream_schema_provider.rs @@ -364,7 +364,10 @@ async fn parquet_scan_plan( batch_size: usize, ) -> Result, DataFusionError> { let (file_format, file_source) = build_parquet_scan_components(schema, filters, state)?; - let cache_store = if object_store_url.as_str() != "file:///" { + let cache_store = if object_store_url.as_str() != "file:///" + && PARSEABLE.options.small_file_cache_enabled + && PARSEABLE.options.small_file_cache_size > 0 + { Some(state.runtime_env().object_store(&object_store_url)?) } else { None @@ -404,6 +407,8 @@ async fn parquet_scan_plan( plan, store, Arc::as_ptr(&PARSEABLE.storage) as *const () as usize, + PARSEABLE.options.small_file_cache_size, + PARSEABLE.options.small_file_cache_enabled, ) } else { Ok(plan) @@ -424,8 +429,12 @@ where F: std::future::Future>, I: Fn(&M) -> bool, { - let mut captured = if predicted { Some(capture()?) } else { None }; + let pending = predicted.then(&mut capture); + let mut captured = None; let mut metadata = load(false).await?; + if predicted && includes_staging(&metadata) { + captured = Some(pending.expect("predicted capture")?); + } if !predicted && includes_staging(&metadata) { captured = Some(capture()?); metadata = load(true).await?; @@ -2503,4 +2512,19 @@ mod snapshot_capture_tests { assert_eq!(snapshot, 42); assert_eq!(staging, None); } + #[tokio::test] + async fn speculative_capture_error_only_fails_when_staging_is_needed() { + for needed in [false, true] { + let result = capture_before_snapshot( + true, + || -> DataFusionResult<()> { + Err(DataFusionError::Execution("capture failed".into())) + }, + |_| async { Ok(needed) }, + |needed| *needed, + ) + .await; + assert_eq!(result.is_err(), needed); + } + } } diff --git a/src/storage/metrics_layer.rs b/src/storage/metrics_layer.rs index 750229777..f15c4f7fa 100644 --- a/src/storage/metrics_layer.rs +++ b/src/storage/metrics_layer.rs @@ -61,6 +61,7 @@ struct StorageMetricSnapshot { response_time_sum_seconds: f64, response_time_count: u64, non_2xx_requests: u64, + not_modified_requests: u64, inflight: u64, } @@ -100,7 +101,9 @@ impl StorageMetricSnapshot { } snapshot.response_time_count += count; snapshot.response_time_sum_seconds += histogram.get_sample_sum(); - if !status.starts_with('2') { + if status == "304" { + snapshot.not_modified_requests += count; + } else if !status.starts_with('2') { snapshot.non_2xx_requests += count; } } @@ -123,6 +126,9 @@ impl StorageMetricSnapshot { .response_time_count .saturating_sub(self.response_time_count), non_2xx_requests: end.non_2xx_requests.saturating_sub(self.non_2xx_requests), + not_modified_requests: end + .not_modified_requests + .saturating_sub(self.not_modified_requests), peak_inflight_sampled, } } @@ -139,6 +145,7 @@ pub struct StorageMetricDelta { pub response_time_sum_seconds: f64, pub response_time_count: u64, pub non_2xx_requests: u64, + pub not_modified_requests: u64, pub peak_inflight_sampled: u64, } @@ -333,11 +340,6 @@ impl ObjectStore for MetricLayer { } async fn get_opts(&self, location: &Path, options: GetOptions) -> ObjectStoreResult { - let requested_bytes = options.range.as_ref().and_then(|range| match range { - object_store::GetRange::Bounded(range) => Some(range.end.saturating_sub(range.start)), - object_store::GetRange::Suffix(bytes) => Some(*bytes), - object_store::GetRange::Offset(_) => None, - }); STORAGE_READ_RANGES_TOTAL .with_label_values(&[&self.provider]) .inc(); @@ -354,18 +356,27 @@ impl ObjectStore for MetricLayer { STORAGE_REQUEST_RESPONSE_TIME .with_label_values(&[&self.provider, "GET", status]) .observe(elapsed); - let requested_bytes = requested_bytes.or_else(|| { - result - .as_ref() - .ok() - .map(|result| result.range.end.saturating_sub(result.range.start)) - }); - if let Some(bytes) = requested_bytes { - STORAGE_READ_BYTES_TOTAL - .with_label_values(&[&self.provider]) - .inc_by(bytes); - } - result + result.map(|response| { + let meta = response.meta.clone(); + let range = response.range.clone(); + let attributes = response.attributes.clone(); + let counter = STORAGE_READ_BYTES_TOTAL.with_label_values(&[&self.provider]); + let stream = response + .into_stream() + .map(move |chunk| { + if let Ok(bytes) = &chunk { + counter.inc_by(bytes.len() as u64); + } + chunk + }) + .boxed(); + GetResult { + payload: object_store::GetResultPayload::Stream(stream), + meta, + range, + attributes, + } + }) } async fn get_ranges( @@ -373,20 +384,17 @@ impl ObjectStore for MetricLayer { location: &Path, ranges: &[Range], ) -> ObjectStoreResult> { - STORAGE_READ_BYTES_TOTAL - .with_label_values(&[&self.provider]) - .inc_by( - ranges - .iter() - .map(|range| range.end.saturating_sub(range.start)) - .sum(), - ); STORAGE_READ_RANGES_TOTAL .with_label_values(&[&self.provider]) .inc_by(ranges.len() as u64); let _guard = InflightGuard::new(&self.provider, "GET_RANGES"); let time = time::Instant::now(); let result = self.inner.get_ranges(location, ranges).await; + if let Ok(bytes) = &result { + STORAGE_READ_BYTES_TOTAL + .with_label_values(&[&self.provider]) + .inc_by(bytes.iter().map(|bytes| bytes.len() as u64).sum()); + } let elapsed = time.elapsed().as_secs_f64(); let status = match &result { @@ -537,8 +545,8 @@ mod tests { use std::sync::Mutex; use object_store::{ - Attribute, Attributes, ObjectStore, ObjectStoreExt, PutPayload, memory::InMemory, - path::Path, + Attribute, Attributes, GetOptions, ObjectStore, ObjectStoreExt, PutPayload, + memory::InMemory, path::Path, }; use super::{MetricLayer, StorageMetricSnapshot, StorageMetricWindow}; @@ -590,6 +598,65 @@ mod tests { ); } + #[tokio::test] + async fn actual_body_bytes_exclude_not_modified_and_errors() { + let layer = MetricLayer::new(InMemory::new(), "actual-body-metrics"); + let path = Path::from("file"); + layer + .put(&path, PutPayload::from_static(b"abc")) + .await + .unwrap(); + let window = StorageMetricWindow::start("actual-body-metrics"); + let result = layer + .get_opts( + &path, + GetOptions { + range: Some(object_store::GetRange::Bounded(0..262145)), + ..Default::default() + }, + ) + .await + .unwrap(); + let etag = result.meta.e_tag.clone(); + assert_eq!(result.bytes().await.unwrap(), "abc"); + assert!(matches!( + layer + .get_opts( + &path, + GetOptions { + if_none_match: etag, + range: Some(object_store::GetRange::Bounded(0..262145)), + ..Default::default() + } + ) + .await, + Err(object_store::Error::NotModified { .. }) + )); + let missing = Path::from("missing"); + assert!( + layer + .get_opts( + &missing, + GetOptions { + range: Some(object_store::GetRange::Bounded(0..262145)), + ..Default::default() + } + ) + .await + .is_err() + ); + assert!( + layer + .get_ranges(&missing, std::slice::from_ref(&(0..10))) + .await + .is_err() + ); + let delta = window.delta(); + assert_eq!(delta.read_bytes, 3); + assert_eq!(delta.not_modified_requests, 1); + assert_eq!(delta.non_2xx_requests, 2); + } + #[test] fn process_window_metric_delta_arithmetic_is_saturating_and_derives_average() { let start = StorageMetricSnapshot { @@ -600,6 +667,7 @@ mod tests { response_time_sum_seconds: 1.0, response_time_count: 6, non_2xx_requests: 1, + not_modified_requests: 0, inflight: 3, }; let end = StorageMetricSnapshot { @@ -610,6 +678,7 @@ mod tests { response_time_sum_seconds: 2.6, response_time_count: 14, non_2xx_requests: 3, + not_modified_requests: 0, inflight: 1, }; let delta = start.delta_to(&end, 17); From 633b8e13985c4e0010f8a0d5c45e4e83a8915696 Mon Sep 17 00:00:00 2001 From: VibeSWE <338779468+VibeSWE@users.noreply.github.com> Date: Tue, 10 Nov 2026 23:46:03 -0600 Subject: [PATCH 3/5] fix: validate small parquet files once per query request --- .../performance/validated-small-file-cache.md | 123 ++++- scripts/performance/request_cache_repro.py | 61 +++ src/parseable/streams.rs | 2 +- src/query/mod.rs | 11 +- src/query/small_file_cache.rs | 438 +++++++++++++++--- src/query/stream_schema_provider.rs | 4 + 6 files changed, 563 insertions(+), 76 deletions(-) create mode 100644 scripts/performance/request_cache_repro.py diff --git a/docs/performance/validated-small-file-cache.md b/docs/performance/validated-small-file-cache.md index 92b7f7f9c..d1f500b53 100644 --- a/docs/performance/validated-small-file-cache.md +++ b/docs/performance/validated-small-file-cache.md @@ -40,19 +40,21 @@ original factory for larger files, preserving its options and metadata cache. For a manifest file size at most 256 KiB, a reader fetches the complete object in one bounded GET. It holds those bytes for its lifetime; metadata, page indexes, -and column reads then use slices of the same body. On a subsequent scan, a +and column reads then use slices of the same body. On a subsequent request, a conditional GET validates the cached ETag. A 304 reuses the bytes without a body; a changed object replaces them. A 404/error invalidates the entry and propagates. -Absent or weak (`W/`) ETags mean no cross-scan body reuse. Growth past the limit uses the original +Absent or weak (`W/`) ETags mean no cross-request body reuse. Growth past the limit uses the original reader with the newly observed size. The bounded probe prevents an unexpected large replacement from being read into memory in full. Keys use the process's singleton storage-provider identity, object-store URL, and full object path, including tenant prefixes. Per-query object-store clients differ, so their pointer identity would prevent cross-query hits. The configured provider lives for the -process lifetime. A physical scan also has an 8 MiB LRU view shared between -DataFusion metadata and data readers, avoiding a second validation of the same -body within that scan. New scans never inherit that view. The keyed linked-list LRU performs lookup, promotion, and removal in O(1), +process lifetime. Each query execution attaches a fresh `RequestView` to its DataFusion session +configuration. Every installed scan shares it. Per-file asynchronous cells coalesce +concurrent metadata, bloom-filter and decoder opens. Validated bodies stay referenced +until the request finishes; they are not evicted by a second, smaller LRU. New +requests never inherit the view. The keyed linked-list LRU performs lookup, promotion, and removal in O(1), without holding its mutex across I/O. Storage defaults to 64 MiB and 4,096 entries. Active readers can retain evicted bodies; as with existing scan buffers, their memory also depends on query concurrency. This limit is a cache residency limit, not a process RSS cap. @@ -63,9 +65,7 @@ Direct reads of a removed file validate and fail; bytes left unreferenced in the cache are reclaimed by bounded eviction. Recreating the same path validates its new ETag. There is no negative cache to hide newly uploaded files. -This deliberately retains one network validation per small file per scan while -the bounded scan view retains it; eviction or concurrent first opens can require -additional validations. Avoiding +This retains one network validation per selected small file per request. Avoiding that last round trip requires a stronger immutable/versioned publication contract. Whole-body reads trade extra cold bytes for fewer round trips; the limit keeps that trade confined to small objects. Large-file metadata behavior is unchanged. @@ -107,8 +107,9 @@ its table as a reproduction of the supplied live-instance endpoint totals. Local MinIO, 60 files containing 28,800 samples over one hour, 894,420 bytes total. Both runs returned identical results; these are the repeated run's metric deltas. The PromQL case scans samples and evaluates `sum(rate(requests{job="api"}[5m]))` -over a range with 60-second steps. The fixture is intentionally narrower than a -real OTLP dataset, and excludes endpoint/catalog overhead as described above. +over a range with 60-second steps. These are historical results from the original plain-Parquet fixture. The fixture +now uses production metrics writer properties, pushes filters to Parquet, and forces +byte-range repartitioning; see the request regression below for HTTP results. | Parquet workload | Before GET | Before GET_RANGES | After GET | After GET_RANGES | Calls before → after | | --- | ---: | ---: | ---: | ---: | ---: | @@ -133,8 +134,9 @@ ranges only; footer/page-index loading does not add to this billing-style total. Setting it to `false` skips reader installation at `parquet_scan_plan`, retaining the original DataFusion reader and its request pattern. `--small-file-cache-size` / `P_SMALL_FILE_CACHE_SIZE` defaults to `67108864` bytes -(64 MiB); zero also disables installation. The per-scan view stays at 8 MiB and -the process cache entry cap stays at 4,096. These are residency limits, not RSS +(64 MiB); zero also disables installation. The process cache entry cap stays at 4,096. The request view retains admitted +files until completion (including streaming completion); its references can outlive +process-LRU residency, just as active readers can. These are residency limits, not RSS limits. No new required configuration or dependency was added. A scan whose selected small-file manifest sizes exceed the configured byte limit, @@ -164,7 +166,7 @@ test compares original and cached query rows and `bytes_scanned`, cold and warm, checks actual storage bytes and warm 304s, and checks evaluated PromQL values. It does not exercise Azure/GCS emulators or the live dashboard service. -## Fix validation (2026-11-10) +## Previous fix validation (plain fixture, 2026-11-10) Real MinIO HTTP on isolated loopback ports, plus the Python response fault proxy: all six cache tests passed, including both explicitly enabled HTTP tests. The full @@ -191,3 +193,98 @@ Final plain `cargo clippy --all-targets` passed with existing warnings only No warnings point to changed code. The temporary MinIO container and its volumes were removed; the live side-by-side service, configuration, and bucket were not used. No push or PR was performed. + + +## Request validation regression (2026-11-10) + +The original measurement missed concurrent opens: its 894,420-byte working set +was below DataFusion's default 1 MiB `repartition_file_min_size`. Its table also +reported unsupported filter pushdown and wrote no bloom filters. Production +`Query::create_session_state` enables repartitioning and work stealing, with a +CPU-derived partition count. DataFusion `FileGroupPartitioner::repartition_evenly_by_size` +checks the **total** scan size against 1 MiB and splits files at byte boundaries. +Several readers can therefore open one tiny file concurrently. In the old +`load_for_scan`, every reader checked an empty view, released its mutex, and +started its own validation before any reader populated the view. A 304 still +requires network I/O, so even warm bodies raced. The old 8 MiB view could also +evict earlier validations on larger requests. + +The instrumented baseline showed two or three `CACHE_LOAD`s per file with the +**same** view pointer and distinct `FileRange`s. DataFusion's +`RowGroupsPrunedParquetOpen::load_bloom_filters` also constructs a replacement reader +for decoding; this reader shares the view and is no longer a source of extra +validations. No second physical query execution was needed to reproduce the +problem. `resolve_stream_names` parses SQL for table discovery, and `into_query` +plans it afterward; that discovery does not execute Parquet scans. + +`Query::execute` now clones the catalog session state and supplies a new +`RequestView` extension. `parquet_scan_plan` passes that view to each installed +reader factory. An async cell per provider/URL/path serializes first loads and +retains success or failure for the execution. The process ETag LRU is unchanged: +a later HTTP request starts an empty view and must issue its own conditional GET. +The request view intentionally retains bodies; an 8 MiB eviction policy cannot +promise at most one validation for a later reader of the same request. + +To reproduce without waiting hours, the ignored `publish_request_fixture` test +publishes 390 synthetic historical minute partitions using real +`Stream::parquet_writer_props` and `create_from_parquet_file`, including metrics +bloom filters, page indexes and sorted metric names. This is synthetic fixture +publication, not a claim that 390 files were ingested over wall-clock minutes. +The HTTP script ingests an actual OTLP gauge into staging before querying. Files +are approximately 19.7 KB. Run a debug standalone server against this isolated +bucket with `P_CACHE_TRACE=1`, then run +`scripts/performance/request_cache_repro.py`. Set `CACHE_REPRO_REQUESTS` to a +worktree-local JSON path to freeze the exact request ranges for before/after; +set `CACHE_REPRO_ASSERT_ONCE=1` for the regression assertions. It performs two +separate HTTP requests per shape, recording the second request's metric delta +and per-path loads; it asserts one load per file on **both** requests. Set +`CACHE_REPRO_URL` and `CACHE_REPRO_LOG` if using different ports/log paths. +Never point this harness at a production service or bucket. + +The 60-file measurement now uses the same writer properties and enables pushed +predicates, work stealing and forced byte-range splits across 32 partitions. +The unit regressions additionally cover concurrent first opens, next-request +overwrite/delete visibility, and late reads after more than 8 MiB of other files. + +Measured warm HTTP totals (all GETs, including metadata), isolated MinIO on +18397 and standalone HTTP on 18398. Original requests ended at wall-clock now. +The frozen-range rerun happened later; background OTLP staging uploads added +files and metadata reads, so selected-file counts are shown explicitly. + +| Shape | Before calls / files / validations | Fixed same ranges calls / files / validations | Fixed fresh now calls / files / validations | +| --- | ---: | ---: | ---: | +| count_10m | 8 / 6 / 6 | 10 / 7 / 7 | 4 / 1 / 1 | +| count_1h | 136 / 56 / 134 | 60 / 57 / 57 | 50 / 47 / 47 | +| group_1h | 137 / 56 / 135 | 60 / 57 / 57 | 50 / 47 / 47 | +| promql_range_1h | 140 / 61 / 138 | 66 / 63 / 63 | 55 / 52 / 52 | +| label_values_1h | 136 / 56 / 134 | 61 / 58 / 58 | 50 / 47 / 47 | +| count_24h | 472 / 390 / 469 | 395 / 391 / 391 | 397 / 393 / 393 | +| promql_instant | 3 / 1 / 1 | 5 / 2 / 2 | 3 / 0 / 0 | + +Every fixed per-file multiplicity was exactly one, on both the first and second +HTTP requests. An instant query with zero remote files was served from staging. +The 24-hour fixture spans two UTC days (three initial metadata reads); background +uploads later changed that overhead: the fixture uses a distinct manifest identity, +so the standalone uploader adds its own hostname-owned manifest alongside it. +This accounts for the extra metadata GET after staging uploads. This synthetic fixture did not reproduce +the live instance's 24-hour statistics shortcut; its 390 historical files were +all opened. The cause/fix is established by per-file loads, independent of that +workload difference. + +The revised 60-file fixture contains 579,821 bytes. With forced splitting across +32 partitions, original-reader warm GET / GET_RANGES were 0 / 91 for COUNT, +1 / 211 for GROUP BY, and 120 / 270 for PromQL scan/evaluation. The fixed cache +made exactly 60 GETs for every shape, cold and warm, with 60 warm 304s. It +returned the same rows, evaluated values and billing-style bytes scanned. + + +Final validation: `cargo test --lib` passed with 1,387 passed, zero failed and +seven ignored. All three cache HTTP tests were explicitly run with +`cargo test --lib query::small_file_cache -- --ignored --nocapture` and passed, +including the production-property index assertions. Both HTTP regression runs +(frozen ranges and fresh now) passed per-request assertions. Plain +`cargo clippy --all-targets` passed with the existing 21 library-test warnings +and staging API compatibility warning; no warnings point to changed code. +The temporary standalone process was stopped and the MinIO container and its +anonymous volume were removed. No live-service settings, data or processes were +changed. No push or PR was performed. diff --git a/scripts/performance/request_cache_repro.py b/scripts/performance/request_cache_repro.py new file mode 100644 index 000000000..b808c9ce6 --- /dev/null +++ b/scripts/performance/request_cache_repro.py @@ -0,0 +1,61 @@ +#!/usr/bin/env python3 +"""Run against the isolated fixture/server described in validated-small-file-cache.md. +Set CACHE_REPRO_ASSERT_ONCE=1 after the fix. Every shape runs twice, with a fresh +HTTP request, and measures the second request. No third-party Python packages. +""" +import base64, collections, datetime, json, os, re, time, urllib.request, urllib.parse +BASE = os.environ.get('CACHE_REPRO_URL', 'http://127.0.0.1:18398') +LOG = os.environ.get('CACHE_REPRO_LOG', 'target/cache-repro/server.log') +HEADERS = {'Authorization': 'Basic ' + base64.b64encode(b'admin:admin').decode(), 'X-P-Stream': 'repro'} +def call(path, body=None): + headers = dict(HEADERS) + if body is not None: + headers['Content-Type'] = 'application/json' + body = json.dumps(body).encode() + try: + with urllib.request.urlopen(urllib.request.Request(BASE + path, data=body, headers=headers), timeout=180) as response: + return response.read().decode() + except urllib.error.HTTPError as error: + raise RuntimeError(error.read().decode()) from error +def metrics(): + return sum(float(line.rsplit(' ', 1)[1]) for line in call('/api/v1/metrics').splitlines() if line.startswith('parseable_storage_request_response_time_count{') and '"GET"' in line) +def stamp(t): + return datetime.datetime.fromtimestamp(t, datetime.timezone.utc).isoformat() +def query(sql, minutes): + now = time.time() + return '/api/v1/query', {'query': sql, 'startTime': stamp(now - minutes*60), 'endTime': stamp(now)} +# Keep the recent staging/remote union in every reproduction. +call('/v1/metrics', {'resourceMetrics': [{'resource': {'attributes': [{'key':'host','value':{'stringValue':'host-0'}}]}, 'scopeMetrics': [{'metrics': [{'name':'cpu','gauge':{'dataPoints':[{'timeUnixNano':str(time.time_ns()),'asDouble':0.5}]}}]}]}]}) +now = time.time() +shapes = [ + ('count_10m', *query('SELECT COUNT(*) FROM repro', 10)), + ('count_1h', *query('SELECT COUNT(*) FROM repro', 60)), + ('group_1h', *query("SELECT host, SUM(data_point_value) FROM repro WHERE metric_name = 'cpu' GROUP BY host", 60)), + ('promql_range_1h', '/prometheus/api/v1/query_range?' + urllib.parse.urlencode({'query':'cpu','start':now-3600,'end':now,'step':60}), None), + ('label_values_1h', '/prometheus/api/v1/label/host/values?' + urllib.parse.urlencode({'match[]':'cpu','start':now-3600,'end':now}), None), + ('count_24h', *query('SELECT COUNT(*) FROM repro',1440)), + ('promql_instant','/prometheus/api/v1/query?' + urllib.parse.urlencode({'query':'cpu','time':now}),None), +] +request_file = os.environ.get('CACHE_REPRO_REQUESTS') +if request_file: + if os.path.exists(request_file): + with open(request_file) as saved: + shapes = json.load(saved) + else: + with open(request_file, 'w') as saved: + json.dump(shapes, saved) +for name, path, body in shapes: + for run in range(2): + before = metrics() + offset = os.path.getsize(LOG) + result = call(path, body) + after = metrics() + with open(LOG) as trace: + trace.seek(offset) + loads = collections.Counter(re.findall(r'CACHE_LOAD .*path=(\S+)',trace.read())) + if run: + print(json.dumps({'shape':name,'calls':int(after-before),'selected_files':len(loads),'validations':sum(loads.values()),'multiplicity':dict(collections.Counter(loads.values()))}), flush=True) + if os.environ.get('CACHE_REPRO_ASSERT_ONCE'): + if name in ('count_1h', 'group_1h', 'promql_range_1h', 'label_values_1h'): + assert len(loads) > 1, (name, 'fixture must select multiple remote files') + assert all(n == 1 for n in loads.values()), (name, loads) diff --git a/src/parseable/streams.rs b/src/parseable/streams.rs index 3730710aa..d389a2131 100644 --- a/src/parseable/streams.rs +++ b/src/parseable/streams.rs @@ -1956,7 +1956,7 @@ impl Stream { #[inline(always)] fn conversion_phase(&self, _phase: ConversionPhase<'_>) {} - fn parquet_writer_props( + pub(crate) fn parquet_writer_props( &self, merged_schema: &Schema, time_partition: Option<&String>, diff --git a/src/query/mod.rs b/src/query/mod.rs index 784686324..803d689e1 100644 --- a/src/query/mod.rs +++ b/src/query/mod.rs @@ -169,6 +169,13 @@ fn get_schema_provider(tenant_id: &Option) -> Box { } } +fn request_context(mut state: SessionState) -> SessionContext { + state + .config_mut() + .set_extension(Arc::new(small_file_cache::RequestView::default())); + SessionContext::new_with_state(state) +} + pub struct InMemorySessionContext { session_context: Arc>, } @@ -436,7 +443,9 @@ impl Query { ) )] pub async fn execute(&self, is_streaming: bool, tenant_id: &Option) -> QueryResult { - let ctx = QUERY_SESSION.get_ctx(); + // The catalog session is process-wide. Attach a fresh validation view + // only to this execution, retaining it through planning and streaming. + let ctx = request_context(QUERY_SESSION.get_ctx().state()); let mut logical_plan = self.final_logical_plan(tenant_id); let (hot_tier_guards, guarded_hot_tier_streams) = hot_tier_query_guards(&logical_plan, &self.time_range).await?; diff --git a/src/query/small_file_cache.rs b/src/query/small_file_cache.rs index 8586e1631..2e4a56024 100644 --- a/src/query/small_file_cache.rs +++ b/src/query/small_file_cache.rs @@ -16,7 +16,7 @@ * */ -//! Validated small-file bodies. Each physical scan validates before serving +//! Validated small-file bodies. Each request validates before serving //! bytes, so remote overwrites and deletes are visible without a TTL. Cache //! scope identifies the configured provider across per-query runtime instances. use bytes::Bytes; @@ -43,7 +43,6 @@ use std::{ const MAX_FILE: u64 = 256 * 1024; #[cfg(test)] const MAX_BYTES: usize = 64 * 1024 * 1024; -const SCAN_BYTES: usize = 8 * 1024 * 1024; const MAX_ENTRIES: usize = 4096; #[derive(Debug, Clone)] struct Entry { @@ -119,6 +118,9 @@ async fn load( namespace: &str, budget: usize, ) -> object_store::Result<(u64, Option)> { + if std::env::var_os("P_CACHE_TRACE").is_some() { + eprintln!("CACHE_LOAD scope={scope} path={path}"); + } let key = Path::from(format!("{namespace}{}", path.as_ref())); // Keep a private reference during validation; eviction cannot lose the body // needed for a 304. Do not hold the cache lock across network I/O. @@ -200,38 +202,37 @@ fn non_partial(error: &object_store::Error) -> bool { } } -// Metadata and data readers of one physical scan share a bounded view. A new -// physical scan always validates against storage; it cannot inherit this view. +// Validated bodies live for the request, including concurrent opens and separate +// physical scans. Unlike the process LRU this map must not evict: a late decoder +// must see the same validation as its metadata reader. Dropping the request's +// plans releases these references; subsequent requests get a fresh view. +type Loaded = std::result::Result<(u64, Option), Arc>; +#[derive(Debug, Default)] +pub(super) struct RequestView { + files: Mutex>>>, +} + async fn load_for_scan( - view: &Mutex, + view: &RequestView, scope: usize, store: &Arc, path: &Path, namespace: &str, budget: usize, ) -> object_store::Result<(u64, Option)> { - let key = Path::from(format!("{namespace}{}", path.as_ref())); - { - let mut view = view.lock().unwrap(); - if let Some(entry) = view.remove(scope, &key) { - let bytes = entry.bytes.clone(); - view.insert(entry, SCAN_BYTES); - return Ok((bytes.len() as u64, Some(bytes))); - } - } - let result = load(&CACHE, scope, store, path, namespace, budget).await?; - if let Some(bytes) = &result.1 { - view.lock().unwrap().insert( - Entry { - scope, - path: key, - etag: String::new(), - bytes: bytes.clone(), - }, - SCAN_BYTES, - ); - } - Ok(result) + let key = (scope, Path::from(format!("{namespace}{}", path.as_ref()))); + let cell = view.files.lock().unwrap().entry(key).or_default().clone(); + cell.get_or_init(|| async { + load(&CACHE, scope, store, path, namespace, budget) + .await + .map_err(Arc::new) + }) + .await + .clone() + .map_err(|source| object_store::Error::Generic { + store: "request small-file cache", + source: Box::new(source), + }) } pub(super) fn install( @@ -240,6 +241,7 @@ pub(super) fn install( scope: usize, budget: usize, enabled: bool, + view: Arc, ) -> datafusion::common::Result> { if !enabled || budget == 0 { return Ok(plan); @@ -284,7 +286,7 @@ pub(super) fn install( scope, namespace, budget, - view: Arc::new(Mutex::default()), + view, })); Ok(Arc::new( exec.clone().with_data_source(Arc::new( @@ -297,7 +299,7 @@ pub(super) fn install( #[derive(Debug)] pub(super) struct SmallFileReaderFactory { - view: Arc>, + view: Arc, scope: usize, namespace: String, budget: usize, @@ -312,6 +314,14 @@ impl ParquetFileReaderFactory for SmallFileReaderFactory { hint: Option, metrics: &ExecutionPlanMetricsSet, ) -> datafusion::common::Result> { + if std::env::var_os("P_CACHE_TRACE").is_some() { + eprintln!( + "CACHE_READER view={:p} partition={partition} path={} range={:?}", + Arc::as_ptr(&self.view), + file.object_meta.location, + file.range + ); + } let fallback = self .fallback .create_reader(partition, file.clone(), hint, metrics)?; @@ -340,7 +350,7 @@ impl ParquetFileReaderFactory for SmallFileReaderFactory { } struct Reader { hint: Option, - view: Arc>, + view: Arc, file_metrics: datafusion::datasource::physical_plan::ParquetFileMetrics, scope: usize, namespace: String, @@ -577,11 +587,11 @@ mod tests { } #[tokio::test] - async fn scan_view_is_bounded_and_new_scan_validates_changes() { + async fn request_view_retains_bodies_and_new_request_validates_changes() { let store: Arc = Arc::new(InMemory::new()); let scope = 99881; let path = Path::from("view/file.parquet"); - let view = Mutex::new(Cache::default()); + let view = RequestView::default(); store .put(&path, Bytes::from_static(b"one").into()) .await @@ -607,7 +617,7 @@ mod tests { "one", "one scan retains its validated body" ); - let next = Mutex::new(Cache::default()); + let next = RequestView::default(); assert_eq!( load_for_scan(&next, scope, &store, &path, "", MAX_BYTES) .await @@ -618,26 +628,106 @@ mod tests { "new scan must validate" ); store.delete(&path).await.unwrap(); - let next = Mutex::new(Cache::default()); + let next = RequestView::default(); assert!( load_for_scan(&next, scope, &store, &path, "", MAX_BYTES) .await .is_err() ); - let mut view = view.lock().unwrap(); + // A late reader must not revalidate even after traversing more than + // the old 8 MiB scan LRU. The process cache may still evict freely. for i in 0..40 { - view.insert( - Entry { - scope, - path: Path::from(format!("{i}")), - etag: String::new(), - bytes: Bytes::from(vec![0; MAX_FILE as usize]), + let other = Path::from(format!("view/{i}.parquet")); + store + .put(&other, Bytes::from(vec![0; MAX_FILE as usize]).into()) + .await + .unwrap(); + load_for_scan(&view, scope, &store, &other, "", MAX_BYTES) + .await + .unwrap(); + } + assert_eq!( + load_for_scan(&view, scope, &store, &path, "", MAX_BYTES) + .await + .unwrap() + .1 + .unwrap(), + "one" + ); + } + + #[tokio::test] + async fn concurrent_readers_share_validation_but_new_requests_do_not() { + use object_store::throttle::{ThrottleConfig, ThrottledStore}; + let store: Arc = Arc::new(MetricLayer::new( + ThrottledStore::new( + InMemory::new(), + ThrottleConfig { + wait_get_per_call: std::time::Duration::from_millis(10), + ..Default::default() }, - SCAN_BYTES, - ); + ), + "request-cache-concurrent", + )); + let path = Path::from("concurrent/file.parquet"); + store + .put(&path, Bytes::from_static(b"first").into()) + .await + .unwrap(); + for request in 0..2 { + let view = RequestView::default(); + let window = StorageMetricWindow::start("request-cache-concurrent"); + let readers = + (0..32).map(|_| load_for_scan(&view, 99882, &store, &path, "", MAX_BYTES)); + for result in futures_util::future::join_all(readers).await { + assert_eq!( + result.unwrap().1.unwrap(), + if request == 0 { "first" } else { "second" } + ); + } + assert_eq!(window.delta().get_requests, 1); + store + .put(&path, Bytes::from_static(b"second").into()) + .await + .unwrap(); + } + } + + #[tokio::test] + async fn request_context_keeps_catalog_and_isolates_validation_views() { + let shared = datafusion::prelude::SessionContext::new(); + shared + .register_table( + "sentinel", + Arc::new(datafusion::datasource::empty::EmptyTable::new(Arc::new( + arrow_schema::Schema::empty(), + ))), + ) + .unwrap(); + let first = super::super::request_context(shared.state()); + let second = super::super::request_context(shared.state()); + assert!(!Arc::ptr_eq( + &first + .state() + .config() + .get_extension::() + .unwrap(), + &second + .state() + .config() + .get_extension::() + .unwrap() + )); + assert!( + shared + .state() + .config() + .get_extension::() + .is_none() + ); + for context in [&shared, &first, &second] { + assert!(context.table("sentinel").await.is_ok()); } - assert!(view.bytes <= SCAN_BYTES); - assert!(view.remove(scope, &path).is_none()); } #[test] @@ -670,7 +760,7 @@ mod measurement { use super::*; use crate::{ promql::{self, QueryPlan, Sample, Series}, - query::stream_schema_provider::build_parquet_scan_components, + query::stream_schema_provider::build_parquet_scan_components_with_full_filters as build_parquet_scan_components, storage::{StorageMetricWindow, metrics_layer::MetricLayer}, }; use arrow_array::{Array, Float64Array, RecordBatch, StringArray, TimestampMillisecondArray}; @@ -690,6 +780,189 @@ mod measurement { use object_store::ObjectStoreExt; use parquet::arrow::ArrowWriter; + fn metric_writer(schema: &Schema) -> parquet::file::properties::WriterProperties { + use crate::{ + cli::{Cli, StorageOptions}, + event::format::{LogSource, LogSourceEntry}, + metadata::LogStreamMetadata, + parseable::Stream, + }; + use clap::Parser; + let StorageOptions::Local(args) = + Cli::try_parse_from(["parseable", "local-store", "target/cache-fixture-store"]) + .unwrap() + .storage + else { + unreachable!() + }; + let stream = Stream::new( + Arc::new(args.options), + "cache-fixture", + LogStreamMetadata { + log_source: vec![LogSourceEntry::new( + LogSource::OtelMetrics, + Default::default(), + )], + ..Default::default() + }, + None, + &None, + ); + stream.parquet_writer_props(schema, None, None, false) + } + + /// Synthetic historical minute partitions, using the ingest writer and manifest + /// builder. Publish before starting the isolated server; normal OTLP ingestion + /// can then populate its staging window without waiting 390 wall-clock minutes. + #[tokio::test] + #[ignore = "publishes an isolated HTTP reproduction fixture to MinIO"] + async fn publish_request_fixture() { + use crate::{ + catalog::{ + manifest::{Manifest, create_from_parquet_file, encode_manifest}, + snapshot::ManifestItem, + }, + event::format::{LogSource, LogSourceEntry}, + storage::ObjectStoreFormat, + }; + let endpoint = std::env::var("P_CACHE_TEST_MINIO_ENDPOINT").unwrap(); + let store = s3(&endpoint); + let schema = Arc::new(Schema::new(vec![ + Field::new( + "p_timestamp", + DataType::Timestamp(TimeUnit::Millisecond, None), + true, + ), + Field::new( + "time_unix_nano", + DataType::Timestamp(TimeUnit::Millisecond, None), + true, + ), + Field::new("metric_name", DataType::Utf8, true), + Field::new("metric_type", DataType::Utf8, true), + Field::new("host", DataType::Utf8, true), + Field::new("metric_description", DataType::Utf8, true), + Field::new("data_point_value", DataType::Float64, true), + ])); + let end = chrono::Utc::now().timestamp_millis() / 60_000 * 60_000; + let mut days = std::collections::BTreeMap::::new(); + let local = std::path::Path::new("target/cache-repro"); + std::fs::create_dir_all(local).unwrap(); + for minute in 3..393 { + let time = end - minute * 60_000; + let times: Vec<_> = (0..480).map(|i| time + (i % 120) * 500).collect(); + let batch = RecordBatch::try_new( + schema.clone(), + vec![ + Arc::new(TimestampMillisecondArray::from(times.clone())), + Arc::new(TimestampMillisecondArray::from(times)), + Arc::new(StringArray::from( + (0..480) + .map(|i| ["cpu", "disk", "memory", "network"][(i / 120) as usize]) + .collect::>(), + )), + Arc::new(StringArray::from(vec!["gauge"; 480])), + Arc::new(StringArray::from( + (0..480) + .map(|i| format!("host-{}", i % 12)) + .collect::>(), + )), + Arc::new(StringArray::from( + (0..480) + .map(|i| { + format!( + "{:016x}{:04x}", + ((minute * 480 + i) as u64).wrapping_mul(0x9e3779b97f4a7c15), + (i as u16).wrapping_mul(40503) + ) + }) + .collect::>(), + )), + Arc::new(Float64Array::from( + (0..480) + .map(|i| ((minute * 480 + i) as f64).sin()) + .collect::>(), + )), + ], + ) + .unwrap(); + let date = chrono::DateTime::from_timestamp_millis(time) + .unwrap() + .format("%Y-%m-%d") + .to_string(); + let path = format!("repro/date={date}/{minute}.parquet"); + let file = local.join("minute.parquet"); + let mut writer = ArrowWriter::try_new( + std::fs::File::create(&file).unwrap(), + schema.clone(), + Some(metric_writer(&schema)), + ) + .unwrap(); + writer.write(&batch).unwrap(); + writer.close().unwrap(); + days.entry(date) + .or_default() + .files + .push(create_from_parquet_file(path.clone(), &file).unwrap()); + store + .put( + &Path::from(path), + Bytes::from(std::fs::read(&file).unwrap()).into(), + ) + .await + .unwrap(); + } + let mut format = ObjectStoreFormat { + created_at: chrono::DateTime::from_timestamp_millis(end - 393 * 60_000) + .unwrap() + .to_rfc3339(), + log_source: vec![LogSourceEntry::new( + LogSource::OtelMetrics, + Default::default(), + )], + ..Default::default() + }; + for (date, manifest) in days { + let lower = chrono::NaiveDate::parse_from_str(&date, "%Y-%m-%d") + .unwrap() + .and_hms_opt(0, 0, 0) + .unwrap() + .and_utc(); + let path = format!("repro/date={date}/manifest.json"); + store + .put( + &Path::from(path.clone()), + encode_manifest(&manifest).unwrap().into(), + ) + .await + .unwrap(); + format.snapshot.manifest_list.push(ManifestItem { + manifest_path: path, + time_lower_bound: lower, + time_upper_bound: lower + chrono::Duration::days(1) + - chrono::Duration::nanoseconds(1), + events_ingested: 0, + ingestion_size: 0, + storage_size: 0, + }); + } + store + .put( + &Path::from("repro/.stream/.stream.json"), + Bytes::from(serde_json::to_vec(&format).unwrap()).into(), + ) + .await + .unwrap(); + store + .put( + &Path::from("repro/.stream/.schema"), + Bytes::from(serde_json::to_vec(schema.as_ref()).unwrap()).into(), + ) + .await + .unwrap(); + println!("Published 390 production-writer minute files ending at {end}"); + } + #[derive(Debug)] struct Table { schema: SchemaRef, @@ -705,6 +978,13 @@ mod measurement { fn table_type(&self) -> TableType { TableType::Base } + fn supports_filters_pushdown( + &self, + filters: &[&Expr], + ) -> datafusion::common::Result> + { + Ok(vec![datafusion::logical_expr::TableProviderFilterPushDown::Inexact; filters.len()]) + } async fn scan( &self, state: &dyn Session, @@ -734,6 +1014,10 @@ mod measurement { 12345, MAX_BYTES, true, + state + .config() + .get_extension::() + .unwrap_or_default(), ) } else if self.disabled { install( @@ -742,6 +1026,7 @@ mod measurement { 12345, MAX_BYTES, false, + Arc::default(), ) } else { Ok(plan) @@ -797,9 +1082,20 @@ mod measurement { ) .unwrap(); let mut body = Vec::new(); - let mut writer = ArrowWriter::try_new(&mut body, schema.clone(), None).unwrap(); + let mut writer = + ArrowWriter::try_new(&mut body, schema.clone(), Some(metric_writer(&schema))) + .unwrap(); writer.write(&batch).unwrap(); writer.close().unwrap(); + let metadata = ParquetMetaDataReader::new() + .parse_and_finish(&Bytes::copy_from_slice(&body)) + .unwrap(); + let metric = metadata + .row_group(0) + .column(schema.index_of("metric_name").unwrap()); + assert!(metric.bloom_filter_offset().is_some()); + assert!(metric.column_index_offset().is_some()); + assert!(metric.offset_index_offset().is_some()); let path = format!("metrics/{minute}.parquet"); files.push(PartitionedFile::new(path.clone(), body.len() as u64)); store @@ -826,7 +1122,6 @@ mod measurement { for (name, sql) in queries { let mut reference = None; let mut reference_bytes = None; - let mut reference_requests = None; for (cached, disabled) in [(false, false), (false, true), (true, false)] { if cached { let mut cache = CACHE.lock().unwrap(); @@ -843,7 +1138,19 @@ mod measurement { for run in 0..2 { // Production builds a new query session; the process cache // survives these sessions, DataFusion's session cache doesn't. - let mut config = SessionConfig::new().with_target_partitions(4); + let mut config = SessionConfig::new() + .with_target_partitions(32) + .with_extension(Arc::new(RequestView::default())); + config + .options_mut() + .execution + .enable_file_stream_work_stealing = true; + config + .options_mut() + .execution + .use_row_number_estimates_to_optimize_partitioning = true; + // Exercise byte-range splits even for this compact fixture. + config.options_mut().optimizer.repartition_file_min_size = 1; config.options_mut().execution.parquet.metadata_size_hint = Some(2 * 1024 * 1024); config.options_mut().execution.parquet.enable_page_index = true; @@ -933,16 +1240,9 @@ mod measurement { } } let delta = window.delta(); - let requests = (delta.get_requests, delta.get_ranges_requests); - if disabled { - assert_eq!( - Some(requests), - reference_requests, - "kill switch must preserve original HTTP request pattern" - ); - } else if !cached { - reference_requests = Some(requests); - } + // The original reader's request count varies with concurrent + // metadata-cache races across byte-range partitions. Plan + // identity below tests the kill switch deterministically. println!( "CACHE_MEASUREMENT {name} cached={cached} disabled={disabled} run={run} GET={} GET_RANGES={}", delta.get_requests, delta.get_ranges_requests @@ -994,12 +1294,28 @@ mod measurement { }; let plan = table.scan(&ctx.state(), None, &[], None).await.unwrap(); for (enabled, budget) in [(false, MAX_BYTES), (true, 0), (true, 9)] { - let actual = install(plan.clone(), store.clone(), 0, budget, enabled).unwrap(); + let actual = install( + plan.clone(), + store.clone(), + 0, + budget, + enabled, + Arc::default(), + ) + .unwrap(); assert!(Arc::ptr_eq(&actual, &plan)); } if count > MAX_ENTRIES { assert!(Arc::ptr_eq( - &install(plan.clone(), store.clone(), 0, MAX_BYTES, true).unwrap(), + &install( + plan.clone(), + store.clone(), + 0, + MAX_BYTES, + true, + Arc::default() + ) + .unwrap(), &plan )); } diff --git a/src/query/stream_schema_provider.rs b/src/query/stream_schema_provider.rs index 31019c43d..44a32f1ee 100644 --- a/src/query/stream_schema_provider.rs +++ b/src/query/stream_schema_provider.rs @@ -409,6 +409,10 @@ async fn parquet_scan_plan( Arc::as_ptr(&PARSEABLE.storage) as *const () as usize, PARSEABLE.options.small_file_cache_size, PARSEABLE.options.small_file_cache_enabled, + state + .config() + .get_extension::() + .unwrap_or_default(), ) } else { Ok(plan) From ff69171e348b49ef8f255a6fa3f773bd2b0949c7 Mon Sep 17 00:00:00 2001 From: VibeSWE <338779468+VibeSWE@users.noreply.github.com> Date: Wed, 11 Nov 2026 08:50:53 -0600 Subject: [PATCH 4/5] fix: bound parquet request cache and reject ranged file growth --- docs/log-severity-clustering.md | 4 +- .../performance/validated-small-file-cache.md | 114 +++- scripts/performance/request_cache_repro.py | 3 +- src/query/small_file_cache.rs | 561 +++++++++++++++--- src/query/stream_schema_provider.rs | 56 +- .../fixtures}/cache_http_proxy.py | 2 +- 6 files changed, 635 insertions(+), 105 deletions(-) rename {src/query => tests/fixtures}/cache_http_proxy.py (94%) diff --git a/docs/log-severity-clustering.md b/docs/log-severity-clustering.md index 21714d9f4..d4a745e6f 100644 --- a/docs/log-severity-clustering.md +++ b/docs/log-severity-clustering.md @@ -83,8 +83,8 @@ of the base commit and this change with the same toolchain and lockfile. The server was pinned to four CPUs with `s3-store` on a local MinIO. Each arm ingested the same 3.6 million JSON rows (about 1% `ERROR`, 22 fields), event-time partitioned, with default 262,144-row groups. Values are medians over three -rotated blocks. Bytes are the server's logical requested ranges -(`parseable_storage_read_bytes_total`), not physical GETs. The run used +rotated blocks. Bytes are successful response body bytes returned by storage +(`parseable_storage_read_bytes_total`), not physical GET counts. The run used `scripts/ingest-performance/clustering_bench.py`. | Measure | Baseline | Clustering off | Clustering on | diff --git a/docs/performance/validated-small-file-cache.md b/docs/performance/validated-small-file-cache.md index d1f500b53..f15e0ba26 100644 --- a/docs/performance/validated-small-file-cache.md +++ b/docs/performance/validated-small-file-cache.md @@ -44,7 +44,7 @@ and column reads then use slices of the same body. On a subsequent request, a conditional GET validates the cached ETag. A 304 reuses the bytes without a body; a changed object replaces them. A 404/error invalidates the entry and propagates. Absent or weak (`W/`) ETags mean no cross-request body reuse. Growth past the limit uses the original -reader with the newly observed size. The bounded probe prevents an unexpected +reader with the newly observed size only for unsplit files; ranged growth fails. The bounded probe prevents an unexpected large replacement from being read into memory in full. Keys use the process's singleton storage-provider identity, object-store URL, and full object path, @@ -56,8 +56,8 @@ concurrent metadata, bloom-filter and decoder opens. Validated bodies stay refer until the request finishes; they are not evicted by a second, smaller LRU. New requests never inherit the view. The keyed linked-list LRU performs lookup, promotion, and removal in O(1), without holding its mutex across I/O. Storage defaults to 64 MiB and 4,096 entries. Active readers -can retain evicted bodies; as with existing scan buffers, their memory also depends -on query concurrency. This limit is a cache residency limit, not a process RSS cap. +can retain evicted bodies under the separate 64 MiB process-wide view reservation +limit described below. Neither limit is a process RSS cap. Stream/tenant removal and retention need no cached catalog invalidation because catalog objects aren't cached across requests. They stop selecting deleted files. @@ -83,7 +83,7 @@ across dashboard refreshes. No TLS handshake benefit has been measured here. ## Reproduction Build locally with `PATH=$HOME/.cargo/bin:$PATH`, `P_BUILD_NO_UI=1`, and -`CARGO_BUILD_JOBS=4`. Run one Cargo command at a time: +`CARGO_BUILD_JOBS=6`. Run one Cargo command at a time: ``` cargo test --lib query::small_file_cache -- --nocapture @@ -97,7 +97,7 @@ cargo clippy --all-targets The measurement uses MetricLayer and StorageMetricWindow, hence the same `parseable_storage_request_response_time_count` samples exposed by `/api/v1/metrics`. It generates 60 one-minute parquet files, opens a fresh DataFusion query session -for each run, compares returned rows, and evaluates a PromQL rate range expression +for each run, sharing a runtime for each cold/warm pair, compares returned rows, and evaluates a PromQL rate range expression from the scanned samples. It isolates parquet I/O: HTTP handlers, schema refresh, `stream.json` and manifest reads are outside the measurement. Do not interpret its table as a reproduction of the supplied live-instance endpoint totals. @@ -136,8 +136,7 @@ the original DataFusion reader and its request pattern. `--small-file-cache-size` / `P_SMALL_FILE_CACHE_SIZE` defaults to `67108864` bytes (64 MiB); zero also disables installation. The process cache entry cap stays at 4,096. The request view retains admitted files until completion (including streaming completion); its references can outlive -process-LRU residency, just as active readers can. These are residency limits, not RSS -limits. No new required configuration or dependency was added. +process-LRU residency. See the bounded admission policy below. No new required configuration or dependency was added. A scan whose selected small-file manifest sizes exceed the configured byte limit, or whose small-file count exceeds 4,096, bypasses installation entirely. It cannot @@ -232,7 +231,7 @@ bloom filters, page indexes and sorted metric names. This is synthetic fixture publication, not a claim that 390 files were ingested over wall-clock minutes. The HTTP script ingests an actual OTLP gauge into staging before querying. Files are approximately 19.7 KB. Run a debug standalone server against this isolated -bucket with `P_CACHE_TRACE=1`, then run +bucket with `RUST_LOG=parseable::query::small_file_cache=trace`, then run `scripts/performance/request_cache_repro.py`. Set `CACHE_REPRO_REQUESTS` to a worktree-local JSON path to freeze the exact request ranges for before/after; set `CACHE_REPRO_ASSERT_ONCE=1` for the regression assertions. It performs two @@ -288,3 +287,102 @@ and staging API compatibility warning; no warnings point to changed code. The temporary standalone process was stopped and the MinIO container and its anonymous volume were removed. No live-service settings, data or processes were changed. No push or PR was performed. + + +## Review fixes and memory bound + +Admission accumulates unique selected manifest bytes across all scans in a request, +including UNION branches, up to the configured budget and 4,096 paths. Separately, +all live request views share a fixed 64 MiB reservation pool. Admission reserves +256 KiB per distinct small file, so stale manifest sizes cannot underreserve a +body. If either limit is exhausted the entire new scan uses the original reader +immediately. Duplicate paths within an admitted request need no extra reservation. +This conservative reservation can bypass a scan even when its actual bodies would +fit. A bypassed scan has the original reader's I/O costs. + +Resident cache-owned body memory is bounded by the configured process LRU budget +plus 64 MiB in request views/readers (128 MiB at defaults), counting shared +allocations conservatively twice. During validation, a loader can temporarily +hold both the previous body and its replacement; at most one loader per reserved +file adds at most another 64 MiB. The worst-case body-memory bound including these +transient references is therefore the LRU budget plus 128 MiB (192 MiB at defaults). +This excludes ordinary DataFusion scan buffers, HTTP client +buffers, decoded batches, metadata and small map/error overhead. It is not an RSS +cap. Large-to-small recovery uses the original reader and cannot acquire an +unreserved body. Every reader owns its view, keeping the reservation alive until +its bytes can be dropped. Bodies remain in the view until request completion: +DataFusion opens replacement decoders and later scan branches after prior readers +have dropped, so reader-count-based release would restore double validation. +The process bound covers concurrent requests, including slow streaming clients. + +DataFusion memory-pool registration is not used: these immutable shared Bytes +cross runtime/pool boundaries through the process LRU; assigning a reservation to +one pool would outlive that query or double-charge shared allocations. Admission +and process reservations enforce a separate explicit bound without blocking. + +Ranged scans reject growth past the planned manifest EOF, including small-body +loads. Existing partitions cannot cover new row groups beyond that EOF. Shrinkage +can still recover; HEAD failures preserve the original metadata error. + +Trace instrumentation uses `tracing::trace!`, enabled by +`RUST_LOG=parseable::query::small_file_cache=trace`; configure plain, uncolored log +output for the reproduction script. The script uses default development +credentials (`admin` / `admin`) and must target only an isolated development +server. The HTTP fixture proxy is located under `tests/fixtures` via the crate +root, independent of the test process working directory. + +The boolean parser was left unchanged after checking clap's sources and adding +parity coverage with the existing `--dashboard-agent-writes` option: both accept +only lowercase `true` and `false`; `1`, `0`, `yes`, `no` and uppercase variants +are rejected by both. Broadening just this option would break that consistency. + +## Review remeasurement (2026-11-11) + +The 60-file, 579,821-byte fixture was run again against isolated real MinIO HTTP, +through the production scan builder, forcing 32 byte-range partitions. Each pair +uses separate request sessions on one shared runtime. The original reader and +disabled-cache reader produced the same counts. All rows, evaluated PromQL points, +and billing `bytes_scanned` matched for both cold and warm runs. + +| Shape | Original cold GET / GET_RANGES | Original shared-runtime warm | Pre-fix cache | Fixed cold | Fixed shared-runtime warm | +| --- | ---: | ---: | ---: | ---: | ---: | +| COUNT | 0 / 91 | 0 / 0 | 60 / 0 | 60 / 0 | 60 / 0 | +| Filtered GROUP BY | 1 / 211 | 1 / 120 | 60 / 0 | 60 / 0 | 60 / 0 | +| PromQL scan and evaluation | 120 / 270 | 120 / 180 | 60 / 0 | 60 / 0 | 60 / 0 | + +All columns were remeasured here. The pre-fix cache used the implementation from +633b8e13 with only the harness changed to share a runtime for cold/warm pairs. +Pre-fix and fixed cache counts were identical for all three shapes, cold and warm. +Every fixed warm run issued 60 conditional GETs, received 60 not-modified outcomes, +and transferred zero body bytes. Cold fixed runs transferred exactly 579,821 bytes. +A warm metadata-only COUNT is cheaper with the original reader's runtime metadata +cache; the validated cache intentionally pays one validation per selected file to +observe remote mutations. Admission bypass preserves original-reader behavior. + +The new non-ignored production-path test uses two distinct UNION scans, each split +across byte ranges, through `request_context` and the production scan builder. It +asserts four GETs for four files on each of two independent requests, so losing +the request extension causes duplicate validations across the UNION branches. +The ranged-growth regression was also run with the growth guards temporarily +removed: it failed because a query succeeded despite row groups beyond manifest +EOF. Restoring the guards makes all three growth cases fail safely. + +An additional dependency limitation was observed while constructing a fault test: +with a stale size and a range starting at zero, a short HTTP body can reach a +Parquet 59 buffer-length assertion in the original DataFusion reader. The HEAD +failure test instead uses a size-consistent corrupt body to isolate error +precedence. The cache-disabled reader is unchanged; this separate upstream +short-range panic was not changed by these fixes. + +The private object_store non-partial-error contract also has a non-ignored CI +test using a minimal loopback HTTP server returning 200 to a Range request; +this exercises the dependency's real response parser without MinIO. + +Final review validation: the full library suite passed with 1,393 passed, zero +failed and seven visibly ignored tests. All 15 cache tests then passed with +`--include-ignored --nocapture --test-threads=1` against isolated MinIO, including +all three opt-in HTTP tests and the independent loopback HTTP contract test. +Plain `cargo clippy --all-targets` passed with the existing 21 library-test +warnings and staging API compatibility warning; none refer to changed code. +The temporary MinIO container and its anonymous volume were removed. No live +service, configuration or bucket was used. No push or PR change was performed. diff --git a/scripts/performance/request_cache_repro.py b/scripts/performance/request_cache_repro.py index b808c9ce6..a6e368614 100644 --- a/scripts/performance/request_cache_repro.py +++ b/scripts/performance/request_cache_repro.py @@ -52,7 +52,8 @@ def query(sql, minutes): after = metrics() with open(LOG) as trace: trace.seek(offset) - loads = collections.Counter(re.findall(r'CACHE_LOAD .*path=(\S+)',trace.read())) + lines = re.sub(r'\x1b\[[0-9;]*m', '', trace.read()).splitlines() + loads = collections.Counter(path for line in lines if 'CACHE_LOAD' in line for path in re.findall(r'\bpath=(\S+)', line)) if run: print(json.dumps({'shape':name,'calls':int(after-before),'selected_files':len(loads),'validations':sum(loads.values()),'multiplicity':dict(collections.Counter(loads.values()))}), flush=True) if os.environ.get('CACHE_REPRO_ASSERT_ONCE'): diff --git a/src/query/small_file_cache.rs b/src/query/small_file_cache.rs index 2e4a56024..01f79a390 100644 --- a/src/query/small_file_cache.rs +++ b/src/query/small_file_cache.rs @@ -37,7 +37,10 @@ use parquet::{ use std::{ collections::HashMap, ops::Range, - sync::{Arc, LazyLock, Mutex}, + sync::{ + Arc, LazyLock, Mutex, + atomic::{AtomicUsize, Ordering}, + }, }; const MAX_FILE: u64 = 256 * 1024; @@ -118,9 +121,7 @@ async fn load( namespace: &str, budget: usize, ) -> object_store::Result<(u64, Option)> { - if std::env::var_os("P_CACHE_TRACE").is_some() { - eprintln!("CACHE_LOAD scope={scope} path={path}"); - } + tracing::trace!(scope, path = %path, "CACHE_LOAD"); let key = Path::from(format!("{namespace}{}", path.as_ref())); // Keep a private reference during validation; eviction cannot lose the body // needed for a 304. Do not hold the cache lock across network I/O. @@ -179,7 +180,10 @@ async fn load( Err(error) if non_partial(&error) => { cache.lock().unwrap().remove(scope, &key); // HEAD obtains the current size without trusting a stale manifest. - Ok((store.head(path).await?.size, None)) + match store.head(path).await { + Ok(meta) => Ok((meta.size, None)), + Err(_) => Err(error), + } } Err(error) => { cache.lock().unwrap().remove(scope, &key); @@ -190,9 +194,16 @@ async fn load( fn non_partial(error: &object_store::Error) -> bool { use std::error::Error; + // object_store 0.13's private GetResultError::NotPartial is exposed only + // through Generic. Match its leaf message, never arbitrary wrapper text. + if !matches!(error, object_store::Error::Generic { .. }) { + return false; + } let mut cause: &dyn Error = error; loop { - if cause.to_string() == "Received non-partial response when range requested" { + if cause.source().is_none() + && cause.to_string() == "Received non-partial response when range requested" + { return true; } match cause.source() { @@ -210,6 +221,108 @@ type Loaded = std::result::Result<(u64, Option), Arc #[derive(Debug, Default)] pub(super) struct RequestView { files: Mutex>>>, + admission: Mutex, +} + +// Reserve the maximum possible small body, rather than trusting manifest sizes. +// Reservations also cover bodies held by readers after their plans are dropped: +// every reader owns the view. No network operation waits for capacity. +const VIEW_BYTES_LIMIT: usize = 64 * 1024 * 1024; +static VIEW_BYTES: AtomicUsize = AtomicUsize::new(0); +#[derive(Debug, Default)] +struct Admission { + predicted: u128, + reserved: usize, + files: HashMap, +} +impl RequestView { + fn admit(&self, files: HashMap, budget: usize) -> bool { + let mut admission = self.admission.lock().unwrap(); + let new: Vec<_> = files + .into_iter() + .filter(|(key, _)| !admission.files.contains_key(key)) + .collect(); + let predicted = + admission.predicted + new.iter().map(|(_, size)| *size as u128).sum::(); + if predicted > budget as u128 || admission.files.len() + new.len() > MAX_ENTRIES { + return false; + } + let reserve = new.len() * MAX_FILE as usize; + if VIEW_BYTES + .fetch_update(Ordering::AcqRel, Ordering::Acquire, |used| { + used.checked_add(reserve) + .filter(|total| *total <= VIEW_BYTES_LIMIT) + }) + .is_err() + { + return false; + } + admission.predicted = predicted; + admission.reserved += reserve; + admission.files.extend(new); + true + } +} +impl Drop for RequestView { + fn drop(&mut self) { + // Drop bodies before making their reserved capacity available again. + self.files.get_mut().unwrap().clear(); + VIEW_BYTES.fetch_sub(self.admission.get_mut().unwrap().reserved, Ordering::AcqRel); + } +} + +// object_store::Error is not Clone. Reconstruct public classifications while +// retaining the original error and its source chain in the shared source. +fn shared_error(error: Arc) -> object_store::Error { + use object_store::Error::*; + match error.as_ref() { + NotFound { path, .. } => NotFound { + path: path.clone(), + source: Box::new(error), + }, + AlreadyExists { path, .. } => AlreadyExists { + path: path.clone(), + source: Box::new(error), + }, + Precondition { path, .. } => Precondition { + path: path.clone(), + source: Box::new(error), + }, + NotModified { path, .. } => NotModified { + path: path.clone(), + source: Box::new(error), + }, + PermissionDenied { path, .. } => PermissionDenied { + path: path.clone(), + source: Box::new(error), + }, + Unauthenticated { path, .. } => Unauthenticated { + path: path.clone(), + source: Box::new(error), + }, + NotSupported { .. } => NotSupported { + source: Box::new(error), + }, + NotImplemented { + operation, + implementer, + } => NotImplemented { + operation: operation.clone(), + implementer: implementer.clone(), + }, + UnknownConfigurationKey { store, key } => UnknownConfigurationKey { + store, + key: key.clone(), + }, + Generic { store, .. } => Generic { + store, + source: Box::new(error), + }, + _ => Generic { + store: "request small-file cache", + source: Box::new(error), + }, + } } async fn load_for_scan( @@ -229,10 +342,7 @@ async fn load_for_scan( }) .await .clone() - .map_err(|source| object_store::Error::Generic { - store: "request small-file cache", - source: Box::new(source), - }) + .map_err(shared_error) } pub(super) fn install( @@ -256,23 +366,25 @@ pub(super) fn install( let (conf, source) = exec .downcast_to_file_source::() .expect("parquet source"); - // Bypass scans whose predicted working set cannot survive one traversal. - let small = conf + let namespace = conf.object_store_url.to_string(); + let files = conf .file_groups .iter() .flat_map(|group| group.iter()) .filter(|file| file.object_meta.size <= MAX_FILE) - .collect::>(); - if small.len() > MAX_ENTRIES - || small - .iter() - .map(|file| file.object_meta.size as u128) - .sum::() - > budget as u128 - { + .map(|file| { + ( + ( + scope, + Path::from(format!("{namespace}{}", file.object_meta.location)), + ), + file.object_meta.size, + ) + }) + .collect(); + if !view.admit(files, budget) { return Ok(plan); } - let namespace = conf.object_store_url.to_string(); let fallback = source .parquet_file_reader_factory() .cloned() @@ -314,14 +426,7 @@ impl ParquetFileReaderFactory for SmallFileReaderFactory { hint: Option, metrics: &ExecutionPlanMetricsSet, ) -> datafusion::common::Result> { - if std::env::var_os("P_CACHE_TRACE").is_some() { - eprintln!( - "CACHE_READER view={:p} partition={partition} path={} range={:?}", - Arc::as_ptr(&self.view), - file.object_meta.location, - file.range - ); - } + tracing::trace!(view = ?Arc::as_ptr(&self.view), partition, path = %file.object_meta.location, range = ?file.range, "CACHE_READER"); let fallback = self .fallback .create_reader(partition, file.clone(), hint, metrics)?; @@ -338,6 +443,7 @@ impl ParquetFileReaderFactory for SmallFileReaderFactory { scope: self.scope, namespace: self.namespace.clone(), budget: self.budget, + planned_size: file.object_meta.size, file, partition, metrics: metrics.clone(), @@ -349,6 +455,7 @@ impl ParquetFileReaderFactory for SmallFileReaderFactory { } } struct Reader { + planned_size: u64, hint: Option, view: Arc, file_metrics: datafusion::datasource::physical_plan::ParquetFileMetrics, @@ -364,10 +471,17 @@ struct Reader { initialized: bool, bytes: Option, } +impl Drop for Reader { + fn drop(&mut self) { + // The view may be this reader's last reference. Release private body + // ownership before its reservation can be returned by RequestView::drop. + self.bytes = None; + } +} impl Reader { async fn init(&mut self) -> Result<()> { if !self.initialized { - if self.file.object_meta.size > MAX_FILE { + if self.planned_size > MAX_FILE { self.initialized = true; return Ok(()); } @@ -381,6 +495,11 @@ impl Reader { ) .await .map_err(|e| ParquetError::External(Box::new(e)))?; + if self.file.range.is_some() && size > self.planned_size { + return Err(ParquetError::General( + "Stale manifest: ranged parquet file grew beyond planned size".into(), + )); + } if bytes.is_none() { self.file.object_meta.size = size; self.fallback = self @@ -438,12 +557,13 @@ impl AsyncFileReader for Reader { Err(error) => { // A formerly large object may have shrunk while the // manifest still describes its old footer position. - let current = self - .store - .head(&self.file.object_meta.location) - .await - .map_err(|e| ParquetError::External(Box::new(e)))?; - if current.size == self.file.object_meta.size { + let current = match self.store.head(&self.file.object_meta.location).await { + Ok(current) => current, + Err(_) => return Err(error), + }; + if current.size == self.file.object_meta.size + || (self.file.range.is_some() && current.size > self.planned_size) + { return Err(error); } self.file.object_meta = current; @@ -730,6 +850,119 @@ mod tests { } } + #[tokio::test] + async fn shared_not_found_keeps_its_kind() { + let store: Arc = Arc::new(InMemory::new()); + let view = RequestView::default(); + for _ in 0..2 { + assert!(matches!( + load_for_scan(&view, 98901, &store, &Path::from("absent"), "", MAX_BYTES).await, + Err(object_store::Error::NotFound { .. }) + )); + } + } + + #[tokio::test] + async fn dependency_non_partial_error_contract() { + use tokio::io::{AsyncReadExt, AsyncWriteExt}; + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let address = listener.local_addr().unwrap(); + let server = tokio::spawn(async move { + let (mut socket, _) = listener.accept().await.unwrap(); + let mut request = Vec::new(); + while !request.ends_with(b"\r\n\r\n") { + request.push(socket.read_u8().await.unwrap()); + } + assert!( + String::from_utf8(request) + .unwrap() + .to_ascii_lowercase() + .contains("range: bytes=0-1") + ); + socket.write_all(b"HTTP/1.1 200 OK\r\nContent-Length: 3\r\nETag: \"v1\"\r\nConnection: close\r\n\r\nabc").await.unwrap(); + }); + let store = object_store::aws::AmazonS3Builder::new() + .with_endpoint(format!("http://{address}")) + .with_allow_http(true) + .with_region("us-east-1") + .with_bucket_name("test") + .with_access_key_id("test") + .with_secret_access_key("test") + .with_retry(object_store::RetryConfig { + max_retries: 0, + ..Default::default() + }) + .build() + .unwrap(); + let error = store + .get_opts( + &Path::from("file"), + GetOptions { + range: Some(GetRange::Bounded(0..2)), + ..Default::default() + }, + ) + .await + .unwrap_err(); + server.await.unwrap(); + assert!(matches!(error, object_store::Error::Generic { .. })); + assert!( + non_partial(&error), + "object_store's private non-partial error contract changed: {error:?}" + ); + let unrelated = object_store::Error::Generic { + store: "test", + source: Box::new(std::io::Error::other("unrelated")), + }; + assert!(!non_partial(&unrelated)); + } + + #[test] + fn cache_boolean_matches_existing_boolean_options() { + use clap::Parser; + for value in ["true", "false", "1", "0", "yes", "no", "TRUE", "FALSE"] { + let cache = crate::cli::Cli::try_parse_from([ + "parseable", + "local-store", + "target/cache-cli-test", + "--small-file-cache-enabled", + value, + ]); + let existing = crate::cli::Cli::try_parse_from([ + "parseable", + "local-store", + "target/cache-cli-test", + "--dashboard-agent-writes", + value, + ]); + assert_eq!(cache.is_ok(), existing.is_ok(), "{value}"); + assert_eq!(cache.is_ok(), matches!(value, "true" | "false")); + } + } + + #[test] + fn cumulative_admission_and_process_bound() { + let view = RequestView::default(); + let files = |name: &str, size| [((717, Path::from(name)), size)].into(); + assert!(view.admit(files("a", 10), 15)); + assert!( + view.admit(files("a", 10), 15), + "repeated paths cost no extra reservation" + ); + assert!( + !view.admit(files("b", 10), 15), + "UNION scans share a request budget" + ); + assert_eq!(view.admission.lock().unwrap().reserved, MAX_FILE as usize); + let too_many = (0..257) + .map(|i| ((718, Path::from(i.to_string())), 1)) + .collect(); + assert!( + !RequestView::default().admit(too_many, MAX_BYTES), + "process capacity bypasses without waiting" + ); + } + #[test] fn bounds_eviction_and_replacement() { let mut cache = Cache::default(); @@ -760,18 +993,13 @@ mod measurement { use super::*; use crate::{ promql::{self, QueryPlan, Sample, Series}, - query::stream_schema_provider::build_parquet_scan_components_with_full_filters as build_parquet_scan_components, storage::{StorageMetricWindow, metrics_layer::MetricLayer}, }; use arrow_array::{Array, Float64Array, RecordBatch, StringArray, TimestampMillisecondArray}; use arrow_schema::{DataType, Field, Schema, SchemaRef, TimeUnit}; use datafusion::{ catalog::Session, - datasource::{ - TableProvider, - file_format::FileFormat, - physical_plan::{FileGroup, FileScanConfigBuilder}, - }, + datasource::TableProvider, execution::object_store::ObjectStoreUrl, logical_expr::TableType, physical_plan::ExecutionPlan, @@ -992,46 +1220,174 @@ mod measurement { filters: &[Expr], limit: Option, ) -> datafusion::common::Result> { - let url = ObjectStoreUrl::parse("s3://cache-measurement").unwrap(); - let (format, source) = - build_parquet_scan_components(self.schema.clone(), filters, state)?; - let mut builder = FileScanConfigBuilder::new(url.clone(), source.into()) - .with_file_groups( - self.files - .chunks(15) - .map(|files| FileGroup::new(files.to_vec())) - .collect(), - ) - .with_limit(limit); - if let Some(p) = projection { - builder = builder.with_projection_indices(Some(p.clone()))?; + super::super::stream_schema_provider::parquet_scan_plan_with_cache_options( + self.schema.clone(), + ObjectStoreUrl::parse("s3://cache-measurement").unwrap(), + self.files.chunks(15).map(|files| files.to_vec()).collect(), + datafusion::common::Statistics::new_unknown(&self.schema), + projection, + filters, + limit, + state, + 8192, + (12345, MAX_BYTES, self.cached && !self.disabled), + ) + .await + } + } + + fn split_context(store: Arc, files: Vec) -> SessionContext { + let mut config = SessionConfig::new().with_target_partitions(32); + config.options_mut().optimizer.repartition_file_min_size = 1; + config.options_mut().optimizer.repartition_file_scans = true; + config + .options_mut() + .execution + .enable_file_stream_work_stealing = true; + let shared = SessionContext::new_with_config(config); + shared + .runtime_env() + .register_object_store(&url::Url::parse("s3://cache-measurement").unwrap(), store); + shared + .register_table( + "metrics", + Arc::new(Table { + schema: Arc::new(Schema::new(vec![Field::new( + "value", + DataType::Int64, + false, + )])), + files, + cached: true, + disabled: false, + }), + ) + .unwrap(); + shared + } + + #[tokio::test] + async fn production_request_validates_once_across_range_readers() { + let store: Arc = Arc::new(MetricLayer::new( + object_store::memory::InMemory::new(), + "production-request", + )); + let body = parquet((0..1000).collect()); + let mut files = Vec::new(); + for i in 0..4 { + let path = format!("production/{i}.parquet"); + store + .put(&Path::from(path.clone()), body.clone().into()) + .await + .unwrap(); + files.push(PartitionedFile::new(path, body.len() as u64)); + } + let shared = split_context(store, files); + for _ in 0..2 { + let ctx = super::super::request_context(shared.state()); + let window = StorageMetricWindow::start("production-request"); + let plan = ctx + .sql("SELECT SUM(value) FROM metrics WHERE value >= 0 UNION ALL SELECT SUM(value) FROM metrics WHERE value > -1") + .await + .unwrap() + .create_physical_plan() + .await + .unwrap(); + let display = datafusion::physical_plan::displayable(plan.as_ref()) + .indent(true) + .to_string(); + assert!( + display.contains("parquet:0.."), + "must split into byte ranges: {display}" + ); + let batches = datafusion::physical_plan::collect(plan, ctx.task_ctx()) + .await + .unwrap(); + assert_eq!( + batches.iter().map(|batch| batch.num_rows()).sum::(), + 2 + ); + for batch in &batches { + for value in batch + .column(0) + .as_any() + .downcast_ref::() + .unwrap() + .values() + { + assert_eq!(*value, 4 * 999 * 1000 / 2); + } } - let plan = format.create_physical_plan(state, builder.build()).await?; - if self.cached { - install( - plan, - state.runtime_env().object_store(url)?, - 12345, - MAX_BYTES, - true, - state - .config() - .get_extension::() - .unwrap_or_default(), - ) - } else if self.disabled { - install( - plan, - state.runtime_env().object_store(url)?, - 12345, - MAX_BYTES, - false, - Arc::default(), + assert_eq!(window.delta().get_requests, 4); + assert_eq!(window.delta().get_ranges_requests, 0); + } + } + + #[tokio::test] + async fn ranged_growth_must_fail_instead_of_losing_row_groups() { + let store: Arc = Arc::new(object_store::memory::InMemory::new()); + // Separate uncompressed row groups put first-page offsets beyond the + // stale manifest EOF, for both formerly small and formerly large files. + let batch = RecordBatch::try_from_iter(vec![( + "value", + Arc::new(arrow_array::Int64Array::from( + (0..100_000).collect::>(), + )) as arrow_array::ArrayRef, + )]) + .unwrap(); + let mut body = Vec::new(); + let props = parquet::file::properties::WriterProperties::builder() + .set_max_row_group_row_count(Some(1000)) + .set_dictionary_enabled(false) + .build(); + let mut writer = ArrowWriter::try_new(&mut body, batch.schema(), Some(props)).unwrap(); + writer.write(&batch).unwrap(); + writer.close().unwrap(); + let metadata = ParquetMetaDataReader::new() + .parse_and_finish(&Bytes::copy_from_slice(&body)) + .unwrap(); + for planned in [1024, MAX_FILE + 1024] { + assert!( + metadata + .row_groups() + .iter() + .any(|rg| rg.column(0).data_page_offset() as u64 >= planned) + ); + store + .put( + &Path::from("growth.parquet"), + Bytes::copy_from_slice(&body).into(), ) - } else { - Ok(plan) - } + .await + .unwrap(); + let shared = split_context( + store.clone(), + vec![PartitionedFile::new("growth.parquet", planned)], + ); + let ctx = super::super::request_context(shared.state()); + let df = ctx.sql("SELECT SUM(value) FROM metrics").await.unwrap(); + assert!( + df.collect().await.is_err(), + "growth beyond planned ranges must not succeed with missing rows" + ); } + // A fetched small body may also grow beyond its stale planned ranges. + let body = parquet((0..1000).collect()); + store + .put(&Path::from("growth.parquet"), body.into()) + .await + .unwrap(); + let ctx = super::super::request_context( + split_context(store, vec![PartitionedFile::new("growth.parquet", 32)]).state(), + ); + assert!( + ctx.sql("SELECT SUM(value) FROM metrics") + .await + .unwrap() + .collect() + .await + .is_err() + ); } // This uses the production ParquetFormat, scan components, metrics layer, @@ -1135,9 +1491,8 @@ mod measurement { ); } } + let runtime = Arc::new(datafusion::execution::runtime_env::RuntimeEnv::default()); for run in 0..2 { - // Production builds a new query session; the process cache - // survives these sessions, DataFusion's session cache doesn't. let mut config = SessionConfig::new() .with_target_partitions(32) .with_extension(Arc::new(RequestView::default())); @@ -1155,7 +1510,9 @@ mod measurement { Some(2 * 1024 * 1024); config.options_mut().execution.parquet.enable_page_index = true; config.options_mut().execution.parquet.pushdown_filters = true; - let ctx = SessionContext::new_with_config(config); + let ctx = super::super::request_context( + SessionContext::new_with_config_rt(config, runtime.clone()).state(), + ); ctx.runtime_env().register_object_store( &url::Url::parse("s3://cache-measurement").unwrap(), store.clone(), @@ -1425,7 +1782,10 @@ mod measurement { std::process::Command::new("python3") .args([ "-u", - "src/query/cache_http_proxy.py", + concat!( + env!("CARGO_MANIFEST_DIR"), + "/tests/fixtures/cache_http_proxy.py" + ), &endpoint, mode.to_str().unwrap(), ]) @@ -1492,6 +1852,23 @@ mod measurement { backend.put(&path, new.clone().into()).await.unwrap(); std::fs::write(&mode, fault).unwrap(); std::fs::write(mode.with_extension("requests"), "").unwrap(); + if fault == "full" { + let error = store + .get_opts( + &path, + GetOptions { + range: Some(GetRange::Bounded(0..MAX_FILE + 1)), + ..Default::default() + }, + ) + .await + .unwrap_err(); + assert!(matches!(error, object_store::Error::Generic { .. })); + assert!( + non_partial(&error), + "object_store non-partial error contract changed: {error:?}" + ); + } let window = StorageMetricWindow::start("http-mutations"); for _ in 0..2 { assert_eq!( @@ -1509,6 +1886,24 @@ mod measurement { } std::fs::write(&mode, "normal").unwrap(); } + std::fs::write(&mode, "headfail").unwrap(); + backend + .put(&path, Bytes::from(vec![0; MAX_FILE as usize + 1]).into()) + .await + .unwrap(); + let original = query(store.clone(), MAX_FILE + 1, false) + .await + .unwrap_err() + .to_string(); + let recovered = query(store.clone(), MAX_FILE + 1, true) + .await + .unwrap_err() + .to_string(); + assert_eq!( + recovered, original, + "HEAD failure must preserve the metadata error" + ); + std::fs::write(&mode, "normal").unwrap(); // Unique integers defeat dictionary compression, producing a real large parquet file. let values = (0..100_000).collect::>(); let large = parquet(values.clone()); diff --git a/src/query/stream_schema_provider.rs b/src/query/stream_schema_provider.rs index 44a32f1ee..125b5cf41 100644 --- a/src/query/stream_schema_provider.rs +++ b/src/query/stream_schema_provider.rs @@ -363,15 +363,51 @@ async fn parquet_scan_plan( state: &dyn Session, batch_size: usize, ) -> Result, DataFusionError> { - let (file_format, file_source) = build_parquet_scan_components(schema, filters, state)?; - let cache_store = if object_store_url.as_str() != "file:///" - && PARSEABLE.options.small_file_cache_enabled - && PARSEABLE.options.small_file_cache_size > 0 - { - Some(state.runtime_env().object_store(&object_store_url)?) + let cache_options = if object_store_url.as_str() == "file:///" { + (0, 0, false) } else { - None + ( + Arc::as_ptr(&PARSEABLE.storage) as *const () as usize, + PARSEABLE.options.small_file_cache_size, + PARSEABLE.options.small_file_cache_enabled, + ) }; + parquet_scan_plan_with_cache_options( + schema, + object_store_url, + partitions, + statistics, + projection, + filters, + limit, + state, + batch_size, + cache_options, + ) + .await +} + +#[allow(clippy::too_many_arguments)] +pub(super) async fn parquet_scan_plan_with_cache_options( + schema: SchemaRef, + object_store_url: ObjectStoreUrl, + partitions: Vec>, + statistics: Statistics, + projection: Option<&Vec>, + filters: &[Expr], + limit: Option, + state: &dyn Session, + batch_size: usize, + cache_options: (usize, usize, bool), +) -> Result, DataFusionError> { + let (file_format, file_source) = build_parquet_scan_components(schema, filters, state)?; + let (cache_scope, cache_budget, cache_enabled) = cache_options; + let cache_store = + if object_store_url.as_str() != "file:///" && cache_enabled && cache_budget > 0 { + Some(state.runtime_env().object_store(&object_store_url)?) + } else { + None + }; // create file groups from vec file partitions let file_groups = partitions.into_iter().map(FileGroup::new).collect_vec(); @@ -406,9 +442,9 @@ async fn parquet_scan_plan( super::small_file_cache::install( plan, store, - Arc::as_ptr(&PARSEABLE.storage) as *const () as usize, - PARSEABLE.options.small_file_cache_size, - PARSEABLE.options.small_file_cache_enabled, + cache_scope, + cache_budget, + cache_enabled, state .config() .get_extension::() diff --git a/src/query/cache_http_proxy.py b/tests/fixtures/cache_http_proxy.py similarity index 94% rename from src/query/cache_http_proxy.py rename to tests/fixtures/cache_http_proxy.py index 5f80801f0..5614aba58 100644 --- a/src/query/cache_http_proxy.py +++ b/tests/fixtures/cache_http_proxy.py @@ -21,7 +21,7 @@ def do_GET(self): mode = mode_file.read_text() with mode_file.with_suffix('.requests').open('a') as log: log.write(str(bool(self.headers.get('If-None-Match'))) + '\n') - if mode == 'fail' and self.headers.get('If-None-Match'): + if (mode == 'headfail' and self.command == 'HEAD') or (mode == 'fail' and self.headers.get('If-None-Match')): self.send_response(503) self.send_header('Content-Length', '0') self.end_headers() From 7544a706d3a1ec2c8110c1dca65e56163dbb950b Mon Sep 17 00:00:00 2001 From: VibeSWE <338779468+VibeSWE@users.noreply.github.com> Date: Wed, 11 Nov 2026 13:34:21 -0600 Subject: [PATCH 5/5] ci: build Quest from a pinned source commit The floating quay.io/parseablehq/quest:main image now expects 202 from stream deletion. This server still deletes synchronously and returns 200, so TestSmokeRbacBasic and TestSmokeRoles fail on main and on every pull request. The previous image digest is no longer available from the registry, so build Quest from the last source commit before that change. The compose files take the image and pull policy from QUEST_IMAGE and QUEST_PULL_POLICY and keep their previous defaults. Refs #151 Co-Authored-By: Claude Fable 5.2 --- .github/workflows/integration-test.yaml | 15 +++++++++++++++ docker-compose-distributed-test-with-kafka.yaml | 4 ++-- docker-compose-distributed-test.yaml | 4 ++-- docker-compose-gcs-distributed-test.yaml | 4 ++-- docker-compose-test-with-kafka.yaml | 4 ++-- docker-compose-test.yaml | 4 ++-- 6 files changed, 25 insertions(+), 10 deletions(-) diff --git a/.github/workflows/integration-test.yaml b/.github/workflows/integration-test.yaml index 123496873..9c4997522 100644 --- a/.github/workflows/integration-test.yaml +++ b/.github/workflows/integration-test.yaml @@ -67,6 +67,13 @@ jobs: PARSEABLE_TEST_IMAGE: parseable-ci:${{ github.sha }} COMPOSE_FILE: ${{ matrix.compose }} QUEST_MODE: ${{ inputs.full && 'load' || 'smoke' }} + # Quest is built from a fixed source commit instead of pulling the + # floating quay.io/parseablehq/quest:main image. Later Quest commits + # expect 202 from stream deletion, which this server does not return yet. + # Remove the pin when issue #151 is done. + QUEST_SOURCE_REF: f4a026eecde1badde8776ff366a743f9bbca2e67 + QUEST_IMAGE: quest-ci:pinned + QUEST_PULL_POLICY: never steps: - uses: actions/checkout@v4 - uses: actions/download-artifact@v4 @@ -84,6 +91,14 @@ jobs: run: | docker compose up --no-build --wait --wait-timeout 180 "$SERVER" docker compose exec -T "$SERVER" sh -ec 'curl --fail --silent --show-error --location http://localhost:8000/next/ | grep -q "Parseable · Observability"' + - name: Build Quest from the pinned source commit + run: | + src="$RUNNER_TEMP/quest-src" + git init --quiet "$src" + git -C "$src" fetch --quiet --depth 1 https://github.com/parseablehq/quest.git "$QUEST_SOURCE_REF" + git -C "$src" checkout --quiet FETCH_HEAD + test "$(git -C "$src" rev-parse HEAD)" = "$QUEST_SOURCE_REF" + docker build --tag "$QUEST_IMAGE" "$src" - name: Run Quest run: docker compose up --no-build --exit-code-from quest - name: Capture service logs diff --git a/docker-compose-distributed-test-with-kafka.yaml b/docker-compose-distributed-test-with-kafka.yaml index f173e22b0..1e22cf39c 100644 --- a/docker-compose-distributed-test-with-kafka.yaml +++ b/docker-compose-distributed-test-with-kafka.yaml @@ -194,8 +194,8 @@ services: quest: platform: linux/amd64 - image: quay.io/parseablehq/quest:main - pull_policy: always + image: ${QUEST_IMAGE:-quay.io/parseablehq/quest:main} + pull_policy: ${QUEST_PULL_POLICY:-always} command: [ "load", diff --git a/docker-compose-distributed-test.yaml b/docker-compose-distributed-test.yaml index b46810649..c3bcdfc4a 100644 --- a/docker-compose-distributed-test.yaml +++ b/docker-compose-distributed-test.yaml @@ -109,8 +109,8 @@ services: quest: platform: linux/amd64 - image: quay.io/parseablehq/quest:main - pull_policy: always + image: ${QUEST_IMAGE:-quay.io/parseablehq/quest:main} + pull_policy: ${QUEST_PULL_POLICY:-always} environment: - QUEST_EDITION=oss command: diff --git a/docker-compose-gcs-distributed-test.yaml b/docker-compose-gcs-distributed-test.yaml index a90e53e2f..66bf18f69 100644 --- a/docker-compose-gcs-distributed-test.yaml +++ b/docker-compose-gcs-distributed-test.yaml @@ -79,8 +79,8 @@ services: quest: platform: linux/amd64 - image: quay.io/parseablehq/quest:main - pull_policy: always + image: ${QUEST_IMAGE:-quay.io/parseablehq/quest:main} + pull_policy: ${QUEST_PULL_POLICY:-always} command: [ "load", diff --git a/docker-compose-test-with-kafka.yaml b/docker-compose-test-with-kafka.yaml index ff8f09f2e..79d337e59 100644 --- a/docker-compose-test-with-kafka.yaml +++ b/docker-compose-test-with-kafka.yaml @@ -68,9 +68,9 @@ services: max_attempts: 3 quest: - image: quay.io/parseablehq/quest:main + image: ${QUEST_IMAGE:-quay.io/parseablehq/quest:main} platform: linux/amd64 - pull_policy: always + pull_policy: ${QUEST_PULL_POLICY:-always} command: [ "load", "http://parseable:8000", diff --git a/docker-compose-test.yaml b/docker-compose-test.yaml index ddef891fb..5fc46d7aa 100644 --- a/docker-compose-test.yaml +++ b/docker-compose-test.yaml @@ -64,9 +64,9 @@ services: max_attempts: 3 quest: - image: quay.io/parseablehq/quest:main + image: ${QUEST_IMAGE:-quay.io/parseablehq/quest:main} platform: linux/amd64 - pull_policy: always + pull_policy: ${QUEST_PULL_POLICY:-always} environment: - QUEST_EDITION=oss command: [