-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add tests for filtering, grouping, aggregation of ARRAYs #9695
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| # Filtering | ||
| ########### | ||
|
|
||
| query error DataFusion error: Arrow error: Invalid argument error: Invalid comparison operation: List\(Field \{ name: "item", data_type: Int64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: \{\} \}\) == List\(Field \{ name: "item", data_type: Int64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: \{\} \}\) |
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.
I was actually surprised I couldn't write a basic filter for ARRAYs -- do you know if this is tracked by any ticket @jayzhan211 ?
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.
No, there is no filed issue with filter. We can open one, it seems like an interesting feature. I think we can utilize logical plan::filter.
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.
No, there is no filed issue with
filter. We can open one, it seems like an interesting feature. I think we can utilize logical plan::filter.
I was confused with array_filter, maybe we just need to support array types for filterExec 🤔
| # Aggregates | ||
| ########### | ||
|
|
||
| query error Internal error: Min/Max accumulator not implemented for type List |
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.
We should probably make this at least a nicer error ("Not supported" rather than "internal error")
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.
I agree
|
@jayzhan211 I wonder if you would have a few minutes to review this PR sometime? |
jayzhan211
left a comment
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.
👍
* Add tests for filtering, grouping, aggregation of ARRAYs * Update output to correct results
Which issue does this PR close?
Related to #9586
Rationale for this change
While working on #9679 I realized that the amount of coverage for
ARRAYs other than the (very extensive) coverage for functions was pretty sparseWhat changes are included in this PR?
Add a new
array_query.sltfile that has basic query patterns for filtering, aggregation, grouping and orderingAre these changes tested?
it is only tests
Are there any user-facing changes?
no