minor: Derive Clone for InvalidPart

[alamb]

Which issue does this PR close?

N/A

Rationale for this change

We have manual code to copy this structure downstream in InfluxDB IOx and it would be nice to have it automatically derived

What changes are included in this PR?

Derive Clone for InvalidPart

Are there any user-facing changes?

New Clone impl

[tustvold]

This is relatively harmless, but does strike me as a bit odd, one wouldn't normally derive Clone for Error types.

I don't know the circumstances in which this error is being used, but .to_string() might be an option

[alamb]

This is relatively harmless, but does strike me as a bit odd, one wouldn't normally derive Clone for Error types.

There are various times I think the lack of Clone causes annoyances downstream for us:

• (e.g. getting a Box<dyn Error> that we are downcasting to a concrete object store error to fulfill some other contract where we would be quite happy to pay the copy cost)
• Trying to save the error for results analysis

I realize the lack of Clone is not arrow-rs specific (it is Rust), but it strikes me as overly pedantic sometimes.

Here is a related discusion: rust-lang/rust#24135 (and having std::io::Error not clone makes it hard/impossible to automatically derive Clone for other error types 🤔 )

[tustvold]

Right, I understand the use-case, the challenge is that currently we consistently don't implement Clone for errors, and for many errors we can't (as they contain Box<dyn Error>). The result is that this PR would introduce inconsistency.

I dunno, I don't feel overly strongly, but it does seem like the ecosystem consensus is not to do this

[alamb]

Right, I understand the use-case, the challenge is that currently we consistently don't implement Clone for errors, and for many errors we can't (as they contain Box<dyn Error>). The result is that this PR would introduce inconsistency.

I dunno, I don't feel overly strongly, but it does seem like the ecosystem consensus is not to do this

Fair enough, I also agree this change is inconsistent (and doesn't really help the main usecase I have downstream). It may also overly constrain future changes to this variant (e.g. prevents adding a Box<dyn Error>

Thank you for the review