Skip to content

Minor: Change LiteralGuarantee try_new to new #12669

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alamb merged 2 commits into from
Sep 30, 2024
Merged

Minor: Change LiteralGuarantee try_new to new #12669

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d5e5113
Change try_new to new, it is not fallible
pgwhalen Sep 28, 2024
700a153
Fix formatting
pgwhalen Sep 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 10 additions & 13 deletions datafusion/physical-expr/src/utils/guarantee.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,18 +93,18 @@ impl LiteralGuarantee {
/// Create a new instance of the guarantee if the provided operator is
/// 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

@alamb alamb Sep 29, 2024

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

datafusion/datafusion/execution/src/runtime_env.rs

Lines 69 to 72 in 810e908

#[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

@pgwhalen pgwhalen Sep 29, 2024

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

@alamb alamb Sep 30, 2024

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.

column_name: impl Into<String>,
guarantee: Guarantee,
literals: impl IntoIterator<Item = &'a ScalarValue>,
) -> Option<Self> {
) -> Self {
let literals: HashSet<_> = literals.into_iter().cloned().collect();

Some(Self {
Self {
column: Column::from_name(column_name),
guarantee,
literals,
})
}
}

/// Return a list of [`LiteralGuarantee`]s that must be satisfied for `expr`
Expand Down Expand Up @@ -338,13 +338,10 @@ impl<'a> GuaranteeBuilder<'a> {
// This is a new guarantee
let new_values: HashSet<_> = new_values.into_iter().collect();

if let Some(guarantee) =
LiteralGuarantee::try_new(col.name(), guarantee, new_values)
{
// add it to the list of guarantees
self.guarantees.push(Some(guarantee));
self.map.insert(key, self.guarantees.len() - 1);
}
let guarantee = LiteralGuarantee::new(col.name(), guarantee, new_values);
// add it to the list of guarantees
self.guarantees.push(Some(guarantee));
self.map.insert(key, self.guarantees.len() - 1);
}

self
Expand Down Expand Up @@ -851,7 +848,7 @@ mod test {
S: Into<ScalarValue> + 'a,
{
let literals: Vec<_> = literals.into_iter().map(|s| s.into()).collect();
LiteralGuarantee::try_new(column, Guarantee::In, literals.iter()).unwrap()
LiteralGuarantee::new(column, Guarantee::In, literals.iter())
}

/// Guarantee that the expression is true if the column is NOT any of the specified values
Expand All @@ -861,7 +858,7 @@ mod test {
S: Into<ScalarValue> + 'a,
{
let literals: Vec<_> = literals.into_iter().map(|s| s.into()).collect();
LiteralGuarantee::try_new(column, Guarantee::NotIn, literals.iter()).unwrap()
LiteralGuarantee::new(column, Guarantee::NotIn, literals.iter())
}

// Schema for testing
Expand Down