kadai0308 commented on PR #16068:
URL: https://github.com/apache/datafusion/pull/16068#issuecomment-2888172345
> Thank you for this PR @kadai0308 -- very helpful
>
> I am not sure about the implications of `Box`ing all the variants -- I
worry it will just add additional overhead for planning and API churn (though
I'll run some benchmarks0
>
> Rather than adding new `Box` would you be willing instead to add in the
`#[clippy(allow(..))` annotations instead for errors about mismatched enum
sizes?
>
> I think this would allow us to unblock the upgrade to 1.87 and we can then
consider how to fix the enum variant size mismatch as a follow on
>
> For example, one thing I would like to consider is making `Expr` smaller,
for example, this PR: #14366
Thanks for the feedback! Yeah, I agree — adding Box to some enum variants
and the associated changes could make the codebase and API more unstable and
fragile.
I just update the PR with `#[allow(clippy::large_enum_variant)]`.
Another notable change is related to this warning:
```
941 | fn resolve_u8(v: &Value) -> AvroResult<u8> {
| ^^^^^^^^^^^^^^ the `Err`-variant is at
least 256 bytes
```
It seems we could return Option<u8> instead of AvroResult<u8>, because
```
Value::Array(items) => Ok(Value::Bytes(
items
.iter()
.map(resolve_u8)
.collect::<Result<Vec<_>, _>>()
.ok()?,
)),
```
If I understand correctly, .ok()? effectively drops the error from
AvroResult, making Option<u8> sufficient in this context.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
