You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "crepererum (via GitHub)" <gi...@apache.org> on 2023/05/04 12:32:38 UTC
[GitHub] [arrow-datafusion] crepererum opened a new pull request, #6226: feat: min/max agg for bool
crepererum opened a new pull request, #6226:
URL: https://github.com/apache/arrow-datafusion/pull/6226
# Which issue does this PR close?
\-
# Rationale for this change
Min/max for bools is supported by Rust stdlib, arrow and by `ScalarValue` but wasn't exposed by the min/max aggregator.
# What changes are included in this PR?
Min/max aggregator support for bool.
# Are these changes tested?
Two new tests.
# Are there any user-facing changes?
Min/max aggregation w/ bool should work now.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #6226: feat: min/max agg for bool
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6226:
URL: https://github.com/apache/arrow-datafusion/pull/6226#discussion_r1185122825
##########
datafusion/physical-expr/src/aggregate/min_max.rs:
##########
@@ -1429,4 +1441,38 @@ mod tests {
ScalarValue::Time64Nanosecond(Some(5))
)
}
+
+ #[test]
+ fn max_bool() -> Result<()> {
Review Comment:
I think it would be good to test NULL handling as well:
1. If the inputs is all nulls
3. The input contains nulls and non nulls
Perhaps we could do so with a sqllogictest: https://github.com/apache/arrow-datafusion/blob/main/datafusion/core/tests/sqllogictests/test_files/aggregate.slt
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] crepererum commented on pull request #6226: feat: min/max agg for bool
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum commented on PR #6226:
URL: https://github.com/apache/arrow-datafusion/pull/6226#issuecomment-1549505665
Rebased and added more tests.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #6226: feat: min/max agg for bool
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6226:
URL: https://github.com/apache/arrow-datafusion/pull/6226#issuecomment-1545059848
Marking as a draft to signify it has some known work waiting
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] crepererum commented on pull request #6226: feat: min/max agg for bool
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum commented on PR #6226:
URL: https://github.com/apache/arrow-datafusion/pull/6226#issuecomment-1549737138
```text
thread 'datasource::file_format::avro::tests::read_null_binary_alltypes_plain_avro' panicked at 'called `Result::unwrap()` on an `Err` value: Canonicalize { path: "/__w/arrow-datafusion/arrow-datafusion/datafusion/core/../../testing/data/avro/alltypes_nulls_plain.avro", source: Os { code: 2, kind: NotFound, message: "No such file or directory" } }', datafusion/core/src/test/object_store.rs:48:62
```
What's happening in this CI job?
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] crepererum commented on pull request #6226: feat: min/max agg for bool
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum commented on PR #6226:
URL: https://github.com/apache/arrow-datafusion/pull/6226#issuecomment-1543950481
I haven't forgotten about this PR, will take care of it next week latest.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #6226: feat: min/max agg for bool
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6226:
URL: https://github.com/apache/arrow-datafusion/pull/6226#issuecomment-1550383450
> What's happening in this CI job?
I believe your PR has somehow changed the pin of the `testing` module
If you revert that change this PR should begin passing
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] crepererum commented on pull request #6226: feat: min/max agg for bool
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum commented on PR #6226:
URL: https://github.com/apache/arrow-datafusion/pull/6226#issuecomment-1551107960
`testing` pin fixed, I don't know what GIT originally did during rebase :shrug:
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] crepererum merged pull request #6226: feat: min/max agg for bool
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum merged PR #6226:
URL: https://github.com/apache/arrow-datafusion/pull/6226
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org