GH-38705: [C++][FS][Azure] Implement CopyFile() by kou · Pull Request #39058 · apache/arrow

kou · 2023-12-04T08:37:17Z

Rationale for this change

CopyFile() copies the given source to the given destination. Both of source and destination must be blob name like other filesystem implementations.

What changes are included in this PR?

Use CopyFromUri() API that should use server-side copy.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

Closes: [C++][FS][Azure] Implement CopyFile() #38705

github-actions · 2023-12-04T08:37:47Z

⚠️ GitHub issue #38705has been automatically assigned in GitHub to PR creator.

kou · 2023-12-04T08:38:45Z

@Tom-Newton @felipecrv What do you think about this behavior? I think that this is convenient but this is different from other filesystem implementations...

If the given destination has only container (container) or has trailing slash (container/directory/), base name of the given source is automatically appended. For example:
Source: container1/directory1/path.txt
Destination: container2
Real destination: container2/path.txt (path.txt is only appended to container2)
This is the same behavior as cp command. But other filesystem implementations require path not directory as destination.

Tom-Newton · 2023-12-04T09:33:28Z

This behaviour sounds fine to me.

I think there is a problem with the CopyFromUri() API though. I believe it requires using SAS token auth on the source, because the source URI needs to be self authenticated.

Tom-Newton · 2023-12-04T09:57:15Z

I think there is a problem with the CopyFromUri() API though.

Nevermind, it looks like you've got a working implementation using CopyFromUri(). Maybe it does auth automatically if it detects that the source is in the same storage account. Or possibly the Azure SDK automatically switches between Copy Blob From URL and Copy Blob

pitrou · 2023-12-04T15:27:33Z

I think that this is convenient but this is different from other filesystem implementations...

We should not diverge from the filesystem spec (as described in the FileSystem class docstrings ).
The end goal is for all implementations to expose a similar behavior.

Also, at some point the Azure fs implementation will have to implement and pass the generic filesystem tests, which do have a test for this situation.

felipecrv · 2023-12-04T18:18:15Z

I think that this is convenient but this is different from other filesystem implementations...
We should not diverge from the filesystem spec (as described in the FileSystem class docstrings ). The end goal is for all implementations to expose a similar behavior.
Also, at some point the Azure fs implementation will have to implement and pass the generic filesystem tests, which do have a test for this situation.

I agree. I think we should fix #38772 before adding more operations. I implemented the GetFileInfo(select) with this as an assumption. CopyFile shouldn't implicitly create directories, so we need a way to check an empty directory exists.

kou · 2023-12-05T00:42:31Z

Oh, sorry. I didn't read the docstring. I'll remove the behavior.

Also, at some point the Azure fs implementation will have to implement and pass the generic filesystem tests, which do have a test for this situation.

I've opened a new issue for it: #39069

kou · 2023-12-05T01:06:42Z

I think there is a problem with the CopyFromUri() API though.
Nevermind, it looks like you've got a working implementation using CopyFromUri(). Maybe it does auth automatically if it detects that the source is in the same storage account. Or possibly the Azure SDK automatically switches between Copy Blob From URL and Copy Blob

I think that "Copy Blob From URL" and "Copy Blob" are the same API internally.
Both of them use the same URI and PUT:

Copy Blob From URL: https://learn.microsoft.com/en-us/rest/api/storageservices/copy-blob-from-url#request
Copy Blob: https://learn.microsoft.com/en-us/rest/api/storageservices/copy-blob#request

And BlobClient provides only CopyFromUri(). (It doesn't provide Copy().)
So I used CopyFromUri().

kou · 2023-12-05T05:28:52Z

Updated:

Accept only blob path for destination like other implementations.

kou · 2023-12-06T21:42:12Z

I'll merge this in a few days if nobody objects it.

Tom-Newton

LGTM

Tom-Newton · 2023-12-06T22:47:27Z

Personally I'm not particularly worried but to be rigorous @felipecrv's comment should probably be addressed.

CopyFile shouldn't implicitly create directories, so we need a way to check an empty directory exists.

kou · 2023-12-07T00:02:37Z

OK. I've implemented it.

conbench-apache-arrow · 2023-12-08T15:05:13Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 70ccf33.

There were 10 benchmark results indicating a performance regression:

Commit Run on ursa-i9-9960x at 2023-12-07 14:23:01Z
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-06, scale_factor=1
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-06, scale_factor=10
and 8 more (see the report linked below)

The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them.

### Rationale for this change `CopyFile()` copies the given source to the given destination. Both of source and destination must be blob name like other filesystem implementations. ### What changes are included in this PR? Use `CopyFromUri()` API that should use server-side copy. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * Closes: apache#38705 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

kou · 2023-12-04T08:37:17Z

Rationale for this change

CopyFile() copies the given source to the given destination. Both of source and destination must be blob name like other filesystem implementations.

What changes are included in this PR?

Use CopyFromUri() API that should use server-side copy.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

Closes: [C++][FS][Azure] Implement CopyFile() #38705

github-actions · 2023-12-04T08:37:47Z

⚠️ GitHub issue #38705has been automatically assigned in GitHub to PR creator.

kou · 2023-12-04T08:38:45Z

@Tom-Newton @felipecrv What do you think about this behavior? I think that this is convenient but this is different from other filesystem implementations...

If the given destination has only container (container) or has trailing slash (container/directory/), base name of the given source is automatically appended. For example:
Source: container1/directory1/path.txt
Destination: container2
Real destination: container2/path.txt (path.txt is only appended to container2)
This is the same behavior as cp command. But other filesystem implementations require path not directory as destination.

Tom-Newton · 2023-12-04T09:33:28Z

This behaviour sounds fine to me.

I think there is a problem with the CopyFromUri() API though. I believe it requires using SAS token auth on the source, because the source URI needs to be self authenticated.

Tom-Newton · 2023-12-04T09:57:15Z

I think there is a problem with the CopyFromUri() API though.

Nevermind, it looks like you've got a working implementation using CopyFromUri(). Maybe it does auth automatically if it detects that the source is in the same storage account. Or possibly the Azure SDK automatically switches between Copy Blob From URL and Copy Blob

pitrou · 2023-12-04T15:27:33Z

I think that this is convenient but this is different from other filesystem implementations...

We should not diverge from the filesystem spec (as described in the FileSystem class docstrings ).
The end goal is for all implementations to expose a similar behavior.

Also, at some point the Azure fs implementation will have to implement and pass the generic filesystem tests, which do have a test for this situation.

felipecrv · 2023-12-04T18:18:15Z

I think that this is convenient but this is different from other filesystem implementations...
We should not diverge from the filesystem spec (as described in the FileSystem class docstrings ). The end goal is for all implementations to expose a similar behavior.
Also, at some point the Azure fs implementation will have to implement and pass the generic filesystem tests, which do have a test for this situation.

I agree. I think we should fix #38772 before adding more operations. I implemented the GetFileInfo(select) with this as an assumption. CopyFile shouldn't implicitly create directories, so we need a way to check an empty directory exists.

kou · 2023-12-05T00:42:31Z

Oh, sorry. I didn't read the docstring. I'll remove the behavior.

Also, at some point the Azure fs implementation will have to implement and pass the generic filesystem tests, which do have a test for this situation.

I've opened a new issue for it: #39069

kou · 2023-12-05T01:06:42Z

I think there is a problem with the CopyFromUri() API though.
Nevermind, it looks like you've got a working implementation using CopyFromUri(). Maybe it does auth automatically if it detects that the source is in the same storage account. Or possibly the Azure SDK automatically switches between Copy Blob From URL and Copy Blob

I think that "Copy Blob From URL" and "Copy Blob" are the same API internally.
Both of them use the same URI and PUT:

Copy Blob From URL: https://learn.microsoft.com/en-us/rest/api/storageservices/copy-blob-from-url#request
Copy Blob: https://learn.microsoft.com/en-us/rest/api/storageservices/copy-blob#request

And BlobClient provides only CopyFromUri(). (It doesn't provide Copy().)
So I used CopyFromUri().

kou · 2023-12-05T05:28:52Z

Updated:

Accept only blob path for destination like other implementations.

kou · 2023-12-06T21:42:12Z

I'll merge this in a few days if nobody objects it.

Tom-Newton

LGTM

Tom-Newton · 2023-12-06T22:47:27Z

Personally I'm not particularly worried but to be rigorous @felipecrv's comment should probably be addressed.

CopyFile shouldn't implicitly create directories, so we need a way to check an empty directory exists.

kou · 2023-12-07T00:02:37Z

OK. I've implemented it.

conbench-apache-arrow · 2023-12-08T15:05:13Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 70ccf33.

There were 10 benchmark results indicating a performance regression:

Commit Run on ursa-i9-9960x at 2023-12-07 14:23:01Z
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-06, scale_factor=1
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-06, scale_factor=10
and 8 more (see the report linked below)

The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them.

### Rationale for this change `CopyFile()` copies the given source to the given destination. Both of source and destination must be blob name like other filesystem implementations. ### What changes are included in this PR? Use `CopyFromUri()` API that should use server-side copy. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * Closes: apache#38705 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

kou · 2023-12-04T08:37:17Z

Rationale for this change

CopyFile() copies the given source to the given destination. Both of source and destination must be blob name like other filesystem implementations.

What changes are included in this PR?

Use CopyFromUri() API that should use server-side copy.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

Closes: [C++][FS][Azure] Implement CopyFile() #38705

github-actions · 2023-12-04T08:37:47Z

⚠️ GitHub issue #38705has been automatically assigned in GitHub to PR creator.

kou · 2023-12-04T08:38:45Z

@Tom-Newton @felipecrv What do you think about this behavior? I think that this is convenient but this is different from other filesystem implementations...

If the given destination has only container (container) or has trailing slash (container/directory/), base name of the given source is automatically appended. For example:
Source: container1/directory1/path.txt
Destination: container2
Real destination: container2/path.txt (path.txt is only appended to container2)
This is the same behavior as cp command. But other filesystem implementations require path not directory as destination.

Tom-Newton · 2023-12-04T09:33:28Z

This behaviour sounds fine to me.

I think there is a problem with the CopyFromUri() API though. I believe it requires using SAS token auth on the source, because the source URI needs to be self authenticated.

Tom-Newton · 2023-12-04T09:57:15Z

I think there is a problem with the CopyFromUri() API though.

Nevermind, it looks like you've got a working implementation using CopyFromUri(). Maybe it does auth automatically if it detects that the source is in the same storage account. Or possibly the Azure SDK automatically switches between Copy Blob From URL and Copy Blob

pitrou · 2023-12-04T15:27:33Z

I think that this is convenient but this is different from other filesystem implementations...

We should not diverge from the filesystem spec (as described in the FileSystem class docstrings ).
The end goal is for all implementations to expose a similar behavior.

Also, at some point the Azure fs implementation will have to implement and pass the generic filesystem tests, which do have a test for this situation.

felipecrv · 2023-12-04T18:18:15Z

I think that this is convenient but this is different from other filesystem implementations...
We should not diverge from the filesystem spec (as described in the FileSystem class docstrings ). The end goal is for all implementations to expose a similar behavior.
Also, at some point the Azure fs implementation will have to implement and pass the generic filesystem tests, which do have a test for this situation.

I agree. I think we should fix #38772 before adding more operations. I implemented the GetFileInfo(select) with this as an assumption. CopyFile shouldn't implicitly create directories, so we need a way to check an empty directory exists.

kou · 2023-12-05T00:42:31Z

Oh, sorry. I didn't read the docstring. I'll remove the behavior.

Also, at some point the Azure fs implementation will have to implement and pass the generic filesystem tests, which do have a test for this situation.

I've opened a new issue for it: #39069

kou · 2023-12-05T01:06:42Z

I think there is a problem with the CopyFromUri() API though.
Nevermind, it looks like you've got a working implementation using CopyFromUri(). Maybe it does auth automatically if it detects that the source is in the same storage account. Or possibly the Azure SDK automatically switches between Copy Blob From URL and Copy Blob

I think that "Copy Blob From URL" and "Copy Blob" are the same API internally.
Both of them use the same URI and PUT:

Copy Blob From URL: https://learn.microsoft.com/en-us/rest/api/storageservices/copy-blob-from-url#request
Copy Blob: https://learn.microsoft.com/en-us/rest/api/storageservices/copy-blob#request

And BlobClient provides only CopyFromUri(). (It doesn't provide Copy().)
So I used CopyFromUri().

kou · 2023-12-05T05:28:52Z

Updated:

Accept only blob path for destination like other implementations.

kou · 2023-12-06T21:42:12Z

I'll merge this in a few days if nobody objects it.

Tom-Newton

LGTM

Tom-Newton · 2023-12-06T22:47:27Z

Personally I'm not particularly worried but to be rigorous @felipecrv's comment should probably be addressed.

CopyFile shouldn't implicitly create directories, so we need a way to check an empty directory exists.

kou · 2023-12-07T00:02:37Z

OK. I've implemented it.

conbench-apache-arrow · 2023-12-08T15:05:13Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 70ccf33.

There were 10 benchmark results indicating a performance regression:

Commit Run on ursa-i9-9960x at 2023-12-07 14:23:01Z
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-06, scale_factor=1
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-06, scale_factor=10
and 8 more (see the report linked below)

The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them.

### Rationale for this change `CopyFile()` copies the given source to the given destination. Both of source and destination must be blob name like other filesystem implementations. ### What changes are included in this PR? Use `CopyFromUri()` API that should use server-side copy. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * Closes: apache#38705 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

kou · 2023-12-04T08:37:17Z

Rationale for this change

CopyFile() copies the given source to the given destination. Both of source and destination must be blob name like other filesystem implementations.

What changes are included in this PR?

Use CopyFromUri() API that should use server-side copy.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

Closes: [C++][FS][Azure] Implement CopyFile() #38705

github-actions · 2023-12-04T08:37:47Z

⚠️ GitHub issue #38705has been automatically assigned in GitHub to PR creator.

kou · 2023-12-04T08:38:45Z

@Tom-Newton @felipecrv What do you think about this behavior? I think that this is convenient but this is different from other filesystem implementations...

If the given destination has only container (container) or has trailing slash (container/directory/), base name of the given source is automatically appended. For example:
Source: container1/directory1/path.txt
Destination: container2
Real destination: container2/path.txt (path.txt is only appended to container2)
This is the same behavior as cp command. But other filesystem implementations require path not directory as destination.

Tom-Newton · 2023-12-04T09:33:28Z

This behaviour sounds fine to me.

I think there is a problem with the CopyFromUri() API though. I believe it requires using SAS token auth on the source, because the source URI needs to be self authenticated.

Tom-Newton · 2023-12-04T09:57:15Z

I think there is a problem with the CopyFromUri() API though.

Nevermind, it looks like you've got a working implementation using CopyFromUri(). Maybe it does auth automatically if it detects that the source is in the same storage account. Or possibly the Azure SDK automatically switches between Copy Blob From URL and Copy Blob

pitrou · 2023-12-04T15:27:33Z

I think that this is convenient but this is different from other filesystem implementations...

We should not diverge from the filesystem spec (as described in the FileSystem class docstrings ).
The end goal is for all implementations to expose a similar behavior.

Also, at some point the Azure fs implementation will have to implement and pass the generic filesystem tests, which do have a test for this situation.

felipecrv · 2023-12-04T18:18:15Z

I think that this is convenient but this is different from other filesystem implementations...
We should not diverge from the filesystem spec (as described in the FileSystem class docstrings ). The end goal is for all implementations to expose a similar behavior.
Also, at some point the Azure fs implementation will have to implement and pass the generic filesystem tests, which do have a test for this situation.

I agree. I think we should fix #38772 before adding more operations. I implemented the GetFileInfo(select) with this as an assumption. CopyFile shouldn't implicitly create directories, so we need a way to check an empty directory exists.

kou · 2023-12-05T00:42:31Z

Oh, sorry. I didn't read the docstring. I'll remove the behavior.

Also, at some point the Azure fs implementation will have to implement and pass the generic filesystem tests, which do have a test for this situation.

I've opened a new issue for it: #39069

kou · 2023-12-05T01:06:42Z

I think there is a problem with the CopyFromUri() API though.
Nevermind, it looks like you've got a working implementation using CopyFromUri(). Maybe it does auth automatically if it detects that the source is in the same storage account. Or possibly the Azure SDK automatically switches between Copy Blob From URL and Copy Blob

I think that "Copy Blob From URL" and "Copy Blob" are the same API internally.
Both of them use the same URI and PUT:

Copy Blob From URL: https://learn.microsoft.com/en-us/rest/api/storageservices/copy-blob-from-url#request
Copy Blob: https://learn.microsoft.com/en-us/rest/api/storageservices/copy-blob#request

And BlobClient provides only CopyFromUri(). (It doesn't provide Copy().)
So I used CopyFromUri().

kou · 2023-12-05T05:28:52Z

Updated:

Accept only blob path for destination like other implementations.

kou · 2023-12-06T21:42:12Z

I'll merge this in a few days if nobody objects it.

Tom-Newton

LGTM

Tom-Newton · 2023-12-06T22:47:27Z

Personally I'm not particularly worried but to be rigorous @felipecrv's comment should probably be addressed.

CopyFile shouldn't implicitly create directories, so we need a way to check an empty directory exists.

kou · 2023-12-07T00:02:37Z

OK. I've implemented it.

conbench-apache-arrow · 2023-12-08T15:05:13Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 70ccf33.

There were 10 benchmark results indicating a performance regression:

Commit Run on ursa-i9-9960x at 2023-12-07 14:23:01Z
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-06, scale_factor=1
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-06, scale_factor=10
and 8 more (see the report linked below)

The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them.

### Rationale for this change `CopyFile()` copies the given source to the given destination. Both of source and destination must be blob name like other filesystem implementations. ### What changes are included in this PR? Use `CopyFromUri()` API that should use server-side copy. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * Closes: apache#38705 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

kou · 2023-12-04T08:37:17Z

Rationale for this change

CopyFile() copies the given source to the given destination. Both of source and destination must be blob name like other filesystem implementations.

What changes are included in this PR?

Use CopyFromUri() API that should use server-side copy.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

Closes: [C++][FS][Azure] Implement CopyFile() #38705

github-actions · 2023-12-04T08:37:47Z

⚠️ GitHub issue #38705has been automatically assigned in GitHub to PR creator.

kou · 2023-12-04T08:38:45Z

@Tom-Newton @felipecrv What do you think about this behavior? I think that this is convenient but this is different from other filesystem implementations...

If the given destination has only container (container) or has trailing slash (container/directory/), base name of the given source is automatically appended. For example:
Source: container1/directory1/path.txt
Destination: container2
Real destination: container2/path.txt (path.txt is only appended to container2)
This is the same behavior as cp command. But other filesystem implementations require path not directory as destination.

Tom-Newton · 2023-12-04T09:33:28Z

This behaviour sounds fine to me.

I think there is a problem with the CopyFromUri() API though. I believe it requires using SAS token auth on the source, because the source URI needs to be self authenticated.

Tom-Newton · 2023-12-04T09:57:15Z

I think there is a problem with the CopyFromUri() API though.

Nevermind, it looks like you've got a working implementation using CopyFromUri(). Maybe it does auth automatically if it detects that the source is in the same storage account. Or possibly the Azure SDK automatically switches between Copy Blob From URL and Copy Blob

pitrou · 2023-12-04T15:27:33Z

I think that this is convenient but this is different from other filesystem implementations...

We should not diverge from the filesystem spec (as described in the FileSystem class docstrings ).
The end goal is for all implementations to expose a similar behavior.

Also, at some point the Azure fs implementation will have to implement and pass the generic filesystem tests, which do have a test for this situation.

felipecrv · 2023-12-04T18:18:15Z

I think that this is convenient but this is different from other filesystem implementations...
We should not diverge from the filesystem spec (as described in the FileSystem class docstrings ). The end goal is for all implementations to expose a similar behavior.
Also, at some point the Azure fs implementation will have to implement and pass the generic filesystem tests, which do have a test for this situation.

I agree. I think we should fix #38772 before adding more operations. I implemented the GetFileInfo(select) with this as an assumption. CopyFile shouldn't implicitly create directories, so we need a way to check an empty directory exists.

kou · 2023-12-05T00:42:31Z

Oh, sorry. I didn't read the docstring. I'll remove the behavior.

Also, at some point the Azure fs implementation will have to implement and pass the generic filesystem tests, which do have a test for this situation.

I've opened a new issue for it: #39069

kou · 2023-12-05T01:06:42Z

I think there is a problem with the CopyFromUri() API though.
Nevermind, it looks like you've got a working implementation using CopyFromUri(). Maybe it does auth automatically if it detects that the source is in the same storage account. Or possibly the Azure SDK automatically switches between Copy Blob From URL and Copy Blob

I think that "Copy Blob From URL" and "Copy Blob" are the same API internally.
Both of them use the same URI and PUT:

Copy Blob From URL: https://learn.microsoft.com/en-us/rest/api/storageservices/copy-blob-from-url#request
Copy Blob: https://learn.microsoft.com/en-us/rest/api/storageservices/copy-blob#request

And BlobClient provides only CopyFromUri(). (It doesn't provide Copy().)
So I used CopyFromUri().

kou · 2023-12-05T05:28:52Z

Updated:

Accept only blob path for destination like other implementations.

kou · 2023-12-06T21:42:12Z

I'll merge this in a few days if nobody objects it.

Tom-Newton

LGTM

Tom-Newton · 2023-12-06T22:47:27Z

Personally I'm not particularly worried but to be rigorous @felipecrv's comment should probably be addressed.

CopyFile shouldn't implicitly create directories, so we need a way to check an empty directory exists.

kou · 2023-12-07T00:02:37Z

OK. I've implemented it.

conbench-apache-arrow · 2023-12-08T15:05:13Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 70ccf33.

There were 10 benchmark results indicating a performance regression:

Commit Run on ursa-i9-9960x at 2023-12-07 14:23:01Z
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-06, scale_factor=1
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-06, scale_factor=10
and 8 more (see the report linked below)

The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them.

### Rationale for this change `CopyFile()` copies the given source to the given destination. Both of source and destination must be blob name like other filesystem implementations. ### What changes are included in this PR? Use `CopyFromUri()` API that should use server-side copy. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * Closes: apache#38705 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

kou · 2023-12-04T08:37:17Z

Rationale for this change

CopyFile() copies the given source to the given destination. Both of source and destination must be blob name like other filesystem implementations.

What changes are included in this PR?

Use CopyFromUri() API that should use server-side copy.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

Closes: [C++][FS][Azure] Implement CopyFile() #38705

github-actions · 2023-12-04T08:37:47Z

⚠️ GitHub issue #38705has been automatically assigned in GitHub to PR creator.

kou · 2023-12-04T08:38:45Z

@Tom-Newton @felipecrv What do you think about this behavior? I think that this is convenient but this is different from other filesystem implementations...

If the given destination has only container (container) or has trailing slash (container/directory/), base name of the given source is automatically appended. For example:
Source: container1/directory1/path.txt
Destination: container2
Real destination: container2/path.txt (path.txt is only appended to container2)
This is the same behavior as cp command. But other filesystem implementations require path not directory as destination.

Tom-Newton · 2023-12-04T09:33:28Z

This behaviour sounds fine to me.

I think there is a problem with the CopyFromUri() API though. I believe it requires using SAS token auth on the source, because the source URI needs to be self authenticated.

Tom-Newton · 2023-12-04T09:57:15Z

I think there is a problem with the CopyFromUri() API though.

Nevermind, it looks like you've got a working implementation using CopyFromUri(). Maybe it does auth automatically if it detects that the source is in the same storage account. Or possibly the Azure SDK automatically switches between Copy Blob From URL and Copy Blob

pitrou · 2023-12-04T15:27:33Z

I think that this is convenient but this is different from other filesystem implementations...

We should not diverge from the filesystem spec (as described in the FileSystem class docstrings ).
The end goal is for all implementations to expose a similar behavior.

Also, at some point the Azure fs implementation will have to implement and pass the generic filesystem tests, which do have a test for this situation.

felipecrv · 2023-12-04T18:18:15Z

I think that this is convenient but this is different from other filesystem implementations...
We should not diverge from the filesystem spec (as described in the FileSystem class docstrings ). The end goal is for all implementations to expose a similar behavior.
Also, at some point the Azure fs implementation will have to implement and pass the generic filesystem tests, which do have a test for this situation.

I agree. I think we should fix #38772 before adding more operations. I implemented the GetFileInfo(select) with this as an assumption. CopyFile shouldn't implicitly create directories, so we need a way to check an empty directory exists.

kou · 2023-12-05T00:42:31Z

Oh, sorry. I didn't read the docstring. I'll remove the behavior.

Also, at some point the Azure fs implementation will have to implement and pass the generic filesystem tests, which do have a test for this situation.

I've opened a new issue for it: #39069

kou · 2023-12-05T01:06:42Z

I think there is a problem with the CopyFromUri() API though.
Nevermind, it looks like you've got a working implementation using CopyFromUri(). Maybe it does auth automatically if it detects that the source is in the same storage account. Or possibly the Azure SDK automatically switches between Copy Blob From URL and Copy Blob

I think that "Copy Blob From URL" and "Copy Blob" are the same API internally.
Both of them use the same URI and PUT:

Copy Blob From URL: https://learn.microsoft.com/en-us/rest/api/storageservices/copy-blob-from-url#request
Copy Blob: https://learn.microsoft.com/en-us/rest/api/storageservices/copy-blob#request

And BlobClient provides only CopyFromUri(). (It doesn't provide Copy().)
So I used CopyFromUri().

kou · 2023-12-05T05:28:52Z

Updated:

Accept only blob path for destination like other implementations.

kou · 2023-12-06T21:42:12Z

I'll merge this in a few days if nobody objects it.

Tom-Newton

LGTM

Tom-Newton · 2023-12-06T22:47:27Z

Personally I'm not particularly worried but to be rigorous @felipecrv's comment should probably be addressed.

CopyFile shouldn't implicitly create directories, so we need a way to check an empty directory exists.

kou · 2023-12-07T00:02:37Z

OK. I've implemented it.

conbench-apache-arrow · 2023-12-08T15:05:13Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 70ccf33.

There were 10 benchmark results indicating a performance regression:

Commit Run on ursa-i9-9960x at 2023-12-07 14:23:01Z
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-06, scale_factor=1
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-06, scale_factor=10
and 8 more (see the report linked below)

The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them.

### Rationale for this change `CopyFile()` copies the given source to the given destination. Both of source and destination must be blob name like other filesystem implementations. ### What changes are included in this PR? Use `CopyFromUri()` API that should use server-side copy. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * Closes: apache#38705 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

kou · 2023-12-04T08:37:17Z

Rationale for this change

CopyFile() copies the given source to the given destination. Both of source and destination must be blob name like other filesystem implementations.

What changes are included in this PR?

Use CopyFromUri() API that should use server-side copy.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

Closes: [C++][FS][Azure] Implement CopyFile() #38705

github-actions · 2023-12-04T08:37:47Z

⚠️ GitHub issue #38705has been automatically assigned in GitHub to PR creator.

kou · 2023-12-04T08:38:45Z

@Tom-Newton @felipecrv What do you think about this behavior? I think that this is convenient but this is different from other filesystem implementations...

If the given destination has only container (container) or has trailing slash (container/directory/), base name of the given source is automatically appended. For example:
Source: container1/directory1/path.txt
Destination: container2
Real destination: container2/path.txt (path.txt is only appended to container2)
This is the same behavior as cp command. But other filesystem implementations require path not directory as destination.

Tom-Newton · 2023-12-04T09:33:28Z

This behaviour sounds fine to me.

I think there is a problem with the CopyFromUri() API though. I believe it requires using SAS token auth on the source, because the source URI needs to be self authenticated.

Tom-Newton · 2023-12-04T09:57:15Z

I think there is a problem with the CopyFromUri() API though.

Nevermind, it looks like you've got a working implementation using CopyFromUri(). Maybe it does auth automatically if it detects that the source is in the same storage account. Or possibly the Azure SDK automatically switches between Copy Blob From URL and Copy Blob

pitrou · 2023-12-04T15:27:33Z

I think that this is convenient but this is different from other filesystem implementations...

We should not diverge from the filesystem spec (as described in the FileSystem class docstrings ).
The end goal is for all implementations to expose a similar behavior.

Also, at some point the Azure fs implementation will have to implement and pass the generic filesystem tests, which do have a test for this situation.

felipecrv · 2023-12-04T18:18:15Z

I think that this is convenient but this is different from other filesystem implementations...
We should not diverge from the filesystem spec (as described in the FileSystem class docstrings ). The end goal is for all implementations to expose a similar behavior.
Also, at some point the Azure fs implementation will have to implement and pass the generic filesystem tests, which do have a test for this situation.

I agree. I think we should fix #38772 before adding more operations. I implemented the GetFileInfo(select) with this as an assumption. CopyFile shouldn't implicitly create directories, so we need a way to check an empty directory exists.

kou · 2023-12-05T00:42:31Z

Oh, sorry. I didn't read the docstring. I'll remove the behavior.

Also, at some point the Azure fs implementation will have to implement and pass the generic filesystem tests, which do have a test for this situation.

I've opened a new issue for it: #39069

kou · 2023-12-05T01:06:42Z

I think there is a problem with the CopyFromUri() API though.
Nevermind, it looks like you've got a working implementation using CopyFromUri(). Maybe it does auth automatically if it detects that the source is in the same storage account. Or possibly the Azure SDK automatically switches between Copy Blob From URL and Copy Blob

I think that "Copy Blob From URL" and "Copy Blob" are the same API internally.
Both of them use the same URI and PUT:

Copy Blob From URL: https://learn.microsoft.com/en-us/rest/api/storageservices/copy-blob-from-url#request
Copy Blob: https://learn.microsoft.com/en-us/rest/api/storageservices/copy-blob#request

And BlobClient provides only CopyFromUri(). (It doesn't provide Copy().)
So I used CopyFromUri().

kou · 2023-12-05T05:28:52Z

Updated:

Accept only blob path for destination like other implementations.

kou · 2023-12-06T21:42:12Z

I'll merge this in a few days if nobody objects it.

Tom-Newton

LGTM

Tom-Newton · 2023-12-06T22:47:27Z

Personally I'm not particularly worried but to be rigorous @felipecrv's comment should probably be addressed.

CopyFile shouldn't implicitly create directories, so we need a way to check an empty directory exists.

kou · 2023-12-07T00:02:37Z

OK. I've implemented it.

conbench-apache-arrow · 2023-12-08T15:05:13Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 70ccf33.

There were 10 benchmark results indicating a performance regression:

Commit Run on ursa-i9-9960x at 2023-12-07 14:23:01Z
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-06, scale_factor=1
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-06, scale_factor=10
and 8 more (see the report linked below)

The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them.

### Rationale for this change `CopyFile()` copies the given source to the given destination. Both of source and destination must be blob name like other filesystem implementations. ### What changes are included in this PR? Use `CopyFromUri()` API that should use server-side copy. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * Closes: apache#38705 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

kou · 2023-12-04T08:37:17Z

Rationale for this change

CopyFile() copies the given source to the given destination. Both of source and destination must be blob name like other filesystem implementations.

What changes are included in this PR?

Use CopyFromUri() API that should use server-side copy.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

Closes: [C++][FS][Azure] Implement CopyFile() #38705

github-actions · 2023-12-04T08:37:47Z

⚠️ GitHub issue #38705has been automatically assigned in GitHub to PR creator.

kou · 2023-12-04T08:38:45Z

@Tom-Newton @felipecrv What do you think about this behavior? I think that this is convenient but this is different from other filesystem implementations...

If the given destination has only container (container) or has trailing slash (container/directory/), base name of the given source is automatically appended. For example:
Source: container1/directory1/path.txt
Destination: container2
Real destination: container2/path.txt (path.txt is only appended to container2)
This is the same behavior as cp command. But other filesystem implementations require path not directory as destination.

Tom-Newton · 2023-12-04T09:33:28Z

This behaviour sounds fine to me.

I think there is a problem with the CopyFromUri() API though. I believe it requires using SAS token auth on the source, because the source URI needs to be self authenticated.

Tom-Newton · 2023-12-04T09:57:15Z

I think there is a problem with the CopyFromUri() API though.

Nevermind, it looks like you've got a working implementation using CopyFromUri(). Maybe it does auth automatically if it detects that the source is in the same storage account. Or possibly the Azure SDK automatically switches between Copy Blob From URL and Copy Blob

pitrou · 2023-12-04T15:27:33Z

I think that this is convenient but this is different from other filesystem implementations...

We should not diverge from the filesystem spec (as described in the FileSystem class docstrings ).
The end goal is for all implementations to expose a similar behavior.

Also, at some point the Azure fs implementation will have to implement and pass the generic filesystem tests, which do have a test for this situation.

felipecrv · 2023-12-04T18:18:15Z

I think that this is convenient but this is different from other filesystem implementations...
We should not diverge from the filesystem spec (as described in the FileSystem class docstrings ). The end goal is for all implementations to expose a similar behavior.
Also, at some point the Azure fs implementation will have to implement and pass the generic filesystem tests, which do have a test for this situation.

I agree. I think we should fix #38772 before adding more operations. I implemented the GetFileInfo(select) with this as an assumption. CopyFile shouldn't implicitly create directories, so we need a way to check an empty directory exists.

kou · 2023-12-05T00:42:31Z

Oh, sorry. I didn't read the docstring. I'll remove the behavior.

Also, at some point the Azure fs implementation will have to implement and pass the generic filesystem tests, which do have a test for this situation.

I've opened a new issue for it: #39069

kou · 2023-12-05T01:06:42Z

I think there is a problem with the CopyFromUri() API though.
Nevermind, it looks like you've got a working implementation using CopyFromUri(). Maybe it does auth automatically if it detects that the source is in the same storage account. Or possibly the Azure SDK automatically switches between Copy Blob From URL and Copy Blob

I think that "Copy Blob From URL" and "Copy Blob" are the same API internally.
Both of them use the same URI and PUT:

Copy Blob From URL: https://learn.microsoft.com/en-us/rest/api/storageservices/copy-blob-from-url#request
Copy Blob: https://learn.microsoft.com/en-us/rest/api/storageservices/copy-blob#request

And BlobClient provides only CopyFromUri(). (It doesn't provide Copy().)
So I used CopyFromUri().

kou · 2023-12-05T05:28:52Z

Updated:

Accept only blob path for destination like other implementations.

kou · 2023-12-06T21:42:12Z

I'll merge this in a few days if nobody objects it.

Tom-Newton

LGTM

Tom-Newton · 2023-12-06T22:47:27Z

Personally I'm not particularly worried but to be rigorous @felipecrv's comment should probably be addressed.

CopyFile shouldn't implicitly create directories, so we need a way to check an empty directory exists.

kou · 2023-12-07T00:02:37Z

OK. I've implemented it.

conbench-apache-arrow · 2023-12-08T15:05:13Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 70ccf33.

There were 10 benchmark results indicating a performance regression:

Commit Run on ursa-i9-9960x at 2023-12-07 14:23:01Z
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-06, scale_factor=1
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-06, scale_factor=10
and 8 more (see the report linked below)

The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them.

### Rationale for this change `CopyFile()` copies the given source to the given destination. Both of source and destination must be blob name like other filesystem implementations. ### What changes are included in this PR? Use `CopyFromUri()` API that should use server-side copy. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * Closes: apache#38705 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

github-actionsBot added Component: C++ awaiting committer review Awaiting committer review labels Dec 4, 2023

kou added 2 commits December 5, 2023 11:27

apacheGH-38705: [C++][FS][Azure] Implement CopyFile()
a46f614

Destination must be a blob path
f1fd538

kou force-pushed the cpp-azurefs-copy-file branch from df57308 to f1fd538Compare December 5, 2023 02:29

Tom-Newton approved these changes Dec 6, 2023
View reviewed changes

Reject directory blob
81af80e

felipecrv approved these changes Dec 7, 2023
View reviewed changes

kou merged commit 70ccf33 into apache:mainDec 7, 2023

kou deleted the cpp-azurefs-copy-file branch December 7, 2023 05:26

kou removed the awaiting committer review Awaiting committer review label Dec 7, 2023

Tom-Newton mentioned this pull request Apr 18, 2024
[C++][FS][Azure] CopyFile doesn't work with Azure hierarchical namespace support #41095
Closed

github-actionsBot added Component: C++ awaiting committer review Awaiting committer review labels Dec 4, 2023

kou added 2 commits December 5, 2023 11:27

apacheGH-38705: [C++][FS][Azure] Implement CopyFile()
a46f614

Destination must be a blob path
f1fd538

kou force-pushed the cpp-azurefs-copy-file branch from df57308 to f1fd538Compare December 5, 2023 02:29

Tom-Newton approved these changes Dec 6, 2023
View reviewed changes

Reject directory blob
81af80e

felipecrv approved these changes Dec 7, 2023
View reviewed changes

kou merged commit 70ccf33 into apache:mainDec 7, 2023

kou deleted the cpp-azurefs-copy-file branch December 7, 2023 05:26

kou removed the awaiting committer review Awaiting committer review label Dec 7, 2023

Tom-Newton mentioned this pull request Apr 18, 2024
[C++][FS][Azure] CopyFile doesn't work with Azure hierarchical namespace support #41095
Closed

github-actionsBot added Component: C++ awaiting committer review Awaiting committer review labels Dec 4, 2023

kou added 2 commits December 5, 2023 11:27

apacheGH-38705: [C++][FS][Azure] Implement CopyFile()
a46f614

Destination must be a blob path
f1fd538

kou force-pushed the cpp-azurefs-copy-file branch from df57308 to f1fd538Compare December 5, 2023 02:29

Tom-Newton approved these changes Dec 6, 2023
View reviewed changes

Reject directory blob
81af80e

felipecrv approved these changes Dec 7, 2023
View reviewed changes

kou merged commit 70ccf33 into apache:mainDec 7, 2023

kou deleted the cpp-azurefs-copy-file branch December 7, 2023 05:26

kou removed the awaiting committer review Awaiting committer review label Dec 7, 2023

Tom-Newton mentioned this pull request Apr 18, 2024
[C++][FS][Azure] CopyFile doesn't work with Azure hierarchical namespace support #41095
Closed

github-actionsBot added Component: C++ awaiting committer review Awaiting committer review labels Dec 4, 2023

kou added 2 commits December 5, 2023 11:27

apacheGH-38705: [C++][FS][Azure] Implement CopyFile()
a46f614

Destination must be a blob path
f1fd538

kou force-pushed the cpp-azurefs-copy-file branch from df57308 to f1fd538Compare December 5, 2023 02:29

Tom-Newton approved these changes Dec 6, 2023
View reviewed changes

Reject directory blob
81af80e

felipecrv approved these changes Dec 7, 2023
View reviewed changes

kou merged commit 70ccf33 into apache:mainDec 7, 2023

kou deleted the cpp-azurefs-copy-file branch December 7, 2023 05:26

kou removed the awaiting committer review Awaiting committer review label Dec 7, 2023

Tom-Newton mentioned this pull request Apr 18, 2024
[C++][FS][Azure] CopyFile doesn't work with Azure hierarchical namespace support #41095
Closed

github-actionsBot added Component: C++ awaiting committer review Awaiting committer review labels Dec 4, 2023

kou added 2 commits December 5, 2023 11:27

apacheGH-38705: [C++][FS][Azure] Implement CopyFile()
a46f614

Destination must be a blob path
f1fd538

kou force-pushed the cpp-azurefs-copy-file branch from df57308 to f1fd538Compare December 5, 2023 02:29

Tom-Newton approved these changes Dec 6, 2023
View reviewed changes

Reject directory blob
81af80e

felipecrv approved these changes Dec 7, 2023
View reviewed changes

kou merged commit 70ccf33 into apache:mainDec 7, 2023

kou deleted the cpp-azurefs-copy-file branch December 7, 2023 05:26

kou removed the awaiting committer review Awaiting committer review label Dec 7, 2023

Tom-Newton mentioned this pull request Apr 18, 2024
[C++][FS][Azure] CopyFile doesn't work with Azure hierarchical namespace support #41095
Closed

github-actionsBot added Component: C++ awaiting committer review Awaiting committer review labels Dec 4, 2023

kou added 2 commits December 5, 2023 11:27

apacheGH-38705: [C++][FS][Azure] Implement CopyFile()
a46f614

Destination must be a blob path
f1fd538

kou force-pushed the cpp-azurefs-copy-file branch from df57308 to f1fd538Compare December 5, 2023 02:29

Tom-Newton approved these changes Dec 6, 2023
View reviewed changes

Reject directory blob
81af80e

felipecrv approved these changes Dec 7, 2023
View reviewed changes

kou merged commit 70ccf33 into apache:mainDec 7, 2023

kou deleted the cpp-azurefs-copy-file branch December 7, 2023 05:26

kou removed the awaiting committer review Awaiting committer review label Dec 7, 2023

Tom-Newton mentioned this pull request Apr 18, 2024
[C++][FS][Azure] CopyFile doesn't work with Azure hierarchical namespace support #41095
Closed

github-actionsBot added Component: C++ awaiting committer review Awaiting committer review labels Dec 4, 2023

kou added 2 commits December 5, 2023 11:27

apacheGH-38705: [C++][FS][Azure] Implement CopyFile()
a46f614

Destination must be a blob path
f1fd538

kou force-pushed the cpp-azurefs-copy-file branch from df57308 to f1fd538Compare December 5, 2023 02:29

Tom-Newton approved these changes Dec 6, 2023
View reviewed changes

Reject directory blob
81af80e

felipecrv approved these changes Dec 7, 2023
View reviewed changes

kou merged commit 70ccf33 into apache:mainDec 7, 2023

kou deleted the cpp-azurefs-copy-file branch December 7, 2023 05:26

kou removed the awaiting committer review Awaiting committer review label Dec 7, 2023

Tom-Newton mentioned this pull request Apr 18, 2024
[C++][FS][Azure] CopyFile doesn't work with Azure hierarchical namespace support #41095
Closed

github-actionsBot added Component: C++ awaiting committer review Awaiting committer review labels Dec 4, 2023

kou added 2 commits December 5, 2023 11:27

apacheGH-38705: [C++][FS][Azure] Implement CopyFile()
a46f614

Destination must be a blob path
f1fd538

kou force-pushed the cpp-azurefs-copy-file branch from df57308 to f1fd538Compare December 5, 2023 02:29

Tom-Newton approved these changes Dec 6, 2023
View reviewed changes

Reject directory blob
81af80e

felipecrv approved these changes Dec 7, 2023
View reviewed changes

kou merged commit 70ccf33 into apache:mainDec 7, 2023

kou deleted the cpp-azurefs-copy-file branch December 7, 2023 05:26

kou removed the awaiting committer review Awaiting committer review label Dec 7, 2023

Tom-Newton mentioned this pull request Apr 18, 2024
[C++][FS][Azure] CopyFile doesn't work with Azure hierarchical namespace support #41095
Closed

Uh oh!

Conversation

kou commented Dec 4, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actionsBot commented Dec 4, 2023

Uh oh!

kou commented Dec 4, 2023

Uh oh!

Tom-Newton commented Dec 4, 2023

Uh oh!

Tom-Newton commented Dec 4, 2023

Uh oh!

pitrou commented Dec 4, 2023

Uh oh!

felipecrv commented Dec 4, 2023

Uh oh!

kou commented Dec 5, 2023

Uh oh!

kou commented Dec 5, 2023

Uh oh!

kou commented Dec 5, 2023

Uh oh!

kou commented Dec 6, 2023

Uh oh!

Tom-Newton left a comment

Choose a reason for hiding this comment

Uh oh!

Tom-Newton commented Dec 6, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kou commented Dec 7, 2023

Uh oh!

conbench-apache-arrowBot commented Dec 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

kou commented Dec 4, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actionsBot commented Dec 4, 2023

Uh oh!

kou commented Dec 4, 2023

Uh oh!

Tom-Newton commented Dec 4, 2023

Uh oh!

Tom-Newton commented Dec 4, 2023

Uh oh!

pitrou commented Dec 4, 2023

Uh oh!

felipecrv commented Dec 4, 2023

Uh oh!

kou commented Dec 5, 2023

Uh oh!

kou commented Dec 5, 2023

Uh oh!

kou commented Dec 5, 2023

Uh oh!

kou commented Dec 6, 2023

Uh oh!

Tom-Newton left a comment

Choose a reason for hiding this comment

Uh oh!

Tom-Newton commented Dec 6, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

kou commented Dec 4, 2023•
edited
Loading

Tom-Newton commented Dec 6, 2023•
edited
Loading

kou commented Dec 4, 2023•
edited
Loading

Tom-Newton commented Dec 6, 2023•
edited
Loading

kou commented Dec 4, 2023•
edited
Loading

Tom-Newton commented Dec 6, 2023•
edited
Loading