You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2023/11/06 16:55:43 UTC
[PR] Minor: remove duplicated `array_replace` tests [arrow-datafusion]
alamb opened a new pull request, #8066:
URL: https://github.com/apache/arrow-datafusion/pull/8066
## Which issue does this PR close?
Part of https://github.com/apache/arrow-datafusion/issues/7988
## Rationale for this change
As pointed out by @jayzhan211 in https://github.com/apache/arrow-datafusion/pull/8054#discussion_r1382678796, sqllogictests tests are easier to maintain and also cover the logic end to end.
I was going to port the rest of the tests for `array_replace` to `array.slt` and when I went to do so I found out they were already there.
## What changes are included in this PR?
1. Remove unit tests from `array_expressions.rs`
2. Reformat tests in `array.slt` to make it clearer what is tested (and in particular all the cases are already covered)
## Are these changes tested?
Yes, by existing tests
## Are there any user-facing changes?
No
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Minor: remove duplicated `array_replace` tests [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8066:
URL: https://github.com/apache/arrow-datafusion/pull/8066#discussion_r1383661072
##########
datafusion/sqllogictest/test_files/array.slt:
##########
@@ -1568,19 +1568,35 @@ select array_positions(column1, make_array(4, 5, 6)), array_positions(make_array
# array_replace scalar function #1
query ???
-select array_replace(make_array(1, 2, 3, 4), 2, 3), array_replace(make_array(1, 4, 4, 5, 4, 6, 7), 4, 0), array_replace(make_array(1, 2, 3), 4, 0);
+select
+ array_replace(make_array(1, 2, 3, 4), 2, 3),
+ array_replace(make_array(1, 4, 4, 5, 4, 6, 7), 4, 0),
+ array_replace(make_array(1, 2, 3), 4, 0);
----
[1, 3, 3, 4] [1, 0, 4, 5, 4, 6, 7] [1, 2, 3]
# array_replace scalar function #2 (element is list)
query ??
-select array_replace(make_array([1, 2, 3], [4, 5, 6], [5, 5, 5], [4, 5, 6], [7, 8, 9]), [4, 5, 6], [1, 1, 1]), array_replace(make_array([1, 3, 2], [2, 3, 4], [2, 3, 4], [5, 3, 1], [1, 3, 2]), [2, 3, 4], [3, 1, 4]);
+select
+ array_replace(
+ make_array([1, 2, 3], [4, 5, 6], [5, 5, 5], [4, 5, 6], [7, 8, 9]),
Review Comment:
by rewriting with some whitespace, I think it is much easier now to see what this test is covering
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Minor: remove duplicated `array_replace` tests [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8066:
URL: https://github.com/apache/arrow-datafusion/pull/8066#issuecomment-1795493535
I wonder what you think of this @jayzhan211 ? If you agree that this is an improvement, I can do the same for the other tests in `array_expressions.rs`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Minor: remove duplicated `array_replace` tests [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8066:
URL: https://github.com/apache/arrow-datafusion/pull/8066
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Minor: remove duplicated `array_replace` tests [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #8066:
URL: https://github.com/apache/arrow-datafusion/pull/8066#issuecomment-1797004052
> I wonder what you think of this @jayzhan211 ? If you agree that this is an improvement, I can do the same for the other tests in `array_expressions.rs`.
We should move test to slt file if possible!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org