Add support for array_to_string and array_to_string_null array functions #4171
Conversation
There was a problem hiding this comment.
Thanks for opening this PR. It looks like a first step in the right direction. It's definitively a good job for your first PR 👍
I've left a few comments that hopefully should help to address the test failures and I also added some suggestions on how to improve the code.
I also resolved the merge conflict caused by merging some other PR's, so be sure to pull the changes before continuing to work on this PR.
| /// # Ok(()) | ||
| /// # } | ||
| /// ``` | ||
| fn array_to_string_null<Arr: ArrayOrNullableArray<Inner=Elem> + SingleValue, Elem: SingleValue>( |
There was a problem hiding this comment.
| fn array_to_string_null<Arr: ArrayOrNullableArray<Inner=Elem> + SingleValue, Elem: SingleValue>( | |
| #[sql_name = "array_to_string"] | |
| fn array_to_string_null<Arr: ArrayOrNullableArray<Inner=Elem> + SingleValue, Elem: SingleValue>( |
as otherwise no sql function with that name exists
Also what's about naming this function: array_to_string_with_null_string?
| fn array_to_string_null<Arr: ArrayOrNullableArray<Inner=Elem> + SingleValue, Elem: SingleValue>( | ||
| arr: Arr, | ||
| delim: Text, | ||
| null_str: Nullable<Text> |
There was a problem hiding this comment.
| null_str: Nullable<Text> | |
| null_str: Text |
As we want to require that optional argument here
| /// # Ok(()) | ||
| /// # } | ||
| /// ``` | ||
| fn array_to_string<Arr: ArrayOrNullableArray<Inner=Elem> + SingleValue, Elem: SingleValue>( |
There was a problem hiding this comment.
This bound can be simplified to
| fn array_to_string<Arr: ArrayOrNullableArray<Inner=Elem> + SingleValue, Elem: SingleValue>( | |
| fn array_to_string<Arr: ArrayOrNullableArray>( |
We don't use Elem anywhere.
|
Will look at the fails, |
| /// Converts each array element to its text representation, and concatenates those separated by the delimiter string. | ||
| /// This variant omits the `null_str` argument. | ||
| /// This variant omits the `null_str` argument. See [array_to_string_null] for a variant with that argument |
There was a problem hiding this comment.
As you rename this function, new name should be used:
| /// This variant omits the `null_str` argument. See [array_to_string_null] for a variant with that argument | |
| /// This variant omits the `null_str` argument. See [`array_to_string_with_null_string`] for a variant with that argument |
|
Need some help I think... I'm a bit stuck and banging my head against a wall at this point, @weiznich can you please review and try to point me in the right direction? |
|
I'm away for a few days. I will have a look as soon as I'm back. |
There was a problem hiding this comment.
Thanks for updating this PR. I've left a few comments that should help to explain how to proceed with this. If something is still unclear do not hesitate to ask for more details. It's sometimes hard for me to get the level of detail for information included in such comments correct.
| fn array_to_string<Arr: ArrayOrNullableArray<Inner=Nullable<Text>> + SingleValue, T: SingleValue>( | ||
| a: Arr, | ||
| e: T | ||
| ) -> Text; |
There was a problem hiding this comment.
| fn array_to_string<Arr: ArrayOrNullableArray<Inner=Nullable<Text>> + SingleValue, T: SingleValue>( | |
| a: Arr, | |
| e: T | |
| ) -> Text; | |
| fn array_to_string<Arr: ArrayOrNullableArray +SingleValue>(array: Arr, del: Text)) -> Text; |
We do not need to make this function generic over the type of del here, as we only want to accept anything that is compatible with Text there. Therefore we can just write the sql side type (Text) there and do not need to care about generics.
Also we want to accept any array type there, not only arrays of the type Text, which means we do not care about the Inner associated type here.
| fn array_to_string_with_null_string<Arr: ArrayOrNullableArray<Inner=Nullable<Text>> + SingleValue, T: SingleValue, N: SingleValue>( | ||
| a: Arr, | ||
| e: T, | ||
| n: N | ||
| ) -> Text; |
There was a problem hiding this comment.
| fn array_to_string_with_null_string<Arr: ArrayOrNullableArray<Inner=Nullable<Text>> + SingleValue, T: SingleValue, N: SingleValue>( | |
| a: Arr, | |
| e: T, | |
| n: N | |
| ) -> Text; | |
| #[sql_name = "array_to_string"] | |
| fn array_to_string_with_null_string<Arr: ArrayOrNullableArray + SingleValue>(array: Arr, del: Text, null: Text) -> Text; |
For similar reasons as outlined for the array_to_string function. The #[sql_name] attribute is required to use the right sql function name in the generated sql
|
@weiznich Need to resolve these merge conflicts though, its my bad 😢 |
Attempt to achieve
array_to_stringfunction for postgres listed in Issue #4153 https://www.postgresql.org/docs/current/functions-array.htmlSince it contained atleast one one optional argument of type
Nullable<Text>I've defined two separate variantsarray_to_string_nullandarray_to_stringas per recommendation.First PR here and first PR ever to open source so feel free to be harsh if this does not achieve intended goals as I understood them or does not follow best practices, open to suggestions.