ianmcook commented on a change in pull request #9745: URL: https://github.com/apache/arrow/pull/9745#discussion_r601568691
########## File path: r/tests/testthat/test-dplyr-arrange.R ########## @@ -0,0 +1,172 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +library(dplyr) + +tbl <- example_data_for_sorting + +test_that("arrange", { + expect_dplyr_equal( + input %>% + arrange(int, chr) %>% + collect(), + tbl %>% + slice_sample(prop = 1L) Review comment: Done in d9a5e09fb2b4507745e9c1033d5b9ff093869dfe ########## File path: r/tests/testthat/test-compute-sort.R ########## @@ -0,0 +1,143 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +context("compute: sorting") + +library(dplyr) + +tbl <- example_data_for_sorting + +test_that("sort(Scalar) is identity function", { + expect_identical( + as.vector(sort(Scalar$create(42L))), + 42L + ) + expect_identical( + as.vector(sort(Scalar$create("foo"))), + "foo" + ) +}) + +test_that("Array$SortIndices()", { + expect_equal( + Array$create(tbl$int)$SortIndices(), + Array$create(0L:9L, type = uint64()) + ) + expect_equal( + Array$create(rev(tbl$int))$SortIndices(descending = TRUE), + Array$create(c(1L:9L, 0L), type = uint64()) + ) +}) + +test_that("ChunkedArray$SortIndices()", { + expect_equal( + ChunkedArray$create(tbl$int[1:5], tbl$int[6:10])$SortIndices(), + Array$create(0L:9L, type = uint64()) + ) + expect_equal( + ChunkedArray$create(rev(tbl$int)[1:5], rev(tbl$int)[6:10])$SortIndices(descending = TRUE), + Array$create(c(1L:9L, 0L), type = uint64()) + ) +}) + +test_that("sort(vector), sort(Array), sort(ChunkedArray) give equivalent results on integers", { + expect_vector_equal( + sort(input), + tbl$int + ) + expect_vector_equal( + sort(input, na.last = NA), + tbl$int + ) + expect_vector_equal( + sort(input, na.last = TRUE), + tbl$int + ) + expect_vector_equal( + sort(input, na.last = FALSE), + tbl$int + ) + expect_vector_equal( + sort(input, decreasing = TRUE), + tbl$int, + ) + expect_vector_equal( + sort(input, decreasing = TRUE, na.last = TRUE), + tbl$int, + ) + expect_vector_equal( + sort(input, decreasing = TRUE, na.last = FALSE), + tbl$int, + ) +}) + +test_that("sort(vector), sort(Array), sort(ChunkedArray) give equivalent results on strings", { + skip_if_not( + identical(Sys.getlocale("LC_COLLATE"), "C"), + "Unexpected LC_COLLATE" + ) + expect_vector_equal( + sort(input, decreasing = TRUE, na.last = FALSE), + tbl$chr + ) + expect_vector_equal( + sort(input, decreasing = TRUE, na.last = FALSE), + tbl$chr + ) +}) + +test_that("sort(vector), sort(Array), sort(ChunkedArray) give equivalent results on floats", { + skip("is.na() evaluates to FALSE on Arrow NaN values (ARROW-12055)") Review comment: Correction: I misunderstood which skipped test this comment was referring to. The _other_ skipped test above were skipped because of the `test_that()` issue, but I worked around that in d9a5e09fb2b4507745e9c1033d5b9ff093869dfe. _This_ test is skipped because of the issue described in ARROW-12055, which is not directly related to the `na.last` workaround. But because of the `na.last` workaround, there are some combinations of `decreasing` and `na.last` values that _do_ work consistently in Arrow and R, so I added those above the `skip()` in 2d0777eba0628b8d4286d3125ce4ef5a2504141b. ########## File path: r/tests/testthat/test-compute-sort.R ########## @@ -0,0 +1,143 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +context("compute: sorting") + +library(dplyr) + +tbl <- example_data_for_sorting + +test_that("sort(Scalar) is identity function", { + expect_identical( + as.vector(sort(Scalar$create(42L))), + 42L + ) + expect_identical( + as.vector(sort(Scalar$create("foo"))), + "foo" + ) +}) + +test_that("Array$SortIndices()", { + expect_equal( + Array$create(tbl$int)$SortIndices(), + Array$create(0L:9L, type = uint64()) Review comment: Fixed in 2d0777eba0628b8d4286d3125ce4ef5a2504141b ########## File path: r/tests/testthat/test-compute-sort.R ########## @@ -0,0 +1,143 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +context("compute: sorting") + +library(dplyr) + +tbl <- example_data_for_sorting + +test_that("sort(Scalar) is identity function", { + expect_identical( + as.vector(sort(Scalar$create(42L))), + 42L + ) Review comment: That would work except that `Scalar$Equals()` wasn't implemented 😁 I implemented it in e44b7a1a56473c126892d5c0d6afe2b6980e3e33 and then simplified this test per your second suggestion. -- 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. For queries about this service, please contact Infrastructure at: [email protected]
