feat(parse): add text::simple_extensions::ArgumentsItem parser

[shanretoo]

Adds a parser for text::simple_extensions::ArgumentsItem that parses the argument.

[CLAassistant]

All committers have signed the CLA.

[mbrobbel]

Thanks for this contribution! This looks great.

It would be really great if we can run tests against the extensions in the substrait submodule.

[shanretoo]

How about adding tests against the extensions in the substrait submodule in the future prs, i.e. after including extensions in this repo like the work you have done in the closed pr: https://github.com/mbrobbel/substrait-rs/blob/aebd6cc53152cda3602decfbd096329e6f4e5850/build.rs#L188-L197
@mbrobbel

[mbrobbel]

How about adding tests against the extensions in the substrait submodule in the future prs, i.e. after including extensions in this repo like the work you have done in the closed pr: https://github.com/mbrobbel/substrait-rs/blob/aebd6cc53152cda3602decfbd096329e6f4e5850/build.rs#L188-L197 @mbrobbel

Sounds good: #186

[shanretoo]

@mbrobbel

I have implemented Parse for those enums and fixed corresponding tests.

But when I try to add checks to ensure the parsing work for core extensions, I encountered a problem:

1. The parse method of Parse trait is using the value receiver of self.

fn parse(self, ctx: &mut C) -> Result<Self::Parsed, Self::Error>;

2. But the ArgumentsItems extracted from EXTENSIONS are references and ArgumentsItem does not implement Copy trait. It is not possible to call parse for them.

error[E0507]: cannot move out of `*item` which is behind a shared reference
   --> src/parse/text/simple_extensions/argument.rs:632:21
    |
632 |                     item.parse(&mut Context::default())?;
    |                     ^^^^ move occurs because `*item` has type `text::simple_extensions::ArgumentsItem`, which does not implement the `Copy` trait

I'm wondering that if we can modify parse method to use the reference receiver.
What's your opinion here?

[mbrobbel]

But the ArgumentsItems extracted from EXTENSIONS are references and ArgumentsItem does not implement Copy trait. It is not possible to call parse for them.

You can clone the SimpleExtensions before parsing them.

I'm wondering that if we can modify parse method to use the reference receiver. What's your opinion here?

We could do this, but it moves cloning (where needed) into the parser impls - unless we also change all parsed items to reference data in the original items. I think the common case would be for users to have owned instances they want to parse, and when they don't, like in your example when you only have a reference, users can explicitly clone. What do you think?

[shanretoo]

Thank you for your detailed explanation. It makes sense to me. This pr is ready for review.

[mbrobbel]

Thanks!
I have some final questions/suggestions - but we can also move some of these to follow-up PRs if you want.

[mbrobbel]

Noticed one typo, but other than that this looks good, thanks again!

[shanretoo]

Really appreciate your review!