Skip to content

Conversation

@r4ntix
Copy link
Contributor

@r4ntix r4ntix commented Oct 20, 2022

Which issue does this PR close?

Closes #3901 and apache/datafusion-ballista#375

Rationale for this change

See #3901

What changes are included in this PR?

  • Add substring expr to support Substring(str [from int] [for int])
  • Modify from_proto.rs to support Substring(str [from int] [for int])
  • Add roundtrip_substr test

Are there any user-facing changes?

None

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.

Thanks @r4ntix

parse_expr(&args[1], registry)?,
)),
ScalarFunction::Substr => {
if args.len() > 2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend adding an assert assert_eq!(args.len(), 3) in this case so that we don't get another silent failure if we add additional argument support to substr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback, I've added the assert in the new commit :)

@alamb alamb merged commit e534c25 into apache:master Oct 21, 2022
@alamb
Copy link
Contributor

alamb commented Oct 21, 2022

Thanks @r4ntix

@r4ntix r4ntix deleted the issues-3901 branch March 25, 2023 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

logical-expr Logical plan and expressions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

datafusion-proto deserialize with Substring(str [from int] [for int]) fails

3 participants