Skip to content

Conversation

@pgwhalen
Copy link
Contributor

Which issue does this PR close?

Minor change so no associated issue - hopefully that's okay, I see comparable "minor" PRs get merged.

Rationale for this change

With https://github.com/apache/datafusion/pull/8654/files, this (non-public) function is no longer fallible, so could read a little more clearly if the name and return type reflects that.

What changes are included in this PR?

Rename LiteralGuarantee::try_new to new and have it return Self rather than Option<Self>.

Are these changes tested?

Yes, existing tests pass.

Are there any user-facing changes?

None.

@github-actions github-actions bot added the physical-expr Changes to the physical-expr crates label Sep 28, 2024
@pgwhalen
Copy link
Contributor Author

Looks like @alamb might be best to review - thanks!

/// supported. Returns None otherwise. See [`LiteralGuarantee::analyze`] to
/// create these structures from an predicate (boolean expression).
fn try_new<'a>(
fn new<'a>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @pgwhalen -- this is a nice change and cleanup and makes sense to me

In order to make this change easier on downstream crates, can you please add back a version of try_new() that is marked "deprecated" with instructions on how to change the code that just calls new()

Here is an example

#[deprecated(note = "please use `try_new` instead")]
pub fn new(config: RuntimeConfig) -> Result<Self> {
Self::try_new(config)
}

Having a deprecation marking means that users will get a message from the compiler telling them what to do rather than having to track it down themselves

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, I will add tonight! I'd assumed it wasn't an issue because the function is not pub (it seems the main public interface to create LiteralGuarantees is analyze) - but I'm a little new to rust so I have some gaps in knowledge of how visibility works.

Copy link
Contributor

Choose a reason for hiding this comment

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

but I'm a little new to rust so I have some gaps in knowledge of how visibility works.

When in doubt I go to the docs.rs page:
https://docs.rs/datafusion/latest/datafusion/physical_expr/utils/struct.LiteralGuarantee.html

An in this case, I think you are right that this is a non pub method and therefore there is no downstream API implications.

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @pgwhalen

@alamb alamb merged commit 29b8af2 into apache:main Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-expr Changes to the physical-expr crates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants