GH-34421: [R] Let GcsFileSystem take a path for json_credentials

[amoeba]

Rationale for this change

Existing documentation for this argument was misleading.

What changes are included in this PR?

A change in functionality, matching tests, and updated documentation are included. json_credentials can now either be a literal string containing credentials or a string containing a path to credentials. In the latter case, credentials will be automatically read in from the fileystem.

Are these changes tested?

Yes

Are there any user-facing changes?

Yes, though not breaking. This affects user-facing APIs and documentation and is both a bug fix and new functionality.

Closes#34421
Closes#33106

• Closes: [R] Allow GcsFileSystem$create to accept a path to json_credentials  #34421

[github-actions]

• Closes: [R] Allow GcsFileSystem$create to accept a path to json_credentials  #34421

[paleolimbot]

Thank you! Just a note about the encoding you assume and what happens when the file contains more than one line.

[paleolimbot]

I forget the exact details but I think readLines() assumes system encoding (rather than UTF-8, which matters on Windows). Any thought as to which one of those is a better assumption if the value points to a file? (The workaround would be for the user to read in the file themselves first with an explicit encoding).

[paleolimbot]

Also, did you want to collapse = "\n"?

[amoeba]

I forget the exact details but I think readLines() assumes system encoding (rather than UTF-8, which matters on Windows). Any thought as to which one of those is a better assumption if the value points to a file? (The workaround would be for the user to read in the file themselves first with an explicit encoding).

Thanks for catching this point. You're right about readLines' behavior. To your question, I'm not entirely sure. If I test on my Windows 11 VM, the system-wide encoding is Windows 1252, but JSON files downloaded via Edge produces either ASCII or UTF-8 files. Maybe this just works or maybe just works on newer Win10/11 builds?

Also, did you want to collapse = "\n"?

I suppose that's slightly better, though I think either works just the same. I'll change it.

[paleolimbot]

Maybe put some non-ASCII text and here? In test-csv there should be an example of how to do this (there are some \u and charToRaw() and writeBin things and I always forget the details).

[amoeba]

Done in f3018b0. I didn't go too wild with it.

[amoeba]

Thanks for the review @paleolimbot. Let me know what you think about the readLines and encoding thing. Other than that I think this is good to go.

[paleolimbot]

Rather than suppress this warning, you could (1) use something other than readLines() or (2) include a final newline in your test data. I'm guessing these are files downloaded from the google cloud console 99% of the time, so including a final newline would probably be sufficient.

(otherwise we might suppress warnings that are actual problems)

[amoeba]

Good point. I found I had to run the character vector through iconv to get exactly the content into the file (via writeBin). See 617d8b3. Otherwise, you always get a \00 (which R calls an embedded nul) at the end.

[paleolimbot]

...in case it's helpful: https://github.com/apache/arrow/blob/main/r/tests/testthat/test-csv.R#L318-L328

...and

read_utf8 <- function(file) {
res <- readBin(file, "raw", n = file.info(file)$size)
res <- rawToChar(res)
Encoding(res) <- "UTF-8"
res
}

[amoeba]

Thanks for this. I tried a few combinations that still used readLines and couldn't get something I liked that also passes CI. Even after ensuring the the test file has the exact UTF-8 bytes I want, readLines(path, encoding = "UTF-8") seems to still decode as Latin1. I stopped short of using a connection or mutating options. I can't reproduce this on my own Windows VM so testing solutions in CI is slow.

I ended up swapping out readLines for your above read_utf8 helper and previously updated the docs to indicate that the file must be UTF-8 encoded. I'll check CI later tonight/tomorrow.

[amoeba]

Hey @paleolimbot this is ready for a review now that all checks pass.

[amoeba]

CI failure is real failure,

-- Failure ('test-gcs.R:107:3'): GcsFileSystem$create() can read json_credentials --
`object` (`actual`) not equal to `expected` (`expected`).
`actual`: "{\"key\" : \"valué\"}"
`expected`: "{\"key\" : \"valué\"}" Backtrace:
x
1. \-arrow:::expect_equal(fs$options$json_credentials, "{\"key\" : \"valué\"}") at test-gcs.R:107:2
2. \-testthat::expect_equal(...) at D:\a\arrow\arrow\r\check\arrow.Rcheck\tests\testthat\helper-expectation.R:42:4

I'll look into this and ping you again when it's ready for review.

[paleolimbot]

Thank you for staying with me on the "non-ASCII" rabbit hole!

[amoeba]

Glad you caught it, thanks again!

[ursabot]

Benchmark runs are scheduled for baseline = ce0d20c and contender = d526fd9. d526fd9 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.42% ⬆️0.03%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.03% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] d526fd9d ec2-t3-xlarge-us-east-2
[Finished] d526fd9d test-mac-arm
[Finished] d526fd9d ursa-i9-9960x
[Finished] d526fd9d ursa-thinkcentre-m75q
[Finished] ce0d20c0 ec2-t3-xlarge-us-east-2
[Finished] ce0d20c0 test-mac-arm
[Finished] ce0d20c0 ursa-i9-9960x
[Finished] ce0d20c0 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[amoeba]

Rationale for this change

Existing documentation for this argument was misleading.

What changes are included in this PR?

A change in functionality, matching tests, and updated documentation are included. json_credentials can now either be a literal string containing credentials or a string containing a path to credentials. In the latter case, credentials will be automatically read in from the fileystem.

Are these changes tested?

Yes

Are there any user-facing changes?

Yes, though not breaking. This affects user-facing APIs and documentation and is both a bug fix and new functionality.

Closes#34421
Closes#33106

• Closes: [R] Allow GcsFileSystem$create to accept a path to json_credentials  #34421

[github-actions]

• Closes: [R] Allow GcsFileSystem$create to accept a path to json_credentials  #34421

[paleolimbot]

Thank you! Just a note about the encoding you assume and what happens when the file contains more than one line.

[paleolimbot]

I forget the exact details but I think readLines() assumes system encoding (rather than UTF-8, which matters on Windows). Any thought as to which one of those is a better assumption if the value points to a file? (The workaround would be for the user to read in the file themselves first with an explicit encoding).

[paleolimbot]

Also, did you want to collapse = "\n"?

[amoeba]

I forget the exact details but I think readLines() assumes system encoding (rather than UTF-8, which matters on Windows). Any thought as to which one of those is a better assumption if the value points to a file? (The workaround would be for the user to read in the file themselves first with an explicit encoding).

Thanks for catching this point. You're right about readLines' behavior. To your question, I'm not entirely sure. If I test on my Windows 11 VM, the system-wide encoding is Windows 1252, but JSON files downloaded via Edge produces either ASCII or UTF-8 files. Maybe this just works or maybe just works on newer Win10/11 builds?

Also, did you want to collapse = "\n"?

I suppose that's slightly better, though I think either works just the same. I'll change it.

[paleolimbot]

Maybe put some non-ASCII text and here? In test-csv there should be an example of how to do this (there are some \u and charToRaw() and writeBin things and I always forget the details).

[amoeba]

Done in f3018b0. I didn't go too wild with it.

[amoeba]

Thanks for the review @paleolimbot. Let me know what you think about the readLines and encoding thing. Other than that I think this is good to go.

[paleolimbot]

Rather than suppress this warning, you could (1) use something other than readLines() or (2) include a final newline in your test data. I'm guessing these are files downloaded from the google cloud console 99% of the time, so including a final newline would probably be sufficient.

(otherwise we might suppress warnings that are actual problems)

[amoeba]

Good point. I found I had to run the character vector through iconv to get exactly the content into the file (via writeBin). See 617d8b3. Otherwise, you always get a \00 (which R calls an embedded nul) at the end.

[paleolimbot]

...in case it's helpful: https://github.com/apache/arrow/blob/main/r/tests/testthat/test-csv.R#L318-L328

...and

read_utf8 <- function(file) {
res <- readBin(file, "raw", n = file.info(file)$size)
res <- rawToChar(res)
Encoding(res) <- "UTF-8"
res
}

[amoeba]

Thanks for this. I tried a few combinations that still used readLines and couldn't get something I liked that also passes CI. Even after ensuring the the test file has the exact UTF-8 bytes I want, readLines(path, encoding = "UTF-8") seems to still decode as Latin1. I stopped short of using a connection or mutating options. I can't reproduce this on my own Windows VM so testing solutions in CI is slow.

I ended up swapping out readLines for your above read_utf8 helper and previously updated the docs to indicate that the file must be UTF-8 encoded. I'll check CI later tonight/tomorrow.

[amoeba]

Hey @paleolimbot this is ready for a review now that all checks pass.

[amoeba]

CI failure is real failure,

-- Failure ('test-gcs.R:107:3'): GcsFileSystem$create() can read json_credentials --
`object` (`actual`) not equal to `expected` (`expected`).
`actual`: "{\"key\" : \"valué\"}"
`expected`: "{\"key\" : \"valué\"}" Backtrace:
x
1. \-arrow:::expect_equal(fs$options$json_credentials, "{\"key\" : \"valué\"}") at test-gcs.R:107:2
2. \-testthat::expect_equal(...) at D:\a\arrow\arrow\r\check\arrow.Rcheck\tests\testthat\helper-expectation.R:42:4

I'll look into this and ping you again when it's ready for review.

[paleolimbot]

Thank you for staying with me on the "non-ASCII" rabbit hole!

[amoeba]

Glad you caught it, thanks again!

[ursabot]

Benchmark runs are scheduled for baseline = ce0d20c and contender = d526fd9. d526fd9 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.42% ⬆️0.03%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.03% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] d526fd9d ec2-t3-xlarge-us-east-2
[Finished] d526fd9d test-mac-arm
[Finished] d526fd9d ursa-i9-9960x
[Finished] d526fd9d ursa-thinkcentre-m75q
[Finished] ce0d20c0 ec2-t3-xlarge-us-east-2
[Finished] ce0d20c0 test-mac-arm
[Finished] ce0d20c0 ursa-i9-9960x
[Finished] ce0d20c0 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[amoeba]

Rationale for this change

Existing documentation for this argument was misleading.

What changes are included in this PR?

A change in functionality, matching tests, and updated documentation are included. json_credentials can now either be a literal string containing credentials or a string containing a path to credentials. In the latter case, credentials will be automatically read in from the fileystem.

Are these changes tested?

Yes

Are there any user-facing changes?

Yes, though not breaking. This affects user-facing APIs and documentation and is both a bug fix and new functionality.

Closes#34421
Closes#33106

• Closes: [R] Allow GcsFileSystem$create to accept a path to json_credentials  #34421

[github-actions]

• Closes: [R] Allow GcsFileSystem$create to accept a path to json_credentials  #34421

[paleolimbot]

Thank you! Just a note about the encoding you assume and what happens when the file contains more than one line.

[paleolimbot]

I forget the exact details but I think readLines() assumes system encoding (rather than UTF-8, which matters on Windows). Any thought as to which one of those is a better assumption if the value points to a file? (The workaround would be for the user to read in the file themselves first with an explicit encoding).

[paleolimbot]

Also, did you want to collapse = "\n"?

[amoeba]

I forget the exact details but I think readLines() assumes system encoding (rather than UTF-8, which matters on Windows). Any thought as to which one of those is a better assumption if the value points to a file? (The workaround would be for the user to read in the file themselves first with an explicit encoding).

Thanks for catching this point. You're right about readLines' behavior. To your question, I'm not entirely sure. If I test on my Windows 11 VM, the system-wide encoding is Windows 1252, but JSON files downloaded via Edge produces either ASCII or UTF-8 files. Maybe this just works or maybe just works on newer Win10/11 builds?

Also, did you want to collapse = "\n"?

I suppose that's slightly better, though I think either works just the same. I'll change it.

[paleolimbot]

Maybe put some non-ASCII text and here? In test-csv there should be an example of how to do this (there are some \u and charToRaw() and writeBin things and I always forget the details).

[amoeba]

Done in f3018b0. I didn't go too wild with it.

[amoeba]

Thanks for the review @paleolimbot. Let me know what you think about the readLines and encoding thing. Other than that I think this is good to go.

[paleolimbot]

Rather than suppress this warning, you could (1) use something other than readLines() or (2) include a final newline in your test data. I'm guessing these are files downloaded from the google cloud console 99% of the time, so including a final newline would probably be sufficient.

(otherwise we might suppress warnings that are actual problems)

[amoeba]

Good point. I found I had to run the character vector through iconv to get exactly the content into the file (via writeBin). See 617d8b3. Otherwise, you always get a \00 (which R calls an embedded nul) at the end.

[paleolimbot]

...in case it's helpful: https://github.com/apache/arrow/blob/main/r/tests/testthat/test-csv.R#L318-L328

...and

read_utf8 <- function(file) {
res <- readBin(file, "raw", n = file.info(file)$size)
res <- rawToChar(res)
Encoding(res) <- "UTF-8"
res
}

[amoeba]

Thanks for this. I tried a few combinations that still used readLines and couldn't get something I liked that also passes CI. Even after ensuring the the test file has the exact UTF-8 bytes I want, readLines(path, encoding = "UTF-8") seems to still decode as Latin1. I stopped short of using a connection or mutating options. I can't reproduce this on my own Windows VM so testing solutions in CI is slow.

I ended up swapping out readLines for your above read_utf8 helper and previously updated the docs to indicate that the file must be UTF-8 encoded. I'll check CI later tonight/tomorrow.

[amoeba]

Hey @paleolimbot this is ready for a review now that all checks pass.

[amoeba]

CI failure is real failure,

-- Failure ('test-gcs.R:107:3'): GcsFileSystem$create() can read json_credentials --
`object` (`actual`) not equal to `expected` (`expected`).
`actual`: "{\"key\" : \"valué\"}"
`expected`: "{\"key\" : \"valué\"}" Backtrace:
x
1. \-arrow:::expect_equal(fs$options$json_credentials, "{\"key\" : \"valué\"}") at test-gcs.R:107:2
2. \-testthat::expect_equal(...) at D:\a\arrow\arrow\r\check\arrow.Rcheck\tests\testthat\helper-expectation.R:42:4

I'll look into this and ping you again when it's ready for review.

[paleolimbot]

Thank you for staying with me on the "non-ASCII" rabbit hole!

[amoeba]

Glad you caught it, thanks again!

[ursabot]

Benchmark runs are scheduled for baseline = ce0d20c and contender = d526fd9. d526fd9 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.42% ⬆️0.03%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.03% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] d526fd9d ec2-t3-xlarge-us-east-2
[Finished] d526fd9d test-mac-arm
[Finished] d526fd9d ursa-i9-9960x
[Finished] d526fd9d ursa-thinkcentre-m75q
[Finished] ce0d20c0 ec2-t3-xlarge-us-east-2
[Finished] ce0d20c0 test-mac-arm
[Finished] ce0d20c0 ursa-i9-9960x
[Finished] ce0d20c0 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[amoeba]

Rationale for this change

Existing documentation for this argument was misleading.

What changes are included in this PR?

A change in functionality, matching tests, and updated documentation are included. json_credentials can now either be a literal string containing credentials or a string containing a path to credentials. In the latter case, credentials will be automatically read in from the fileystem.

Are these changes tested?

Yes

Are there any user-facing changes?

Yes, though not breaking. This affects user-facing APIs and documentation and is both a bug fix and new functionality.

Closes#34421
Closes#33106

• Closes: [R] Allow GcsFileSystem$create to accept a path to json_credentials  #34421

[github-actions]

• Closes: [R] Allow GcsFileSystem$create to accept a path to json_credentials  #34421

[paleolimbot]

Thank you! Just a note about the encoding you assume and what happens when the file contains more than one line.

[paleolimbot]

I forget the exact details but I think readLines() assumes system encoding (rather than UTF-8, which matters on Windows). Any thought as to which one of those is a better assumption if the value points to a file? (The workaround would be for the user to read in the file themselves first with an explicit encoding).

[paleolimbot]

Also, did you want to collapse = "\n"?

[amoeba]

I forget the exact details but I think readLines() assumes system encoding (rather than UTF-8, which matters on Windows). Any thought as to which one of those is a better assumption if the value points to a file? (The workaround would be for the user to read in the file themselves first with an explicit encoding).

Thanks for catching this point. You're right about readLines' behavior. To your question, I'm not entirely sure. If I test on my Windows 11 VM, the system-wide encoding is Windows 1252, but JSON files downloaded via Edge produces either ASCII or UTF-8 files. Maybe this just works or maybe just works on newer Win10/11 builds?

Also, did you want to collapse = "\n"?

I suppose that's slightly better, though I think either works just the same. I'll change it.

[paleolimbot]

Maybe put some non-ASCII text and here? In test-csv there should be an example of how to do this (there are some \u and charToRaw() and writeBin things and I always forget the details).

[amoeba]

Done in f3018b0. I didn't go too wild with it.

[amoeba]

Thanks for the review @paleolimbot. Let me know what you think about the readLines and encoding thing. Other than that I think this is good to go.

[paleolimbot]

Rather than suppress this warning, you could (1) use something other than readLines() or (2) include a final newline in your test data. I'm guessing these are files downloaded from the google cloud console 99% of the time, so including a final newline would probably be sufficient.

(otherwise we might suppress warnings that are actual problems)

[amoeba]

Good point. I found I had to run the character vector through iconv to get exactly the content into the file (via writeBin). See 617d8b3. Otherwise, you always get a \00 (which R calls an embedded nul) at the end.

[paleolimbot]

...in case it's helpful: https://github.com/apache/arrow/blob/main/r/tests/testthat/test-csv.R#L318-L328

...and

read_utf8 <- function(file) {
res <- readBin(file, "raw", n = file.info(file)$size)
res <- rawToChar(res)
Encoding(res) <- "UTF-8"
res
}

[amoeba]

Thanks for this. I tried a few combinations that still used readLines and couldn't get something I liked that also passes CI. Even after ensuring the the test file has the exact UTF-8 bytes I want, readLines(path, encoding = "UTF-8") seems to still decode as Latin1. I stopped short of using a connection or mutating options. I can't reproduce this on my own Windows VM so testing solutions in CI is slow.

I ended up swapping out readLines for your above read_utf8 helper and previously updated the docs to indicate that the file must be UTF-8 encoded. I'll check CI later tonight/tomorrow.

[amoeba]

Hey @paleolimbot this is ready for a review now that all checks pass.

[amoeba]

CI failure is real failure,

-- Failure ('test-gcs.R:107:3'): GcsFileSystem$create() can read json_credentials --
`object` (`actual`) not equal to `expected` (`expected`).
`actual`: "{\"key\" : \"valué\"}"
`expected`: "{\"key\" : \"valué\"}" Backtrace:
x
1. \-arrow:::expect_equal(fs$options$json_credentials, "{\"key\" : \"valué\"}") at test-gcs.R:107:2
2. \-testthat::expect_equal(...) at D:\a\arrow\arrow\r\check\arrow.Rcheck\tests\testthat\helper-expectation.R:42:4

I'll look into this and ping you again when it's ready for review.

[paleolimbot]

Thank you for staying with me on the "non-ASCII" rabbit hole!

[amoeba]

Glad you caught it, thanks again!

[ursabot]

Benchmark runs are scheduled for baseline = ce0d20c and contender = d526fd9. d526fd9 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.42% ⬆️0.03%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.03% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] d526fd9d ec2-t3-xlarge-us-east-2
[Finished] d526fd9d test-mac-arm
[Finished] d526fd9d ursa-i9-9960x
[Finished] d526fd9d ursa-thinkcentre-m75q
[Finished] ce0d20c0 ec2-t3-xlarge-us-east-2
[Finished] ce0d20c0 test-mac-arm
[Finished] ce0d20c0 ursa-i9-9960x
[Finished] ce0d20c0 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[amoeba]

Rationale for this change

Existing documentation for this argument was misleading.

What changes are included in this PR?

A change in functionality, matching tests, and updated documentation are included. json_credentials can now either be a literal string containing credentials or a string containing a path to credentials. In the latter case, credentials will be automatically read in from the fileystem.

Are these changes tested?

Yes

Are there any user-facing changes?

Yes, though not breaking. This affects user-facing APIs and documentation and is both a bug fix and new functionality.

Closes#34421
Closes#33106

• Closes: [R] Allow GcsFileSystem$create to accept a path to json_credentials  #34421

[github-actions]

• Closes: [R] Allow GcsFileSystem$create to accept a path to json_credentials  #34421

[paleolimbot]

Thank you! Just a note about the encoding you assume and what happens when the file contains more than one line.

[paleolimbot]

I forget the exact details but I think readLines() assumes system encoding (rather than UTF-8, which matters on Windows). Any thought as to which one of those is a better assumption if the value points to a file? (The workaround would be for the user to read in the file themselves first with an explicit encoding).

[paleolimbot]

Also, did you want to collapse = "\n"?

[amoeba]

I forget the exact details but I think readLines() assumes system encoding (rather than UTF-8, which matters on Windows). Any thought as to which one of those is a better assumption if the value points to a file? (The workaround would be for the user to read in the file themselves first with an explicit encoding).

Thanks for catching this point. You're right about readLines' behavior. To your question, I'm not entirely sure. If I test on my Windows 11 VM, the system-wide encoding is Windows 1252, but JSON files downloaded via Edge produces either ASCII or UTF-8 files. Maybe this just works or maybe just works on newer Win10/11 builds?

Also, did you want to collapse = "\n"?

I suppose that's slightly better, though I think either works just the same. I'll change it.

[paleolimbot]

Maybe put some non-ASCII text and here? In test-csv there should be an example of how to do this (there are some \u and charToRaw() and writeBin things and I always forget the details).

[amoeba]

Done in f3018b0. I didn't go too wild with it.

[amoeba]

Thanks for the review @paleolimbot. Let me know what you think about the readLines and encoding thing. Other than that I think this is good to go.

[paleolimbot]

Rather than suppress this warning, you could (1) use something other than readLines() or (2) include a final newline in your test data. I'm guessing these are files downloaded from the google cloud console 99% of the time, so including a final newline would probably be sufficient.

(otherwise we might suppress warnings that are actual problems)

[amoeba]

Good point. I found I had to run the character vector through iconv to get exactly the content into the file (via writeBin). See 617d8b3. Otherwise, you always get a \00 (which R calls an embedded nul) at the end.

[paleolimbot]

...in case it's helpful: https://github.com/apache/arrow/blob/main/r/tests/testthat/test-csv.R#L318-L328

...and

read_utf8 <- function(file) {
res <- readBin(file, "raw", n = file.info(file)$size)
res <- rawToChar(res)
Encoding(res) <- "UTF-8"
res
}

[amoeba]

Thanks for this. I tried a few combinations that still used readLines and couldn't get something I liked that also passes CI. Even after ensuring the the test file has the exact UTF-8 bytes I want, readLines(path, encoding = "UTF-8") seems to still decode as Latin1. I stopped short of using a connection or mutating options. I can't reproduce this on my own Windows VM so testing solutions in CI is slow.

I ended up swapping out readLines for your above read_utf8 helper and previously updated the docs to indicate that the file must be UTF-8 encoded. I'll check CI later tonight/tomorrow.

[amoeba]

Hey @paleolimbot this is ready for a review now that all checks pass.

[amoeba]

CI failure is real failure,

-- Failure ('test-gcs.R:107:3'): GcsFileSystem$create() can read json_credentials --
`object` (`actual`) not equal to `expected` (`expected`).
`actual`: "{\"key\" : \"valué\"}"
`expected`: "{\"key\" : \"valué\"}" Backtrace:
x
1. \-arrow:::expect_equal(fs$options$json_credentials, "{\"key\" : \"valué\"}") at test-gcs.R:107:2
2. \-testthat::expect_equal(...) at D:\a\arrow\arrow\r\check\arrow.Rcheck\tests\testthat\helper-expectation.R:42:4

I'll look into this and ping you again when it's ready for review.

[paleolimbot]

Thank you for staying with me on the "non-ASCII" rabbit hole!

[amoeba]

Glad you caught it, thanks again!

[ursabot]

Benchmark runs are scheduled for baseline = ce0d20c and contender = d526fd9. d526fd9 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.42% ⬆️0.03%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.03% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] d526fd9d ec2-t3-xlarge-us-east-2
[Finished] d526fd9d test-mac-arm
[Finished] d526fd9d ursa-i9-9960x
[Finished] d526fd9d ursa-thinkcentre-m75q
[Finished] ce0d20c0 ec2-t3-xlarge-us-east-2
[Finished] ce0d20c0 test-mac-arm
[Finished] ce0d20c0 ursa-i9-9960x
[Finished] ce0d20c0 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[amoeba]

Rationale for this change

Existing documentation for this argument was misleading.

What changes are included in this PR?

A change in functionality, matching tests, and updated documentation are included. json_credentials can now either be a literal string containing credentials or a string containing a path to credentials. In the latter case, credentials will be automatically read in from the fileystem.

Are these changes tested?

Yes

Are there any user-facing changes?

Yes, though not breaking. This affects user-facing APIs and documentation and is both a bug fix and new functionality.

Closes#34421
Closes#33106

• Closes: [R] Allow GcsFileSystem$create to accept a path to json_credentials  #34421

[github-actions]

• Closes: [R] Allow GcsFileSystem$create to accept a path to json_credentials  #34421

[paleolimbot]

Thank you! Just a note about the encoding you assume and what happens when the file contains more than one line.

[paleolimbot]

I forget the exact details but I think readLines() assumes system encoding (rather than UTF-8, which matters on Windows). Any thought as to which one of those is a better assumption if the value points to a file? (The workaround would be for the user to read in the file themselves first with an explicit encoding).

[paleolimbot]

Also, did you want to collapse = "\n"?

[amoeba]

I forget the exact details but I think readLines() assumes system encoding (rather than UTF-8, which matters on Windows). Any thought as to which one of those is a better assumption if the value points to a file? (The workaround would be for the user to read in the file themselves first with an explicit encoding).

Thanks for catching this point. You're right about readLines' behavior. To your question, I'm not entirely sure. If I test on my Windows 11 VM, the system-wide encoding is Windows 1252, but JSON files downloaded via Edge produces either ASCII or UTF-8 files. Maybe this just works or maybe just works on newer Win10/11 builds?

Also, did you want to collapse = "\n"?

I suppose that's slightly better, though I think either works just the same. I'll change it.

[paleolimbot]

Maybe put some non-ASCII text and here? In test-csv there should be an example of how to do this (there are some \u and charToRaw() and writeBin things and I always forget the details).

[amoeba]

Done in f3018b0. I didn't go too wild with it.

[amoeba]

Thanks for the review @paleolimbot. Let me know what you think about the readLines and encoding thing. Other than that I think this is good to go.

[paleolimbot]

Rather than suppress this warning, you could (1) use something other than readLines() or (2) include a final newline in your test data. I'm guessing these are files downloaded from the google cloud console 99% of the time, so including a final newline would probably be sufficient.

(otherwise we might suppress warnings that are actual problems)

[amoeba]

Good point. I found I had to run the character vector through iconv to get exactly the content into the file (via writeBin). See 617d8b3. Otherwise, you always get a \00 (which R calls an embedded nul) at the end.

[paleolimbot]

...in case it's helpful: https://github.com/apache/arrow/blob/main/r/tests/testthat/test-csv.R#L318-L328

...and

read_utf8 <- function(file) {
res <- readBin(file, "raw", n = file.info(file)$size)
res <- rawToChar(res)
Encoding(res) <- "UTF-8"
res
}

[amoeba]

Thanks for this. I tried a few combinations that still used readLines and couldn't get something I liked that also passes CI. Even after ensuring the the test file has the exact UTF-8 bytes I want, readLines(path, encoding = "UTF-8") seems to still decode as Latin1. I stopped short of using a connection or mutating options. I can't reproduce this on my own Windows VM so testing solutions in CI is slow.

I ended up swapping out readLines for your above read_utf8 helper and previously updated the docs to indicate that the file must be UTF-8 encoded. I'll check CI later tonight/tomorrow.

[amoeba]

Hey @paleolimbot this is ready for a review now that all checks pass.

[amoeba]

CI failure is real failure,

-- Failure ('test-gcs.R:107:3'): GcsFileSystem$create() can read json_credentials --
`object` (`actual`) not equal to `expected` (`expected`).
`actual`: "{\"key\" : \"valué\"}"
`expected`: "{\"key\" : \"valué\"}" Backtrace:
x
1. \-arrow:::expect_equal(fs$options$json_credentials, "{\"key\" : \"valué\"}") at test-gcs.R:107:2
2. \-testthat::expect_equal(...) at D:\a\arrow\arrow\r\check\arrow.Rcheck\tests\testthat\helper-expectation.R:42:4

I'll look into this and ping you again when it's ready for review.

[paleolimbot]

Thank you for staying with me on the "non-ASCII" rabbit hole!

[amoeba]

Glad you caught it, thanks again!

[ursabot]

Benchmark runs are scheduled for baseline = ce0d20c and contender = d526fd9. d526fd9 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.42% ⬆️0.03%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.03% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] d526fd9d ec2-t3-xlarge-us-east-2
[Finished] d526fd9d test-mac-arm
[Finished] d526fd9d ursa-i9-9960x
[Finished] d526fd9d ursa-thinkcentre-m75q
[Finished] ce0d20c0 ec2-t3-xlarge-us-east-2
[Finished] ce0d20c0 test-mac-arm
[Finished] ce0d20c0 ursa-i9-9960x
[Finished] ce0d20c0 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[amoeba]

Rationale for this change

Existing documentation for this argument was misleading.

What changes are included in this PR?

A change in functionality, matching tests, and updated documentation are included. json_credentials can now either be a literal string containing credentials or a string containing a path to credentials. In the latter case, credentials will be automatically read in from the fileystem.

Are these changes tested?

Yes

Are there any user-facing changes?

Yes, though not breaking. This affects user-facing APIs and documentation and is both a bug fix and new functionality.

Closes#34421
Closes#33106

• Closes: [R] Allow GcsFileSystem$create to accept a path to json_credentials  #34421

[github-actions]

• Closes: [R] Allow GcsFileSystem$create to accept a path to json_credentials  #34421

[paleolimbot]

Thank you! Just a note about the encoding you assume and what happens when the file contains more than one line.

[paleolimbot]

I forget the exact details but I think readLines() assumes system encoding (rather than UTF-8, which matters on Windows). Any thought as to which one of those is a better assumption if the value points to a file? (The workaround would be for the user to read in the file themselves first with an explicit encoding).

[paleolimbot]

Also, did you want to collapse = "\n"?

[amoeba]

I forget the exact details but I think readLines() assumes system encoding (rather than UTF-8, which matters on Windows). Any thought as to which one of those is a better assumption if the value points to a file? (The workaround would be for the user to read in the file themselves first with an explicit encoding).

Thanks for catching this point. You're right about readLines' behavior. To your question, I'm not entirely sure. If I test on my Windows 11 VM, the system-wide encoding is Windows 1252, but JSON files downloaded via Edge produces either ASCII or UTF-8 files. Maybe this just works or maybe just works on newer Win10/11 builds?

Also, did you want to collapse = "\n"?

I suppose that's slightly better, though I think either works just the same. I'll change it.

[paleolimbot]

Maybe put some non-ASCII text and here? In test-csv there should be an example of how to do this (there are some \u and charToRaw() and writeBin things and I always forget the details).

[amoeba]

Done in f3018b0. I didn't go too wild with it.

[amoeba]

Thanks for the review @paleolimbot. Let me know what you think about the readLines and encoding thing. Other than that I think this is good to go.

[paleolimbot]

Rather than suppress this warning, you could (1) use something other than readLines() or (2) include a final newline in your test data. I'm guessing these are files downloaded from the google cloud console 99% of the time, so including a final newline would probably be sufficient.

(otherwise we might suppress warnings that are actual problems)

[amoeba]

Good point. I found I had to run the character vector through iconv to get exactly the content into the file (via writeBin). See 617d8b3. Otherwise, you always get a \00 (which R calls an embedded nul) at the end.

[paleolimbot]

...in case it's helpful: https://github.com/apache/arrow/blob/main/r/tests/testthat/test-csv.R#L318-L328

...and

read_utf8 <- function(file) {
res <- readBin(file, "raw", n = file.info(file)$size)
res <- rawToChar(res)
Encoding(res) <- "UTF-8"
res
}

[amoeba]

Thanks for this. I tried a few combinations that still used readLines and couldn't get something I liked that also passes CI. Even after ensuring the the test file has the exact UTF-8 bytes I want, readLines(path, encoding = "UTF-8") seems to still decode as Latin1. I stopped short of using a connection or mutating options. I can't reproduce this on my own Windows VM so testing solutions in CI is slow.

I ended up swapping out readLines for your above read_utf8 helper and previously updated the docs to indicate that the file must be UTF-8 encoded. I'll check CI later tonight/tomorrow.

[amoeba]

Hey @paleolimbot this is ready for a review now that all checks pass.

[amoeba]

CI failure is real failure,

-- Failure ('test-gcs.R:107:3'): GcsFileSystem$create() can read json_credentials --
`object` (`actual`) not equal to `expected` (`expected`).
`actual`: "{\"key\" : \"valué\"}"
`expected`: "{\"key\" : \"valué\"}" Backtrace:
x
1. \-arrow:::expect_equal(fs$options$json_credentials, "{\"key\" : \"valué\"}") at test-gcs.R:107:2
2. \-testthat::expect_equal(...) at D:\a\arrow\arrow\r\check\arrow.Rcheck\tests\testthat\helper-expectation.R:42:4

I'll look into this and ping you again when it's ready for review.

[paleolimbot]

Thank you for staying with me on the "non-ASCII" rabbit hole!

[amoeba]

Glad you caught it, thanks again!

[ursabot]

Benchmark runs are scheduled for baseline = ce0d20c and contender = d526fd9. d526fd9 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.42% ⬆️0.03%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.03% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] d526fd9d ec2-t3-xlarge-us-east-2
[Finished] d526fd9d test-mac-arm
[Finished] d526fd9d ursa-i9-9960x
[Finished] d526fd9d ursa-thinkcentre-m75q
[Finished] ce0d20c0 ec2-t3-xlarge-us-east-2
[Finished] ce0d20c0 test-mac-arm
[Finished] ce0d20c0 ursa-i9-9960x
[Finished] ce0d20c0 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[amoeba]

Rationale for this change

Existing documentation for this argument was misleading.

What changes are included in this PR?

A change in functionality, matching tests, and updated documentation are included. json_credentials can now either be a literal string containing credentials or a string containing a path to credentials. In the latter case, credentials will be automatically read in from the fileystem.

Are these changes tested?

Yes

Are there any user-facing changes?

Yes, though not breaking. This affects user-facing APIs and documentation and is both a bug fix and new functionality.

Closes#34421
Closes#33106

• Closes: [R] Allow GcsFileSystem$create to accept a path to json_credentials  #34421

[github-actions]

• Closes: [R] Allow GcsFileSystem$create to accept a path to json_credentials  #34421

[paleolimbot]

Thank you! Just a note about the encoding you assume and what happens when the file contains more than one line.

[paleolimbot]

I forget the exact details but I think readLines() assumes system encoding (rather than UTF-8, which matters on Windows). Any thought as to which one of those is a better assumption if the value points to a file? (The workaround would be for the user to read in the file themselves first with an explicit encoding).

[paleolimbot]

Also, did you want to collapse = "\n"?

[amoeba]

I forget the exact details but I think readLines() assumes system encoding (rather than UTF-8, which matters on Windows). Any thought as to which one of those is a better assumption if the value points to a file? (The workaround would be for the user to read in the file themselves first with an explicit encoding).

Thanks for catching this point. You're right about readLines' behavior. To your question, I'm not entirely sure. If I test on my Windows 11 VM, the system-wide encoding is Windows 1252, but JSON files downloaded via Edge produces either ASCII or UTF-8 files. Maybe this just works or maybe just works on newer Win10/11 builds?

Also, did you want to collapse = "\n"?

I suppose that's slightly better, though I think either works just the same. I'll change it.

[paleolimbot]

Maybe put some non-ASCII text and here? In test-csv there should be an example of how to do this (there are some \u and charToRaw() and writeBin things and I always forget the details).

[amoeba]

Done in f3018b0. I didn't go too wild with it.

[amoeba]

Thanks for the review @paleolimbot. Let me know what you think about the readLines and encoding thing. Other than that I think this is good to go.

[paleolimbot]

Rather than suppress this warning, you could (1) use something other than readLines() or (2) include a final newline in your test data. I'm guessing these are files downloaded from the google cloud console 99% of the time, so including a final newline would probably be sufficient.

(otherwise we might suppress warnings that are actual problems)

[amoeba]

Good point. I found I had to run the character vector through iconv to get exactly the content into the file (via writeBin). See 617d8b3. Otherwise, you always get a \00 (which R calls an embedded nul) at the end.

[paleolimbot]

...in case it's helpful: https://github.com/apache/arrow/blob/main/r/tests/testthat/test-csv.R#L318-L328

...and

read_utf8 <- function(file) {
res <- readBin(file, "raw", n = file.info(file)$size)
res <- rawToChar(res)
Encoding(res) <- "UTF-8"
res
}

[amoeba]

Thanks for this. I tried a few combinations that still used readLines and couldn't get something I liked that also passes CI. Even after ensuring the the test file has the exact UTF-8 bytes I want, readLines(path, encoding = "UTF-8") seems to still decode as Latin1. I stopped short of using a connection or mutating options. I can't reproduce this on my own Windows VM so testing solutions in CI is slow.

I ended up swapping out readLines for your above read_utf8 helper and previously updated the docs to indicate that the file must be UTF-8 encoded. I'll check CI later tonight/tomorrow.

[amoeba]

Hey @paleolimbot this is ready for a review now that all checks pass.

[amoeba]

CI failure is real failure,

-- Failure ('test-gcs.R:107:3'): GcsFileSystem$create() can read json_credentials --
`object` (`actual`) not equal to `expected` (`expected`).
`actual`: "{\"key\" : \"valué\"}"
`expected`: "{\"key\" : \"valué\"}" Backtrace:
x
1. \-arrow:::expect_equal(fs$options$json_credentials, "{\"key\" : \"valué\"}") at test-gcs.R:107:2
2. \-testthat::expect_equal(...) at D:\a\arrow\arrow\r\check\arrow.Rcheck\tests\testthat\helper-expectation.R:42:4

I'll look into this and ping you again when it's ready for review.

[paleolimbot]

Thank you for staying with me on the "non-ASCII" rabbit hole!

[amoeba]

Glad you caught it, thanks again!

[ursabot]

Benchmark runs are scheduled for baseline = ce0d20c and contender = d526fd9. d526fd9 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.42% ⬆️0.03%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.03% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] d526fd9d ec2-t3-xlarge-us-east-2
[Finished] d526fd9d test-mac-arm
[Finished] d526fd9d ursa-i9-9960x
[Finished] d526fd9d ursa-thinkcentre-m75q
[Finished] ce0d20c0 ec2-t3-xlarge-us-east-2
[Finished] ce0d20c0 test-mac-arm
[Finished] ce0d20c0 ursa-i9-9960x
[Finished] ce0d20c0 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java