-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Fix ArrayAgg schema mismatch issue #8055
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @jayzhan211 -- it would be nice if there was some way to avoid having to plumb the is_expr_nullable
flag through, but I don't think there is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @jayzhan211 for the contribution and working on this area of the code
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again @jayzhan211
Which issue does this PR close?
Closes #8032.
Rationale for this change
The main issue for #8032 is as what the errors said we have different schema for result.schema which is
logical-expr schema
and results.collect()[0].schema which isphysical-expr schema
.We should have the same
is_nullable
for both logical-expr and physical-exprWe can see that the logical-expr of List is set with expr.nullable(schema)
https://github.com/apache/arrow-datafusion/blob/3469c4e09a3d32381949dd0c0f626f406c00c6ad/datafusion/expr/src/expr_schema.rs#L289-L305
and logical-expr of List element is set with the return type of Aggregatefunciton::ArrayAgg
https://github.com/apache/arrow-datafusion/blob/3469c4e09a3d32381949dd0c0f626f406c00c6ad/datafusion/expr/src/aggregate_function.rs#L286-L290
What changes are included in this PR?
Fix schema
is_nullable
Are these changes tested?
I did not test end to end like #8032 , just make sure the schema is the same.
Are there any user-facing changes?