-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
fix: Fix incorrect lazy schema for explode()
in agg()
#19629
Conversation
f97929c
to
7a7eb1e
Compare
explode()
in agg()
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #19629 +/- ##
=======================================
Coverage 79.86% 79.86%
=======================================
Files 1537 1537
Lines 211923 211925 +2
Branches 2446 2446
=======================================
+ Hits 169249 169254 +5
+ Misses 42120 42117 -3
Partials 554 554 ☔ View full report in Codecov by Sentry. |
So the fix boils down to correctly passing the proper |
This comment was marked as outdated.
This comment was marked as outdated.
}, | ||
Explode(expr) => { | ||
let field = arena.get(*expr).to_field_impl(schema, arena, nested)?; | ||
*nested = nested.saturating_sub(1); |
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.
and also Explode
doesn't actually return a scalar so we don't subtract the nested state
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.
Interesting. Does that work? It removes a level of nesting?
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'm not entirely around this nested
variable - but it basically does this (you can see above)
if nested >= 1 {
field.coerce(field.dtype().clone().implode());
}
As I understand it just means we have to set nested
to 0
if we don't need auto-implode. I also suspect the exact value of the nested
value doesn't matter other than this property, but I'm not sure :/
At the beginning it's initialized to 1
if we are Context::Aggregation
and 0
otherwise. So if a result doesn't need an auto-implode (e.g. sum() aggregation), we just set it to 0.
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.
Now that I think of it, I think it does need to subtract if nested >= 2
.
b0867e4
to
6f02ac7
Compare
Fixes #19562
Explode
during dtype resolution mistakenly indicated that it did not need auto-imploding (see thenested
variable in the code)ApplyExpr
Context
recursively