You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "jcsherin (via GitHub)" <gi...@apache.org> on 2024/07/12 14:00:42 UTC
[I] Add nullable in `StateFieldArgs` [datafusion]
jcsherin opened a new issue, #11433:
URL: https://github.com/apache/datafusion/issues/11433
I think given the existing nth function, we should let nullable configurable. And, the nullability is actually for the list element. We should add nullable in `StateFieldArgs`.
```rust
let mut fields = vec![Field::new_list(
format_state_name(self.name(), "nth_value"),
Field::new("item", args.input_type.clone(), self.nullable),
false)]
```
@eejbyfeldt is working on it in #11063
_Originally posted by @jayzhan211 in https://github.com/apache/datafusion/pull/11287#discussion_r1667222783_
--
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.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: [I] Add nullable in `StateFieldArgs` [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on issue #11433:
URL: https://github.com/apache/datafusion/issues/11433#issuecomment-2225749277
Sorry, I do not follow. Could you please elaborate on the incorrect part.
--
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: [I] Add nullable in `StateFieldArgs` [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on issue #11433:
URL: https://github.com/apache/datafusion/issues/11433#issuecomment-2241019588
> @jcsherin would you be willing to draft a PR updating the docs?
Yes, I'll be happy to take care of this.
--
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: [I] Add nullable in `StateFieldArgs` [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on issue #11433:
URL: https://github.com/apache/datafusion/issues/11433#issuecomment-2225971515
For `nth_value` aggregation there are 3 possible states:
1. The nth row exists and the value is returned.
2. The nth row is out of range and NULL is returned.
3. No rows qualified for aggregation and NULL is returned.
2 & 3 is covered by the else branch in `Accumulator::evaluate`. The result does not depend on `nullable` of list in the accumulator state.
https://github.com/apache/datafusion/blob/1dfac86a89750193491cf3e04917e37b92c64ffa/datafusion/functions-aggregate/src/nth_value.rs#L337-L342
The `nullable`(Line 141) for the list is rendered no-op. https://github.com/apache/datafusion/blob/1dfac86a89750193491cf3e04917e37b92c64ffa/datafusion/functions-aggregate/src/nth_value.rs#L134-L142
This possibly explains passing tests. Thoughts?
--
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: [I] Add nullable in `StateFieldArgs` [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on issue #11433:
URL: https://github.com/apache/datafusion/issues/11433#issuecomment-2225757052
Like what we have in #11299 . To returns null if row qualified, the `nullable` for list should be `true`, but the current code is false which is not what I thought of.
--
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: [I] Add nullable in `StateFieldArgs` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #11433:
URL: https://github.com/apache/datafusion/issues/11433#issuecomment-2240247560
@jcsherin would you be willing to draft a PR updating the 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: [I] Add nullable in `StateFieldArgs` [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on issue #11433:
URL: https://github.com/apache/datafusion/issues/11433#issuecomment-2225700932
For comparison I ran a few queries from #11299 with `nth_value`. This looks right to me.
```sql
DataFusion CLI v40.0.0
> create table t(a int, b float, c bigint) as values (1, 1.2, 2);
0 row(s) fetched.
> select nth_value(a, 1) from t where a > 2;
+-------------------------+
| nth_value(t.a,Int64(1)) |
+-------------------------+
| |
+-------------------------+
1 row(s) fetched.
> select nth_value(a, 1), count(1) from t where a > 3 group by a;
+-------------------------+-----------------+
| nth_value(t.a,Int64(1)) | count(Int64(1)) |
+-------------------------+-----------------+
+-------------------------+-----------------+
0 row(s) fetched.
> select nth_value(a, 1), count(1) from t where a > 3;
+-------------------------+-----------------+
| nth_value(t.a,Int64(1)) | count(Int64(1)) |
+-------------------------+-----------------+
| | 0 |
+-------------------------+-----------------+
1 row(s) fetched.
> select nth_value(a, 1 order by a desc), count(1) from t where a > 2;
+---------------------------------------------------------+-----------------+
| nth_value(t.a,Int64(1)) ORDER BY [t.a DESC NULLS FIRST] | count(Int64(1)) |
+---------------------------------------------------------+-----------------+
| | 0 |
+---------------------------------------------------------+-----------------+
1 row(s) fetched.
```
--
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: [I] Add nullable in `StateFieldArgs` [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on issue #11433:
URL: https://github.com/apache/datafusion/issues/11433#issuecomment-2226704815
My guess is because the query is not multiple phase aggregate so there is no result that check with `state_field`. Maybe we should figure out some complex query that has state result for each aggregate partition
--
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: [I] Add nullable in `StateFieldArgs` [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on issue #11433:
URL: https://github.com/apache/datafusion/issues/11433#issuecomment-2237184057
I tested this out in many more ways. Each time making sure it's multi-phase grouping. So that state fields are being used. This is the understanding I've reached about the correct `nullable` value for both <ins>the list</ins> and <ins>the items in the list</ins>:
1. It is impossible for the intermediate state of `values` (the list) to ever be null. It can be empty, but never null. So the current setting of `nullable: false` for the list of values is correct.
2. The `nullable` for items in the list need not be configurable(like I initially thought). The computation of nth row does not depend on the `nullable` defined in schema of the input column expression. It should remain set to `true` to allow `null` value to exist as an item in the list. Adding this field to `StateFieldArgs` for the sake of `nth_value` is unnecessary in my opinion.
```rust
let mut fields = vec![Field::new_list(
format_state_name(self.name(), "nth_value"),
Field::new("item", args.input_type.clone(), true), // See [2]
false, // See [1]
)];
```
It will be beneficial to future readers of this code to add some comments here. It took me more time than I think was necessary to reach the conclusions stated above because of how they are now implicit in code.
But other than that the good news is no code changes are required.
--
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: [I] Add nullable in `StateFieldArgs` [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on issue #11433:
URL: https://github.com/apache/datafusion/issues/11433#issuecomment-2253076728
Thanks again @jayzhan211.
--
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: [I] Add nullable in `StateFieldArgs` [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on issue #11433:
URL: https://github.com/apache/datafusion/issues/11433#issuecomment-2225728116
> This comment is also outdated because of #11299.
>
> https://github.com/apache/datafusion/blob/1dfac86a89750193491cf3e04917e37b92c64ffa/datafusion/functions-aggregate/src/nth_value.rs#L136-L139
Oh, so actually the current code is incorrect 😕 I though that is the one you proposed to change. But why the result is expected 🤔
--
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: [I] Add nullable in `StateFieldArgs` [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on issue #11433:
URL: https://github.com/apache/datafusion/issues/11433#issuecomment-2225655947
Hold on, I think it is outdated, since I found that most of the sql database returns null if no row qualified. #11299
--
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: [I] Add nullable in `StateFieldArgs` [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on issue #11433:
URL: https://github.com/apache/datafusion/issues/11433#issuecomment-2229187055
This is a recent change introduce by #11093.
https://github.com/apache/datafusion/blob/0965455486b7dcbd8c9a5efa8d2370ca5460bb9f/datafusion/functions-aggregate/src/nth_value.rs#L404-L407
In `state_fields` the nullability is set to `false`. But here when `Accumulator::evaluate()` is called the nullability of the list is overridden to always be `true`.
It is surprising to me that the schema defined in `state_fields` doesn't match the Accumulator and still this works.
--
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: [I] Add nullable in `StateFieldArgs` [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on issue #11433:
URL: https://github.com/apache/datafusion/issues/11433#issuecomment-2229152203
When adding trace statements in `nth_value` aggregate I can see that the following are executed in order:
- `update_batch()`
- `state()`
- `merge_batch()`
- `evaluate()`
1. __nth_value always exists__ - In the query below the 2nd item in `C13` is always present because for all groups `a..=e` the count in each group is either 18, 19 or 21.
```sql
SELECT C1
, COUNT(C1) as n
, NTH_VALUE(C13, 2 ORDER BY C1, C13 ASC) as nth -- get 2nd row
FROM aggregate_test_100
GROUP BY C1
ORDER BY C1;
+----+----+--------------------------------+
| c1 | n | nth |
+----+----+--------------------------------+
| a | 21 | Amn2K87Db5Es3dFQO9cw9cvpAM6h35 |
| b | 19 | 6FPJlLAcaQ5uokyOWZ9HGdLZObFvOZ |
| c | 21 | 6WfVFBVGJSQb7FhA7E0lBwdvjfZnSW |
| d | 18 | 1aOcrEGd0cOqZe2I5XBOm0nDcwtBZO |
| e | 21 | 3BEOHQsMEFZ58VcNTOJYShTBpAPzbt |
+----+----+--------------------------------+
5 row(s) fetched.
```
2. __nth_value is sometime out of bounds__ - Since 'b' and 'd' groups have only 18 and 19 values, we can set `N` to be 20.
```sql
SELECT C1
, COUNT(C1) as n
, NTH_VALUE(C13, 20 ORDER BY C1, C13 ASC) as nth -- get 20th row
FROM aggregate_test_100
GROUP BY C1
ORDER BY C1;
+----+----+--------------------------------+
| c1 | n | nth |
+----+----+--------------------------------+
| a | 21 | waIGbOGl1PM6gnzZ4uuZt4E2yDWRHs |
| b | 19 | |
| c | 21 | pLk3i59bZwd5KBZrI1FiweYTd5hteG |
| d | 18 | |
| e | 21 | ukOiFGGFnQJDHFgZxHMpvhD3zybF0M |
+----+----+--------------------------------+
5 row(s) fetched.
```
---
### Create Table
```sql
CREATE EXTERNAL TABLE aggregate_test_100 (
c1 VARCHAR NOT NULL,
c2 TINYINT NOT NULL,
c3 SMALLINT NOT NULL,
c4 SMALLINT,
c5 INT,
c6 BIGINT NOT NULL,
c7 SMALLINT NOT NULL,
c8 INT NOT NULL,
c9 BIGINT UNSIGNED NOT NULL,
c10 VARCHAR NOT NULL,
c11 FLOAT NOT NULL,
c12 DOUBLE NOT NULL,
c13 VARCHAR NOT NULL
)
STORED AS CSV
-- path is relative to `datafusion-cli`
LOCATION '../testing/data/csv/aggregate_test_100.csv'
OPTIONS ('format.has_header' 'true');
```
--
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: [I] Add nullable in `StateFieldArgs` [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on issue #11433:
URL: https://github.com/apache/datafusion/issues/11433#issuecomment-2225706313
This comment is also outdated because of #11299.
https://github.com/apache/datafusion/blob/1dfac86a89750193491cf3e04917e37b92c64ffa/datafusion/functions-aggregate/src/nth_value.rs#L136-L139
--
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: [I] Add nullable in `StateFieldArgs` [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on issue #11433:
URL: https://github.com/apache/datafusion/issues/11433#issuecomment-2229650572
> This is a recent change introduced in #11093.
>
> https://github.com/apache/datafusion/blob/0965455486b7dcbd8c9a5efa8d2370ca5460bb9f/datafusion/functions-aggregate/src/nth_value.rs#L404-L407
>
> In `state_fields` the nullability is set to `false`. But inside `Accumulator::evaluate()` the nullability of the list is overridden to always be `true`.
>
> It is surprising to me that the schema defined in `state_fields` doesn't match the Accumulator and still this works.
Maybe the schema matching is not correct
--
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: [I] Add nullable in `StateFieldArgs` [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on issue #11433:
URL: https://github.com/apache/datafusion/issues/11433#issuecomment-2238576514
cc @eejbyfeldt and @alamb I think @jcsherin find something interesting that nullable for list element should be `true` in order to allow computing with null element. I think the idea here applies to array_agg too. We probably could set nullable to `false` for list in `state_fields`
```rust
fn state_fields(&self, args: StateFieldsArgs) -> Result<Vec<Field>> {
if args.is_distinct {
return Ok(vec![Field::new_list(
format_state_name(args.name, "distinct_array_agg"),
Field::new("item", args.input_type.clone(), true),
false,
)]);
}
Ok(vec![Field::new_list(
format_state_name(args.name, "array_agg"),
Field::new("item", args.input_type.clone(), true),
false,
)])
}
```
I think we should definitely add docs about this, we had several PRs dealing with nullability already. It would be helpful for others too.
--
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: [I] Add nullable in `StateFieldArgs` [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin closed issue #11433: Add nullable in `StateFieldArgs`
URL: https://github.com/apache/datafusion/issues/11433
--
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: [I] Add nullable in `StateFieldArgs` [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on issue #11433:
URL: https://github.com/apache/datafusion/issues/11433#issuecomment-2225720653
This one looks good to me too. But, it is nice to have a case that benefit on the nullability of the element (like the query that is optimized based on this)
```rust
let mut fields = vec![Field::new_list(
format_state_name(self.name(), "nth_value"),
Field::new("item", args.input_type.clone(), self.nullable),
true)]
```
Otherwise for simplicity, we can assume it is nullable
```rust
let mut fields = vec![Field::new_list(
format_state_name(self.name(), "nth_value"),
Field::new("item", args.input_type.clone(), true),
true)]
```
--
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