Skip to content

Add hooks to SchemaAdapter to add custom column generators - #15261

Closed
adriangb wants to merge 3 commits into
apache:mainfrom
pydantic:defaults-schema-adapter
Closed

Add hooks to SchemaAdapter to add custom column generators#15261
adriangb wants to merge 3 commits into
apache:mainfrom
pydantic:defaults-schema-adapter

Conversation

@adriangb

@adriangbadriangb commented Mar 16, 2025•
edited
Loading

Copy link
Copy Markdown
Contributor

Closes#15220

A lot of the work of this PR is meant to resolve#15220 (comment). I think I'll move that into a standalone PR.

@github-actionsgithub-actionsBot added core Core DataFusion crate datasource Changes to the datasource crate labels Mar 16, 2025
@adriangb
adriangb marked this pull request as draft March 16, 2025 20:48
adriangb
adriangb commented Mar 16, 2025
View reviewed changes
Comment threaddatafusion/core/src/datasource/mod.rs Outdated
Comment on lines +279 to +280
let err = adapter.map_schema(&file_schema).unwrap_err().to_string();
assert_eq!(err, "Error during planning: Column a is missing from the file schema, cannot be generated, and is non-nullable");

@adriangbadriangbMar 16, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's nice that this fails earlier 😄

Comment threaddatafusion/core/src/datasource/mod.rs Outdated
Comment threaddatafusion/datasource-parquet/src/row_filter.rs
Comment threaddatafusion/datasource-parquet/src/row_filter.rs Outdated
Comment threaddatafusion/datasource-parquet/src/row_filter.rs Outdated
@adriangb

adriangb commented Mar 17, 2025

Copy link
Copy Markdown
ContributorAuthor

I've moved the complex bit over to #15263. I'll let that settle first then resume work here.

@adriangb

adriangb commented Mar 17, 2025

Copy link
Copy Markdown
ContributorAuthor

Noting that in #15263 (comment) I realized that it might be good to have a system to report stats for columns that will be generated before they are generated (is it all nulls? is it a constant?) to be used with stats pruning.

@adriangb

adriangb commented Mar 19, 2025

Copy link
Copy Markdown
ContributorAuthor

Now that #15263 is merged I'll come back here and:

  1. Resolve conflicts.
  2. Add an API for the SchemaAdapter to declare the stats for potentially generated columns if they are known ahead of time.

@adriangb
adriangbforce-pushed the defaults-schema-adapter branch from 896eb8b to 74b884fCompareMarch 20, 2025 17:15
@adriangb
adriangbforce-pushed the defaults-schema-adapter branch from de02a9a to 0bdebc8CompareMarch 20, 2025 17:20
@adriangb
adriangb marked this pull request as ready for review March 20, 2025 17:20
@adriangb

adriangb commented Mar 20, 2025

Copy link
Copy Markdown
ContributorAuthor

Marking as ready for review. The main TODO is an API for transmitting statistics information for generated columns before they get generated, but that can even be a followup PR.

@adriangb

adriangb commented Apr 30, 2025•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Looking at how filter pushdown interacts with partition columns I think this could improve that.
Currently the partition values get bound when the FileStream is created which is after the predicate pushdown is applied. The filtering for filters that depend both on the partition values and data happens via a FilterExec.
This means that partition values are not available in predicate pushdown, and instead happens upstream in a FilterExec.

I feel like this change could help with that... but some details are missing: we somehow need to pipe the partition values into the FileSource so that it can in turn pass in the info to generate the partition columns on the fly if needed. Or something like that...

@adriangbadriangb mentioned this pull request May 7, 2025
Merged
leoyvens
leoyvens approved these changes Jun 16, 2025
View reviewed changes

@leoyvensleoyvens left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a maintainer, but I also have a use case for this and the patch LGTM.

Thank you @adriangb for contributing this!

alamb
alamb reviewed Jun 16, 2025
View reviewed changes
Comment threaddatafusion/datasource/src/schema_adapter.rs
fn map_batch(&self, batch: RecordBatch) -> datafusion_common::Result<RecordBatch>;
}

pub trait MissingColumnGeneratorFactory: Debug + Send + Sync {

@alambalambJun 16, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't had a chance to review this entire PR yet, but I am feeling a little strange that these traits are not used by SchemaMapper but instead are used by DefaultSchemaMapper

I wonder what is the usecase for these extra two traits, compared to say adding a method on SchemaMapper

This was referenced Jun 22, 2025
Merged
Open
@adriangb
adriangb marked this pull request as draft June 27, 2025 18:48
@adriangb

adriangb commented Jun 27, 2025

Copy link
Copy Markdown
ContributorAuthor

Marking as draft until I have time to work on this

@adriangb

adriangb commented Jul 20, 2025

Copy link
Copy Markdown
ContributorAuthor

I'm proposing we replace SchemaAdapter in #16800 so I don't plan to work on this PR anymore

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alambalambalamb left review comments
+1 more reviewer
@leoyvensleoyvensleoyvens approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

coreCore DataFusion cratedatasourceChanges to the datasource crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support default values for columns in SchemaAdapter

3 participants

@adriangb@alamb@leoyvens