Skip to content
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

feat: ScalarValue from String #8411

Merged
merged 2 commits into from
Dec 5, 2023
Merged

feat: ScalarValue from String #8411

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8458a3b
feat: scalar from string
QuenKar Dec 2, 2023
2b148d4
chore: cr comment
QuenKar Dec 4, 2023
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
16 changes: 16 additions & 0 deletions datafusion/common/src/scalar.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3022,6 +3022,12 @@ impl FromStr for ScalarValue {
}
}

impl From<String> for ScalarValue {
fn from(value: String) -> Self {
ScalarValue::Utf8(Some(value))
}
}

impl From<Vec<(&str, ScalarValue)>> for ScalarValue {
fn from(value: Vec<(&str, ScalarValue)>) -> Self {
let (fields, scalars): (SchemaBuilder, Vec<_>) = value
Expand Down Expand Up @@ -4645,6 +4651,16 @@ mod tests {
);
}

#[test]
fn test_scalar_value_from_string() {
let scalar = ScalarValue::from("foo");
assert_eq!(scalar, ScalarValue::Utf8(Some("foo".to_string())));
let scalar = ScalarValue::from("foo".to_string());
assert_eq!(scalar, ScalarValue::Utf8(Some("foo".to_string())));
Comment on lines +4658 to +4659
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, as we already implement From<&str> for ScalarValue, we can simply do this for a String if we want to have a ScalarValue for it:

let scalar = ScalarValue::from("foo".to_string().as_str());
assert_eq!(scalar, ScalarValue::Utf8(Some("foo".to_string())));

I think the only difference is From<String> avoids copying the string content?

Do we have used as_str() for String in current code to create ScalarValue? We probably can change to remove as_str().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I find that there is no code similar to "foo".to_string().as_str() or some_string.as_str() or &some_string or as_str().into() in this use(But maybe I don't find all of them...

Copy link
Contributor

Choose a reason for hiding this comment

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

One tricky might be removing From<&str> implementqtion and seeing if something fails to compile.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the only difference is From avoids copying the string content?

It avoids copying as well as makes creating ScalarValues easier to create / user (e.g. someone was confused as reported on #8380)

One tricky might be removing From<&str> implementqtion and seeing if something fails to compile.

I tried this and found there weren't any obvious examples in the codebase -- the more common pattern is ScalarValue::from("foo") with a constant string

Copy link
Member

Choose a reason for hiding this comment

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

I tried this and found there weren't any obvious examples in the codebase -- the more common pattern is ScalarValue::from("foo") with a constant string

Nice!

let scalar = ScalarValue::from_str("foo").unwrap();
assert_eq!(scalar, ScalarValue::Utf8(Some("foo".to_string())));
}

#[test]
fn test_scalar_struct() {
let field_a = Arc::new(Field::new("A", DataType::Int32, false));
Expand Down