ARROW-4749: [Rust] Return Result for RecordBatch::new()

[nevi-me]

Adds more validation between schemas and columns,
returning an error when record types mismatch the schema

[nevi-me]

@andygrove @paddyhoran PTAL when you get a chance

[nevi-me]

@andygrove there's a failure in sql.rs re. mismatching column and batch lengths. If that's not supposed to error, I can remove the validation on RecordBatch::new()

[andygrove]

@nevi-me Nice! You found a bug in aggregate schemas ... I couldn't figure out how to push the fix direct to your branch but here is the new code (for aggregate.rs around line 188):

 let mut output_fields: Vec<Field> = vec![];
                for expr in group_expr {
                    output_fields.push(expr_to_field(expr, input_schema.as_ref()));
                }
                for expr in aggr_expr {
                    output_fields.push(expr_to_field(expr, input_schema.as_ref()));
                }
                let rel = AggregateRelation::new(
                    Arc::new(Schema::new(output_fields)),
                    input_rel,
                    compiled_group_expr,
                    compiled_aggr_expr,
                );

You can also look at the ARROW-4749 branch in my repo (andygrove).

[nevi-me]

I'll fix it manually when I get home. I normally submit a PR on the author's branch

[kszucs]

LGTM, but returning result from new feels a bit odd to me, especially because other types' new return Self.

[sunchao]

LGTM except a few nits. Also as @kszucs mentioned, maybe we can change new to try_new to reflect the fact that it returns a Result now?

[sunchao]

nit: you can replace unwrap() with ? for better error message.

[nevi-me]

The function doesn't return a Result, so I can't ? there. Should I leave the unwrap?

[sunchao]

In main you can actually use ?, see here.

[nevi-me]

If my main has a Result return type? I tried it before commenting, and I got an error about needing to implement TryFrom.

I've got it working with returning Result<()>

[sunchao]

Yes you are right - seems you have to change the main interface and add return Ok(()) from the main as well.. I guess it may not worth it for this purpose. We can just keep as it is.

[nevi-me]

:( I had already changed it, can I keep it the way it is now? If I need to change anything else on the PR I'll revert it back to its original form

[sunchao]

OK. Let's keep as it is.

[sunchao]

nit: not sure ParseError is appropriate here. Do we have other candidate such as InvalidArgumentError?

[nevi-me]

InvalidArgumentError sounds good, I was avoiding adding another error option

[andygrove]

LGTM - agree with the other comments on this PR

[sunchao]

LGTM.