You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2024/09/25 11:47:29 UTC
[PR] docs: improve the documentation for Aggregate code [datafusion]
alamb opened a new pull request, #12617:
URL: https://github.com/apache/datafusion/pull/12617
## Which issue does this PR close?
N/A
## Rationale for this change
As part of reviewing the PR for https://github.com/apache/datafusion/pull/12269 from @jayzhan211 I found some
documentation that would make the code easier to understand.
## What changes are included in this PR?
1. Update documentation
## Are these changes tested?
By docs CI
## Are there any user-facing changes?
No functional changes, only docs
--
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] docs: improve the documentation for Aggregate code [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #12617:
URL: https://github.com/apache/datafusion/pull/12617#discussion_r1776419786
##########
datafusion/physical-plan/src/aggregates/group_values/column_wise.rs:
##########
@@ -36,7 +36,7 @@ use datafusion_physical_expr::binary_map::OutputType;
use hashbrown::raw::RawTable;
-/// Compare GroupValue Rows column by column
+/// A [`GroupValues`] that stores multiple columns of group values.
Review Comment:
In Rows implementation, we convert (1, 'a') to row and compare against it. In Column implementation, we compare iteratively from 1 to 'a' in this case.
--
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] docs: improve the documentation for Aggregate code [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #12617:
URL: https://github.com/apache/datafusion/pull/12617#discussion_r1775793749
##########
datafusion/physical-plan/src/aggregates/group_values/group_value_row.rs:
##########
@@ -60,11 +61,13 @@ pub trait ArrayRowEq: Send + Sync {
fn take_n(&mut self, n: usize) -> ArrayRef;
}
+/// An implementation of [`ArrayRowEq`] for primitive types.
pub struct PrimitiveGroupValueBuilder<T: ArrowPrimitiveType> {
group_values: Vec<T::Native>,
nulls: Vec<bool>,
Review Comment:
should be it a BooleanArray? so this null checks will be faster and in 1 place?
--
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] docs: improve the documentation for Aggregate code [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #12617:
URL: https://github.com/apache/datafusion/pull/12617#discussion_r1776115903
##########
datafusion/physical-plan/src/aggregates/group_values/column_wise.rs:
##########
@@ -36,7 +36,7 @@ use datafusion_physical_expr::binary_map::OutputType;
use hashbrown::raw::RawTable;
-/// Compare GroupValue Rows column by column
+/// A [`GroupValues`] that stores multiple columns of group values.
Review Comment:
Consider the following example, (1, 'a') is one of the group values, so is (2, 'b') and (3, 'c')
```
statement ok
create table t(a int, b varchar) as values (1, 'a'), (2, 'b'), (1, 'a'), (3, 'c');
query ITI
select a, b, count(*) from t group by a, b;
----
1 a 2
2 b 1
3 c 1
```
--
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] docs: improve the documentation for Aggregate code [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #12617:
URL: https://github.com/apache/datafusion/pull/12617#discussion_r1776419786
##########
datafusion/physical-plan/src/aggregates/group_values/column_wise.rs:
##########
@@ -36,7 +36,7 @@ use datafusion_physical_expr::binary_map::OutputType;
use hashbrown::raw::RawTable;
-/// Compare GroupValue Rows column by column
+/// A [`GroupValues`] that stores multiple columns of group values.
Review Comment:
In Rows implementation, we convert (1, 'a') to row and compare against it. In Column implementation, we compare iteratively from 1 and 'a' in this case.
--
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] docs: improve the documentation for Aggregate code [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #12617:
URL: https://github.com/apache/datafusion/pull/12617#discussion_r1775797249
##########
datafusion/physical-plan/src/aggregates/group_values/group_value_row.rs:
##########
@@ -60,11 +61,13 @@ pub trait ArrayRowEq: Send + Sync {
fn take_n(&mut self, n: usize) -> ArrayRef;
}
+/// An implementation of [`ArrayRowEq`] for primitive types.
Review Comment:
was it renamed in https://github.com/apache/datafusion/pull/12619?
##########
datafusion/physical-plan/src/aggregates/group_values/group_value_row.rs:
##########
@@ -154,13 +157,14 @@ impl<T: ArrowPrimitiveType> ArrayRowEq for PrimitiveGroupValueBuilder<T> {
}
}
+/// An implementation of [`ArrayRowEq`] for binary and utf8 types.
Review Comment:
renamed?
--
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] docs: improve the documentation for Aggregate code [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #12617:
URL: https://github.com/apache/datafusion/pull/12617#discussion_r1775792354
##########
datafusion/physical-plan/src/aggregates/group_values/column_wise.rs:
##########
@@ -36,7 +36,7 @@ use datafusion_physical_expr::binary_map::OutputType;
use hashbrown::raw::RawTable;
-/// Compare GroupValue Rows column by column
+/// A [`GroupValues`] that stores multiple columns of group values.
Review Comment:
I'm not very familiar with it and thus the question, what is group values?
is it group keys, or exact values attached for specific key?
--
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