jayzhan211 commented on PR #6940: URL: https://github.com/apache/arrow-datafusion/pull/6940#issuecomment-1636567035
I think those are good suggestions but as you said we could improve them iteratively. I think we can merge this PR for now! 🙂 On Sat, Jul 15, 2023, 4:58 AM Andrew Lamb ***@***.***> wrote: > ***@***.**** approved this pull request. > > Thank you @jayzhan211 <https://github.com/jayzhan211> and @izveigor > <https://github.com/izveigor> > > I think there are some improvements / issues with the code in this PR. > However, I think it is a step forward for datafusion and I think we can > merge it and keep iterating on master. > > I defer to your preference. Let me know what you want to do > ------------------------------ > > In datafusion/physical-expr/src/array_expressions.rs > <https://github.com/apache/arrow-datafusion/pull/6940#discussion_r1264167397> > : > > > + let s = s.strip_suffix(delimeter).unwrap().to_string(); > + res.push(Some(s)); > > I am not quite sure if it is sure that delimiter is always present. If it > is not, this code will panic. > > Perhaps it could be something like the following to aavoid the unwrap? > ⬇️ Suggested change > > - let s = s.strip_suffix(delimeter).unwrap().to_string(); > - res.push(Some(s)); > + let s = s.strip_suffix(delimeter).unwrap_or(s).to_string(); > + res.push(Some(s)); > > ------------------------------ > > In datafusion/physical-expr/src/array_expressions.rs > <https://github.com/apache/arrow-datafusion/pull/6940#discussion_r1264170324> > : > > > @@ -1195,21 +1195,56 @@ pub fn array_to_string(args: &[ArrayRef]) -> Result<ArrayRef> { > } > > let mut arg = String::from(""); > - let mut res = compute_array_to_string( > - &mut arg, > - arr.clone(), > - delimeter.clone(), > - null_string, > - with_null_string, > - )? > - .clone(); > - match res.as_str() { > - "" => Ok(Arc::new(StringArray::from(vec![Some(res)]))), > + let mut res: Vec<Option<String>> = Vec::new(); > + > + match arr.data_type() { > + DataType::List(_) | DataType::LargeList(_) | DataType::FixedSizeList(_, _) => { > + let list_array = arr.as_list::<i32>(); > > I don't think this is going to work (will panic) for LargeList as it > should be i64 -- we would probably have to make a generic version of this > function. > > However, for now I think it looks reasonable and a step forward to me > > — > Reply to this email directly, view it on GitHub > <https://github.com/apache/arrow-datafusion/pull/6940#pullrequestreview-1531032030>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ADZCLRZDJ4OSGW3BQA6Y27TXQGXITANCNFSM6AAAAAA2IIUBZI> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> > -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
