Skip to content

Add documentation for UNION schema handling.#17248

Merged
alamb merged 6 commits into
apache:mainfrom
influxdata:12736/union-schema-handling
Aug 22, 2025
Merged

Add documentation for UNION schema handling.#17248
alamb merged 6 commits into
apache:mainfrom
influxdata:12736/union-schema-handling

Conversation

@wiedld

@wiedld wiedld commented Aug 19, 2025

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Part of #12736

Rationale for this change

Demonstrate handling of union schema and metadata.

What changes are included in this PR?

Docs and very minor refactoring.

Are these changes tested?

N/A

Are there any user-facing changes?

New docs.

@github-actions github-actions Bot added logical-expr Logical plan and expressions optimizer Optimizer rules labels Aug 19, 2025
@wiedld wiedld changed the title 12736/union schema handling Add documentation for UNION schema handling. Aug 19, 2025
merged.retain(|k, v| input.get(k) == Some(&*v));
}
merged
intersect_for_union(inputs)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note that at construction, we handle the schema by intersection.

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.

since this function just calls another one, I think we could remove this one and call the other

Comment on lines +960 to +962
///
/// ## Schema and Field Handling in Union Coercion
///

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note that here in the analyzer, we handle union coercion differently than at construction.

@wiedld
wiedld marked this pull request as ready for review August 19, 2025 22:43
}
}

/// The metadata used in [`Field::metadata`].

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.

It would be good to describe the key and value, ideally with some example

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Thank you.
Added: baa3f8d

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.

As we move to working with more field metadata, maybe should move this function into its own module -- datafusion/expr/src/metadata.rs or something 🤔

As a follow on PR

@comphead comphead 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.

Thanks @wiedld

@alamb alamb 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.

Thanks @wiedld and @comphead -- made two small changes and merged up from main. this is a nice addition

Comment thread datafusion/expr/src/expr.rs Outdated
/// # Returns
///
/// A new `SchemaFieldMetadata` containing only the intersected metadata
pub fn intersect_for_union<'a>(

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.

give this operates on metadata, I think we should have metadata in the name

merged.retain(|k, v| input.get(k) == Some(&*v));
}
merged
intersect_for_union(inputs)

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.

since this function just calls another one, I think we could remove this one and call the other

}
}

/// The metadata used in [`Field::metadata`].

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.

As we move to working with more field metadata, maybe should move this function into its own module -- datafusion/expr/src/metadata.rs or something 🤔

As a follow on PR

@alamb
alamb merged commit bd249ba into apache:main Aug 22, 2025
27 checks passed
@alamb

alamb commented Aug 22, 2025

Copy link
Copy Markdown
Contributor

Thanks again @comphead and @wiedld

@wiedld

wiedld commented Aug 22, 2025

Copy link
Copy Markdown
Contributor Author

Thanks for the updates @alamb

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

Labels

logical-expr Logical plan and expressions optimizer Optimizer rules

3 participants