refactor(scan) refactor PlanContext to allow for different manifest sources - #3108
Open
xanderbailey wants to merge 2 commits into
Open
refactor(scan) refactor PlanContext to allow for different manifest sources#3108xanderbailey wants to merge 2 commits into
xanderbailey wants to merge 2 commits into
Conversation
xanderbailey
commented
Aug 28, 2026
| manifest_source: Arc::new(SnapshotSource::new(snapshot.clone())), | ||
| table_metadata: self.table.metadata_ref(), | ||
| snapshot_schema: schema, | ||
| plan_schema: schema, |
Contributor
Author
There was a problem hiding this comment.
Renamed this to a more generic "plan_schema"
xanderbailey
commented
Aug 28, 2026
|
|
||
| /// Returns the partition filter for a manifest. See [`PartitionFilterCache::get`] for the | ||
| /// always-true fallback when the manifest's spec cannot be resolved against the scan schema. | ||
| fn get_partition_filter(&self, manifest_file: &ManifestFile) -> Result<Arc<BoundPredicate>> { |
Contributor
Author
There was a problem hiding this comment.
moved private method down to make pub (crate) more obvious
xanderbailey
commented
Aug 28, 2026
| plan_context: Option<PlanContext>, | ||
| /// The snapshot being scanned, if this table has at least one snapshot. | ||
| /// When absent the scan yields no rows. | ||
| snapshot: Option<SnapshotRef>, |
Contributor
Author
There was a problem hiding this comment.
needed because TableScan has a pub api to ask for it's snapshot and I didn't want to break that
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
1 participant
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.
Which issue does this PR close?
What changes are included in this PR?
Previously
PlanContextonly held a snapshot reference which makes it very hard to be extended for incremental append scans since they require multiple manifest lists so we introduceManifestSourceas a way to represent a source ofManifestFile.This has the added benefit now having a non-optional
PlanContexton theTableScanas a result ofEmptySource. The reason this wasn't possible before was because metadata without any snapshots couldn't construct aPlanContext- this isn't true anymore. The result of this is that previouslySELECT "not_a_column" FROM tablewould actually skip schema validation before if there wasn't a snapshot in the metadata.This refactor has the benefit of creating a single pub (crate) method on the
PlanContext. Before it was the case that thePlanContexthad a load manifest_list method that it then passed into the otherbuild_manifest_file_contextsmethod which seemed like a leaky abstraction to me.We're also refactoring the actual planning in
TableScanscan out into a reusableplan.rssuch that we can directly use it in an append scan.There are alternatives here for the reviewer to consider:
build_manifest_file_contextscould take Vec but you'd still need to generalize thePlanContextto remove theget_manifest_listsince this only makes sense for a single snapshot scan.PlanContextcould store a list of manifest files but that involves resolving them when we build the table scan rather than we ask for the plan, this would makeTableScanBuilder::buildasync and eagerly resolve the manifest files which I don't think is desirable.Are these changes tested?
AI Disclosure