ARROW-10623: [CI][R] Version 1.0.1 breaks data.frame attributes when reading file written by 2.0.0 #9118

jonkeane · 2021-01-06T23:13:02Z

No description provided.

jonkeane · 2021-01-06T23:17:40Z

@github-actions crossbow submit test-r-version-compatibility

nealrichardson · 2021-01-06T23:28:11Z

dev/tasks/r/azure.linux.yml

not needed?

r/extra-tests/write-parquet.R

nealrichardson · 2021-01-06T23:32:10Z

r/tests/testthat/test-parquet-compatibility.R

resolve this. is this for testing that we can read files written by older versions?

nealrichardson · 2021-01-06T23:32:55Z

r/tests/testthat/test-parquet-compatibility.R

why is that a problem? testthat evals in the package namespace so you can call unexported functions

nealrichardson · 2021-01-06T23:34:39Z

r/tests/testthat/test-parquet-compatibility.R

This workaround is for reading files written by 2.0 in 1.0, right? IIUC that should be tested in the extra-tests

Yeah, this file isn't what it needs to be for the golden tests we were thinking about — that's my next step. This is basically as it was when I was using it for forward compatibility

jonkeane · 2021-01-07T01:23:10Z

@github-actions crossbow submit test-r-version-compatibility

jonkeane · 2021-01-07T02:12:47Z

@github-actions crossbow submit test-r-version-compatibility

jonkeane · 2021-01-07T16:43:12Z

@github-actions crossbow submit test-r-version-compatibility

github-actions · 2021-01-07T16:52:39Z

Revision: 1c58ce2e42fe9b1bf3ace430ca965b52fa079d09

Submitted crossbow builds: ursa-labs/crossbow @ actions-835

Task	Status
test-r-version-compatibility

jonkeane · 2021-01-07T17:49:19Z

@github-actions crossbow submit test-r-version-compatibility

github-actions · 2021-01-07T17:50:36Z

Revision: 1c58ce2e42fe9b1bf3ace430ca965b52fa079d09

Submitted crossbow builds: ursa-labs/crossbow @ actions-836

Task	Status
test-r-version-compatibility

jonkeane · 2021-01-07T19:02:46Z

@github-actions crossbow submit test-r-version-compatibility

github-actions · 2021-01-07T19:04:34Z

Revision: c92939bfc102618d1e683ea192962061b41fa56e

Submitted crossbow builds: ursa-labs/crossbow @ actions-837

Task	Status
test-r-version-compatibility

nealrichardson

Looks like you're moving in the right direction

nealrichardson · 2021-01-07T19:16:29Z

r/extra-tests/test-read-parquet.R

This op is a little non-obvious. I'd rather have explicit skip function names (skip_if_version_equal, skip_if_version_less_than, etc.)

jonkeane · 2021-01-07T20:22:55Z

@github-actions crossbow submit test-r-version-compatibility

github-actions · 2021-01-07T20:24:19Z

Revision: 1d7788cb1466438b143f8bd2d24b9338fd0c7ac9

Submitted crossbow builds: ursa-labs/crossbow @ actions-839

Task	Status
test-r-version-compatibility

jonkeane · 2021-01-07T22:19:17Z

@github-actions crossbow submit test-r-version-compatibility

github-actions · 2021-01-07T22:20:24Z

Revision: 0cc1d259342f30c1e353f2680b7778c7c89d1f4e

Submitted crossbow builds: ursa-labs/crossbow @ actions-840

Task	Status
test-r-version-compatibility

jonkeane · 2021-01-07T22:53:02Z

@github-actions crossbow submit test-r-version-compatibility

github-actions · 2021-01-07T22:53:59Z

Revision: 2d9cdf3042b2a2763a5d0e0f0d67685dc3def499

Submitted crossbow builds: ursa-labs/crossbow @ actions-841

Task	Status
test-r-version-compatibility

jonkeane · 2021-01-08T18:28:41Z

@github-actions crossbow submit test-r-version-compatibility

github-actions · 2021-01-08T19:07:50Z

Revision: da48c83b1587e49b92c074096bae7e74db2e712f

Submitted crossbow builds: ursa-labs/crossbow @ actions-848

Task	Status
test-r-version-compatibility

jonkeane · 2021-01-08T20:33:48Z

And fork CI passing:
https://github.com/jonkeane/arrow/actions/runs/472646827
https://github.com/jonkeane/arrow/actions/runs/472646829

nealrichardson

A few notes. Can you please update the JIRA in the PR title to be all caps etc.?

nealrichardson · 2021-01-08T21:48:25Z

r/extra-tests/helpers.R

why not packageVersion("arrow")?

nealrichardson · 2021-01-08T21:51:56Z

r/extra-tests/test-read-files.R

Suggested change

test_that(paste0("Can read feather version 1"), {

test_that("Can read feather version 1", {

nealrichardson · 2021-01-08T21:53:45Z

r/extra-tests/test-read-files.R

Suggested change

test_that("Can see the metadata (feather)", {

test_that("Can see the metadata (parquet)", {

?

nealrichardson · 2021-01-08T21:55:15Z

r/tests/testthat/test-backwards-compatibility.R

Shouldn't this be failing on master per ARROW-10850?

It looks like 1.0.1 works fine, but earlier versions (e.g. 0.17.0) saves files with the wrong case and does fail, but 1.0.1 (and 1.0.0) doesn't seem to have this issue. I need to dig more to inspect the files saved by 1.0.1 / through the source to find if there was a tolower() or the like that was introduced after 0.17

Indeed, AFAIK, the bug only occurs for files written with 0.17, and when reading with 2.0.0.
In Arrow 0.17, we had experimental compression support specifically for feather (using some metadata string), while starting from Arrow 1.0, compression is part of the official format spec. So it's only those older pre-1.0.0 files that are the problem. And it only started failing to read them in Arrow 2.0, because we simply changed how the compressions are coded internally from lower case (as it was stored in the feather file) to upper case, in an unrelated commit (3694794#r43851633), forgetting the compatibility issue with older feather files.

Thanks for confirming this. I went searching on Friday and hadn't yet fully reconstructed that time log from git history :)

jorisvandenbossche · 2021-01-11T14:15:19Z

r/tests/testthat/test-backwards-compatibility.R

So once #9128 is merged, this should be fixed

Great, I'll change these to skips and than add the Jira to the message then.

github-actions · 2021-01-11T15:06:47Z

https://issues.apache.org/jira/browse/ARROW-10623

…odecs

jonkeane · 2021-01-11T15:55:59Z

@github-actions crossbow submit test-r-version-compatibility

nealrichardson

Looking good, just a note about consolidating some test code. Is there anything left to do here otherwise?

nealrichardson · 2021-01-11T16:22:52Z

r/extra-tests/test-read-files.R

+  expect_s3_class(df, "tbl")
+
+  expect_equal(
+    attributes(df),
+    list(
+      names = letters[1:4],
+      row.names = 1L,
+      top_level = list(
+        field_one = 12,
+        field_two = "more stuff"
+      ),
+      class = c("tbl_df", "tbl", "data.frame")
+    )
+  )
+
+  # column-level attributes
+  expect_equal(attributes(df$a), list(class = "special_string"))
+  expect_equal(
+    attributes(df$c),
+    list(
+      row.names = 1L,
+      names = c("c1", "c2", "c3"),
+      class = c("tbl_df", "tbl", "data.frame")
+    )
+  )
+})


What do you think about factoring this out to an expect_something() helper in this file, such that your tests here are just expect_something(df)?

Of course, the simplest one would be expect_identical(df, example_with_metadata), right? Though there are tradeoffs with that.

nealrichardson · 2021-01-11T16:25:14Z

r/tests/testthat/test-backwards-compatibility.R

+  # 1.0.1 didn't save top-level metadata, so we need to remove it.
+  example_with_metadata_sans_toplevel <- example_with_metadata
+  attributes(example_with_metadata_sans_toplevel)$top_level <- NULL
+  expect_equal(df, example_with_metadata_sans_toplevel)


why not just expect_identical and skip the rest of the assertions?

jonkeane · 2021-01-11T16:52:10Z

I'll consolidate those tests some more like you mentioned. The only thing left to do (if we want to) would be to add backwards compatibility tests for stream-to-filesystem. I can add them if they add substantially more coverage than we have with feather files themselves.

github-actions · 2021-01-11T17:57:09Z

Revision: 5c9fe3f

Submitted crossbow builds: ursa-labs/crossbow @ actions-870

Task	Status
test-r-version-compatibility

nealrichardson reviewed Jan 6, 2021

View reviewed changes

github-actions bot added the Component: R label Jan 7, 2021

jonkeane changed the title ~~[WIP] Arrow 10623: [R] Version 1.0.1 breaks data.frame attributes when reading file written by 2.0.0~~ [WIP] Arrow 10623: [CI][R] Version 1.0.1 breaks data.frame attributes when reading file written by 2.0.0 Jan 7, 2021

nealrichardson reviewed Jan 7, 2021

View reviewed changes

nealrichardson mentioned this pull request Jan 7, 2021

ARROW-11163: [C++] Fix reading of compressed IPC/Feather files written with Arrow 0.17 #9128

Closed

jonkeane changed the title ~~[WIP] Arrow 10623: [CI][R] Version 1.0.1 breaks data.frame attributes when reading file written by 2.0.0~~ Arrow 10623: [CI][R] Version 1.0.1 breaks data.frame attributes when reading file written by 2.0.0 Jan 8, 2021

nealrichardson reviewed Jan 8, 2021

View reviewed changes

jorisvandenbossche changed the title ~~Arrow 10623: [CI][R] Version 1.0.1 breaks data.frame attributes when reading file written by 2.0.0~~ ARROW-10623: [CI][R] Version 1.0.1 breaks data.frame attributes when reading file written by 2.0.0 Jan 11, 2021

jorisvandenbossche reviewed Jan 11, 2021

View reviewed changes

jonkeane added 2 commits January 11, 2021 09:50

Basic forwards compatibility testing

babe5b7

Trying on GHA

f0ce0f1

jonkeane added 20 commits January 11, 2021 09:50

Cleanup

0961b2b

oops

afb0aee

Oy

350c483

still more changes

b4c23df

Move workflow to crossbox

d9e7e61

Add a crossbow task

8b92050

Escape jinja

5cd59d5

More indirection?

4814d4a

Oops extra typo

faa9f37

A slightly different way of loading the crossbow...

b6ec09f

Add feather tests

3959ed3

More filetypes, will probably fail on 1.0.1 for streams + metadata.

afed5b6

Helpers need some copying too

71e74ea

Add backwards compatibility testing

54d8d2d

Add skips if snappy isn't available

9871abc

Add feather files

5cab1b2

Add apache licensing where it should be.

70abb3e

PR comments, add 0.17.0 fixture that exhibits the case mis-match on c…

2323e8f

…odecs

Add skip for upcoming compatibility change.

8bcb580

Remove the skip now that ARROW-11163 is merged

5c9fe3f

jonkeane force-pushed the ARROW-10623-attr-writing-test branch from 64f69c7 to 5c9fe3f Compare January 11, 2021 15:52

nealrichardson reviewed Jan 11, 2021

View reviewed changes

Make a helper, test some (but not all) of the 0.17.0 reading.

e134e60

nealrichardson closed this in 9b3aaa8 Jan 11, 2021

jonkeane deleted the ARROW-10623-attr-writing-test branch May 5, 2021 12:53

asfimport mentioned this pull request Mar 10, 2021

[R] Version 1.0.1 breaks data.frame attributes when reading file written by 2.0.0 #26579

Closed

	test_that(paste0("Can read feather version 1"), {
	test_that("Can read feather version 1", {

	test_that("Can see the metadata (feather)", {
	test_that("Can see the metadata (parquet)", {

ARROW-10623: [CI][R] Version 1.0.1 breaks data.frame attributes when reading file written by 2.0.0 #9118

ARROW-10623: [CI][R] Version 1.0.1 breaks data.frame attributes when reading file written by 2.0.0 #9118

Uh oh!

Conversation

jonkeane commented Jan 6, 2021

Uh oh!

jonkeane commented Jan 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonkeane commented Jan 7, 2021

Uh oh!

jonkeane commented Jan 7, 2021

Uh oh!

jonkeane commented Jan 7, 2021

Uh oh!

github-actions bot commented Jan 7, 2021

Uh oh!

jonkeane commented Jan 7, 2021

Uh oh!

github-actions bot commented Jan 7, 2021

Uh oh!

jonkeane commented Jan 7, 2021

Uh oh!

github-actions bot commented Jan 7, 2021

Uh oh!

nealrichardson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonkeane commented Jan 7, 2021

Uh oh!

github-actions bot commented Jan 7, 2021

Uh oh!

jonkeane commented Jan 7, 2021

Uh oh!

github-actions bot commented Jan 7, 2021

Uh oh!

jonkeane commented Jan 7, 2021

Uh oh!

github-actions bot commented Jan 7, 2021

Uh oh!

jonkeane commented Jan 8, 2021

Uh oh!

github-actions bot commented Jan 8, 2021

Uh oh!

jonkeane commented Jan 8, 2021

Uh oh!

nealrichardson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 11, 2021