Open
Conversation
github-actions
bot
removed
the
action/benchmark-sql
Contributor
Benchmarks: PolarSignals ProfilingSummary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=1 on NVMESummary
Detailed Results Table
|
Contributor
Benchmarks: FineWeb NVMeSummary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=1 on S3Summary
Detailed Results Table
|
Contributor
Benchmarks: TPC-DS SF=1 on NVMESummary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=10 on NVMESummary
Detailed Results Table
|
Contributor
Benchmarks: FineWeb S3Summary
Detailed Results Table
|
Contributor
Benchmarks: Statistical and Population GeneticsSummary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=10 on S3Summary
Detailed Results Table
|
Contributor
Benchmarks: Clickbench on NVMESummary
Detailed Results Table
|
github-actions
bot
removed
the
action/benchmark-sql
github-actions
bot
removed
the
action/benchmark-sql
joseph-isaacs
added
the
action/benchmark
github-actions
bot
removed
the
action/benchmark
Contributor
Polar Signals Profiling ResultsLatest Run
Previous Runs (7)
Powered by Polar Signals Cloud |
Contributor
Benchmarks: Random AccessSummary
Detailed Results Table
|
Contributor
|
Shall we make a second PR with the CI flag switched |
Contributor
Author
|
Yeah I have lots of follow ups before we even thinking about cutting over. Some queries are 15x slower due to lack of stats for example |
gatesn
commented
Feb 19, 2026
Comment on lines
+288
to
+292
| // Vortex handles parallelism internally — always use a single partition. | ||
| let mut this = self.clone(); | ||
| this.num_partitions = NonZero::new(target_partitions) | ||
| .ok_or_else(|| DataFusionError::Internal("non-zero partitions".to_string()))?; | ||
| Ok(Some(Arc::new(this))) |
Contributor
Author
There was a problem hiding this comment.
I want to store the updated num_partitions and use that as a concurrency hint for ourselves
AdamGS
reviewed
Feb 19, 2026
AdamGS
reviewed
Feb 19, 2026
| } | ||
|
|
||
| /// Convert a Vortex [`Option<Precision>`] to a DataFusion [`Precision`](DFPrecision). | ||
| fn estimate_to_df_precision(est: &Option<Precision<u64>>) -> DFPrecision<usize> { |
Contributor
There was a problem hiding this comment.
we have the PrecisionExt trait for this
AdamGS
reviewed
Feb 19, 2026
Comment on lines
+82
to
+86
| _state: &dyn Session, | ||
| // Unlike filters and limit, we _do_ apply the projection at this stage since DataFusion's | ||
| // physical projection expression push-down is still in its early stages. In theory, we | ||
| // could also wait to apply the projection until we can push down over the physical plan. | ||
| projection: Option<&Vec<usize>>, |
Contributor
There was a problem hiding this comment.
you can, not sure what you mean here
AdamGS
reviewed
Feb 19, 2026
| if let Some(size) = file.size { | ||
| options = options.with_file_size(size); | ||
| } | ||
| if let Some(footer) = session.multi_file().get_footer(&cache_key) { |
Contributor
There was a problem hiding this comment.
storing this on the session will cause the same issue we had with metrics, where memory usage can grow indefinitely I see its capped
AdamGS
reviewed
Feb 19, 2026
| impl Default for MultiFileSession { | ||
| fn default() -> Self { | ||
| Self { | ||
| footer_cache: moka::sync::Cache::builder().max_capacity(10_000).build(), |
Contributor
There was a problem hiding this comment.
10k footers is ~640MB, why not use the approximated size?
AdamGS
reviewed
Feb 19, 2026
Comment on lines
+16
to
+24
| //! # Future Work | ||
| //! | ||
| //! - **Schema union**: Allow missing columns (filled with nulls) and compatible type upcasts | ||
| //! across sources instead of requiring exact dtype matches. | ||
| //! - **Hive-style partitioning**: Extract partition values from file paths (e.g. `year=2024/month=01/`) | ||
| //! and expose them as virtual columns. | ||
| //! - **Virtual columns**: `filename`, `file_row_number`, `file_index`. | ||
| //! - **Per-file statistics**: Merge column statistics across sources for planner hints. | ||
| //! - **Error resilience**: Skip failed sources instead of aborting the entire scan. |
Contributor
There was a problem hiding this comment.
nit - seems like this should be an issue and not docs?
AdamGS
reviewed
Feb 19, 2026
Comment on lines
+235
to
+241
| for (i, child) in children.iter().enumerate() { | ||
| match child { | ||
| MultiChild::Opened(ds) => ready.push_back(ds.clone()), | ||
| MultiChild::Deferred(factory) => deferred.push_back((i, factory.clone())), | ||
| } | ||
| } | ||
| drop(children); |
Contributor
There was a problem hiding this comment.
can't you std::mem::take the children and then children.into_iter()?
AdamGS
reviewed
Feb 19, 2026
Comment on lines
+283
to
+287
| use vortex::file::multi::MultiFileDataSource; | ||
| use vortex::io::object_store::ObjectStoreFileSystem; | ||
| use vortex::io::session::RuntimeSessionExt; | ||
| use vortex::scan::api::DataSource as _; | ||
| use vortex_datafusion::v2::VortexTable; |
Contributor
There was a problem hiding this comment.
move to top? also - lets log here so its visible which API is used
AdamGS
reviewed
Feb 19, 2026
| /// in an array is guaranteed to be less than or equal to the inexact value, but equal to the exact | ||
| /// value. | ||
| /// | ||
| // TODO(ngates): should we model Unknown as a variant of Precision? Or have Option<Precision<T>>? |
AdamGS
reviewed
Feb 19, 2026
| } | ||
|
|
||
| /// Approximate the in-memory size of a footer. | ||
| fn estimate_footer_size(footer: &Footer) -> usize { |
AdamGS
reviewed
Feb 19, 2026
AdamGS
reviewed
Feb 19, 2026
| &self, | ||
| target_partitions: usize, | ||
| _repartition_file_min_size: usize, | ||
| _output_ordering: Option<LexOrdering>, |
Contributor
There was a problem hiding this comment.
if output ordering is provided, we might need to do something else (potentially, keep a single overall partition?).
AdamGS
reviewed
Feb 19, 2026
Comment on lines
+107
to
+110
| let fs = self | ||
| .fs | ||
| .map(Ok) | ||
| .unwrap_or_else(|| create_local_filesystem(&self.session))?; |
Contributor
There was a problem hiding this comment.
Suggested change
| let fs = self | |
| .fs | |
| .map(Ok) | |
| .unwrap_or_else(|| create_local_filesystem(&self.session))?; | |
| let fs = match self.fs { | |
| Some(fs) => fs, | |
| None => create_local_filesystem(&self.session)?, | |
| }; |
AdamGS
reviewed
Feb 19, 2026
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| #![allow(clippy::unwrap_used)] |
Contributor
There was a problem hiding this comment.
thought its already allowed in tests?
AdamGS
reviewed
Feb 19, 2026
| /// Serialize a split from this data source. | ||
| fn serialize_split(&self, split: &dyn Split) -> VortexResult<Vec<u8>>; | ||
| /// Serialize the [`DataSource`] to pass to a remote worker. | ||
| fn serialize(&self) -> VortexResult<Option<Vec<u8>>> { |
Contributor
There was a problem hiding this comment.
do we need to include serialization in this PR? Is it used anywhere?
AdamGS
reviewed
Feb 19, 2026
Comment on lines
+75
to
+81
| fn row_count_estimate(&self) -> Option<Precision<u64>> { | ||
| None | ||
| } | ||
|
|
||
| /// Returns a scan over the source. | ||
| async fn scan(&self, scan_request: ScanRequest) -> VortexResult<DataSourceScanRef>; | ||
| /// Returns an estimate of the byte size of the un-filtered source. | ||
| fn byte_size_estimate(&self) -> Option<Precision<u64>> { | ||
| None |
Contributor
There was a problem hiding this comment.
nit - estimate in the name is redundant as the type itself communicates that
AdamGS
reviewed
Feb 19, 2026
| /// Sets the number of deferred sources to open concurrently during scanning. | ||
| /// | ||
| /// Higher values overlap more file-opening I/O with split execution but use more memory | ||
| /// for in-flight metadata. Defaults to 8. |
Contributor
There was a problem hiding this comment.
nit - make the const public and point to it here, so docs don't go stale.
AdamGS
reviewed
Feb 19, 2026
| /// opened source without re-opening. | ||
| pub struct MultiDataSource { | ||
| dtype: DType, | ||
| children: Arc<Mutex<Vec<MultiChild>>>, |
Contributor
There was a problem hiding this comment.
I don't think the Mutex is need, if you remove it here and make MultiChild Clone everything works, cloning the vec a couple of times seems really minor and way less dangerous that running into an exclusive lock
AdamGS
reviewed
Feb 19, 2026
| found | ||
| } | ||
|
|
||
| fn is_decimal(dt: &DataType) -> bool { |
Contributor
There was a problem hiding this comment.
nit - this is not needed anymore 🥳 there should be DataType::is_decimal
Contributor
|
I don't quite know how to review this. My preference is starting with something fast that then could have adjusted api instead of starting with a desired api that then cannot be made fast |
AdamGS
reviewed
Feb 19, 2026
| // expression (i.e. we require that coercion semantics have already been performed by the | ||
| // engine). Instead, DataFusion will push down filters through the physical plan via | ||
| // the VortexScanSource DataSource. | ||
| _filters: &[Expr], |
Contributor
There was a problem hiding this comment.
we can extract column references from the filter here, which will tell us which statistics are worth fetching
AdamGS
reviewed
Feb 19, 2026
Comment on lines
+92
to
+93
| // Similarly for limit, we wait until we can push down over the physical plan. | ||
| _limit: Option<usize>, |
Contributor
There was a problem hiding this comment.
I think we can use limit if there are no expressions
AdamGS
reviewed
Feb 19, 2026
Comment on lines
+58
to
+61
| pub trait DataSourceFactory: 'static + Send + Sync { | ||
| /// Opens the data source, or returns `None` if it should be skipped. | ||
| async fn open(&self) -> VortexResult<Option<DataSourceRef>>; | ||
| } |
Contributor
There was a problem hiding this comment.
I might be missing some detail here, but DataSourceFactory and DataSourceOpener seems really similar, not sure why we want both
AdamGS
reviewed
Feb 19, 2026
| } | ||
|
|
||
| fn splits(self: Box<Self>) -> SplitStream { | ||
| stream::unfold( |
Contributor
There was a problem hiding this comment.
unfold is so useful but I really hate it 😢
Contributor
|
Is there maybe a way to set these up as a additional benchmark variant for one of the benchmarks so we can see the constant comparison? |
robert3005
reviewed
Feb 19, 2026
Contributor
|
+1 for @robert3005 idea to benchmark both variants |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Experiment to use the Scan API from DuckDB and DataFusion integrations.
This PR introduces:
vortex::file::MultiFileDataSource- a data source builder that takes a listing of files, (in the future) unifies schemas, adds support for hive-style partitioning columns, etc, etc.vortex::scan::api- improvements to the Scan API as have become apparent while implementing support in two engines. I'd like to implement more engines before we decide on a path to stability.