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.

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

Conversation

QuenKar
Copy link
Contributor

@QuenKar QuenKar commented Dec 4, 2023

Which issue does this PR close?

Closes #8380 .

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@QuenKar QuenKar changed the title feat: ScalarValue from string feat: ScalarValue from String Dec 4, 2023
Copy link
Member

@Weijun-H Weijun-H left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @QuenKar

Copy link
Member

@jackwener jackwener left a comment

Choose a reason for hiding this comment

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

Is there anywhere else the code could use this change? if so I think it could be changed together.

Comment on lines +4658 to +4659
let scalar = ScalarValue::from("foo".to_string());
assert_eq!(scalar, ScalarValue::Utf8(Some("foo".to_string())));
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!

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 @QuenKar and @Dandandan and @viirya

Comment on lines +4658 to +4659
let scalar = ScalarValue::from("foo".to_string());
assert_eq!(scalar, ScalarValue::Utf8(Some("foo".to_string())));
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

@alamb alamb merged commit 2e5ad7a into apache:main Dec 5, 2023
22 checks passed
appletreeisyellow pushed a commit to appletreeisyellow/datafusion that referenced this pull request Dec 15, 2023
* feat: scalar from string

* chore: cr comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make it easier to create ScalarValue from String and &str
6 participants