You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "saadtajwar (via GitHub)" <gi...@apache.org> on 2026/08/01 01:50:55 UTC
[PR] fix: rows_to_array cleanup for expecting single field [datafusion]
saadtajwar opened a new pull request, #24040:
URL: https://github.com/apache/datafusion/pull/24040
## Which issue does this PR close?
- Closes #23996
## Rationale for this change
Making the `rows_to_array` method in`RowsGroupColumn` behavior of expecting a singular field explicit, as before it used a generic vector operation of `swap_remove(0)` and a `debug_assert_eq!` for ensuring that there was only a single field extracted
## What changes are included in this PR?
Changed the `debug_assert_eq!` to the non-debug macro with a clear error message & changing the `swap_remove(0)` to a `pop` with `unwrap` now that we are guaranteeing the invariant
## Are these changes tested?
Yes
## Are there any user-facing changes?
No
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: rows_to_array cleanup for expecting single field [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #24040:
URL: https://github.com/apache/datafusion/pull/24040#discussion_r3695757201
##########
datafusion/physical-plan/src/aggregates/group_values/multi_group_by/row_backed.rs:
##########
@@ -213,8 +213,13 @@ impl RowsGroupColumn {
.row_converter
.convert_rows(rows)
.expect("row conversion during emit");
- debug_assert_eq!(arrays.len(), 1, "single-field row converter");
- let array = arrays.swap_remove(0);
+ assert_eq!(
+ arrays.len(),
+ 1,
+ "Single field row converter must produce exactly one array, actual length is {}",
Review Comment:
Nit: when the assert fails, it will already print the values, so "actual length is ..." is redundant.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: rows_to_array cleanup for expecting single field [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24040:
URL: https://github.com/apache/datafusion/pull/24040#issuecomment-5149150635
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24040?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `75.00000%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 80.85%. Comparing base ([`f2b4835`](https://app.codecov.io/gh/apache/datafusion/commit/f2b483598b23e1557a0f25097df55191f36a7a41?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`aea4c51`](https://app.codecov.io/gh/apache/datafusion/commit/aea4c51230d63ac4828187005801576db3b87835?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/24040?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...gregates/group\_values/multi\_group\_by/row\_backed.rs](https://app.codecov.io/gh/apache/datafusion/pull/24040?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Faggregates%2Fgroup_values%2Fmulti_group_by%2Frow_backed.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9hZ2dyZWdhdGVzL2dyb3VwX3ZhbHVlcy9tdWx0aV9ncm91cF9ieS9yb3dfYmFja2VkLnJz) | 75.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24040?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #24040 +/- ##
==========================================
- Coverage 80.85% 80.85% -0.01%
==========================================
Files 1101 1101
Lines 375467 375469 +2
Branches 375467 375469 +2
==========================================
- Hits 303596 303585 -11
- Misses 53777 53786 +9
- Partials 18094 18098 +4
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24040?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: rows_to_array cleanup for expecting single field [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway merged PR #24040:
URL: https://github.com/apache/datafusion/pull/24040
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: rows_to_array cleanup for expecting single field [datafusion]
Posted by "saadtajwar (via GitHub)" <gi...@apache.org>.
saadtajwar commented on PR #24040:
URL: https://github.com/apache/datafusion/pull/24040#issuecomment-5148943417
@kosiew ready for review! 😄
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org