-
Notifications
You must be signed in to change notification settings - Fork 874
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.
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
Minor: Allow Field::new
and Field::new_with_dict
to take existing String
as well as &str
#3288
Conversation
Field::new
and Field::new_with_dict
to take existing String
as well as &strField::new
and Field::new_with_dict
to take existing String
as well as &str
fn test_new_with_string() { | ||
// Fields should allow owned Strings to support reuse | ||
let s = String::from("c1"); | ||
Field::new(s, DataType::Int64, false); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I understand the purpose clearly. Doesn't it work for now:
let s = String::from("c1");
Field::new(&s, DataType::Int64, false);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Internally that will create a new memory allocation and copy the string data across, effectively this saves a Clone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, I think this represents an api-change?
Added the label. Feel free to remove it if you think it is not necessary. |
I agree it is technically an API change, I think it will be backwards compatible in the sense that any code that created The backwards compatible story is consistent with the fact that no tests needed to be changed |
Technically this is still a breaking change, as it could now fail to infer the type of an expression where previously it could. However, in the past we haven't considered this sort of change "breaking". |
Well let's leave the label on it then |
Which issue does this PR close?
N/A
Rationale for this change
Sometimes (e.g. apache/datafusion#4534) a caller already has an owned
String
but today Field requires&str
which forces another cloneWhat changes are included in this PR?
Change
Field::new()
to takeimpl Into<String>
so that anything that implementsimpl Into<String>
, such asString
, can be passedAre there any user-facing changes?
The API is changed but is backwards compatible