You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "gruuya (via GitHub)" <gi...@apache.org> on 2024/04/04 10:52:59 UTC
[PR] Change the sentinel value's type for COUNT(*) to Int64 [arrow-datafusion]
gruuya opened a new pull request, #9944:
URL: https://github.com/apache/arrow-datafusion/pull/9944
## Which issue does this PR close?
Closes #9943.
## Rationale for this change
Make `COUNT(*)` equivalent to `COUNT(1)`.
## What changes are included in this PR?
Change the type of `COUNT_STAR_EXPANSION` to `ScalarValue::Int64`.
## Are these changes tested?
Yes, with revised existing tests and a new one that tests the optimization for `count(1)` case.
## Are there any user-facing changes?
--
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
Re: [PR] Change the sentinel value's type for COUNT(*) to Int64 [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9944:
URL: https://github.com/apache/arrow-datafusion/pull/9944#discussion_r1552035289
##########
datafusion/expr/src/utils.rs:
##########
@@ -42,7 +42,7 @@ use sqlparser::ast::{ExceptSelectItem, ExcludeSelectItem, WildcardAdditionalOpti
/// The value to which `COUNT(*)` is expanded to in
/// `COUNT(<constant>)` expressions
-pub const COUNT_STAR_EXPANSION: ScalarValue = ScalarValue::UInt8(Some(1));
+pub const COUNT_STAR_EXPANSION: ScalarValue = ScalarValue::Int64(Some(1));
Review Comment:
> This does however result in count(1) being auto-named to count(*) now.
FWIW, this is what happens on main with `count(uint8)` (the same thing)
```sql
❯ select count(arrow_cast(1, 'UInt8')) from (values (1));
+----------+
| COUNT(*) |
+----------+
| 1 |
+----------+
1 row(s) fetched.
Elapsed 0.002 seconds.
```
Though admittedly almost no one would actually type `count(arrow_cast(1, 'UInt8'))` so it is likely not a big deal.
> That might be fine as it reduces ambiguity.
I agree having the `COUNT(*)` in the plan actually helps as it makes it clearer when the fast path is being used.
Let's start with this and if someone else has a different opinion we can make another PR
--
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
Re: [PR] Optimize `COUNT(1)`: Change the sentinel value's type for COUNT(*) to Int64 [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #9944:
URL: https://github.com/apache/arrow-datafusion/pull/9944
--
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
Re: [PR] Change the sentinel value's type for COUNT(*) to Int64 [arrow-datafusion]
Posted by "gruuya (via GitHub)" <gi...@apache.org>.
gruuya commented on code in PR #9944:
URL: https://github.com/apache/arrow-datafusion/pull/9944#discussion_r1551563337
##########
datafusion/expr/src/utils.rs:
##########
@@ -42,7 +42,7 @@ use sqlparser::ast::{ExceptSelectItem, ExcludeSelectItem, WildcardAdditionalOpti
/// The value to which `COUNT(*)` is expanded to in
/// `COUNT(<constant>)` expressions
-pub const COUNT_STAR_EXPANSION: ScalarValue = ScalarValue::UInt8(Some(1));
+pub const COUNT_STAR_EXPANSION: ScalarValue = ScalarValue::Int64(Some(1));
Review Comment:
This does however result in `count(1)` being auto-named to `count(*)` now.
```bash
❯ explain select count(1) from hits;
+---------------+---------------------------------------------------+
| plan_type | plan |
+---------------+---------------------------------------------------+
| logical_plan | Aggregate: groupBy=[[]], aggr=[[COUNT(Int64(1))]] |
| | TableScan: hits projection=[] |
| physical_plan | ProjectionExec: expr=[99997497 as COUNT(*)] |
| | PlaceholderRowExec |
| | |
+---------------+---------------------------------------------------+
2 row(s) fetched.
Elapsed 0.263 seconds.
❯ select count(1) from hits;
+----------+
| COUNT(*) |
+----------+
| 99997497 |
+----------+
1 row(s) fetched.
Elapsed 0.015 seconds.
```
That might be fine as it reduces ambiguity.
Otherwise it could be resolved by e.g.:
```diff
diff --git a/datafusion/core/src/physical_optimizer/aggregate_statistics.rs b/datafusion/core/src/physical_optimizer/aggregate_statistics.rs
index df5422227..0e35b6a2b 100644
--- a/datafusion/core/src/physical_optimizer/aggregate_statistics.rs
+++ b/datafusion/core/src/physical_optimizer/aggregate_statistics.rs
@@ -160,6 +160,11 @@ fn take_optimizable_table_count(
ScalarValue::Int64(Some(num_rows as i64)),
COUNT_STAR_NAME,
));
+ } else if lit_expr.value() == &ScalarValue::Int64(Some(1)) {
+ return Some((
+ ScalarValue::Int64(Some(num_rows as i64)),
+ "COUNT(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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize `COUNT(1)`: Change the sentinel value's type for COUNT(*) to Int64 [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9944:
URL: https://github.com/apache/arrow-datafusion/pull/9944#issuecomment-2039536374
Thanks again @gruuya
--
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
Re: [PR] Change the sentinel value's type for COUNT(*) to Int64 [arrow-datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #9944:
URL: https://github.com/apache/arrow-datafusion/pull/9944#issuecomment-2036895674
# Benchmark results
<details>
<summary>Benchmarks comparing 4bd7c137e0e205140e273a7c25824c94b457c660 (main) and dbc50200beaea15b3245704d21a87021ed92663d (PR)</summary>
```
Comparing 4bd7c13 and dbc5020
--------------------
Benchmark tpch_mem_sf1.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━┳━━━━━━━━━━┳━━━━━━━━━━━━━━━┓
┃ Query ┃ 4bd7c13 ┃ dbc5020 ┃ Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━╇━━━━━━━━━━╇━━━━━━━━━━━━━━━┩
│ QQuery 1 │ 291.81ms │ 293.02ms │ no change │
│ QQuery 2 │ 39.03ms │ 40.57ms │ no change │
│ QQuery 3 │ 58.57ms │ 59.64ms │ no change │
│ QQuery 4 │ 103.21ms │ 79.51ms │ +1.30x faster │
│ QQuery 5 │ 130.88ms │ 99.08ms │ +1.32x faster │
│ QQuery 6 │ 16.41ms │ 16.31ms │ no change │
│ QQuery 7 │ 233.86ms │ 231.76ms │ no change │
│ QQuery 8 │ 43.20ms │ 44.62ms │ no change │
│ QQuery 9 │ 126.29ms │ 118.55ms │ +1.07x faster │
│ QQuery 10 │ 109.28ms │ 111.10ms │ no change │
│ QQuery 11 │ 46.48ms │ 45.68ms │ no change │
│ QQuery 12 │ 59.06ms │ 59.39ms │ no change │
│ QQuery 13 │ 107.43ms │ 106.03ms │ no change │
│ QQuery 14 │ 18.85ms │ 18.99ms │ no change │
│ QQuery 15 │ 31.99ms │ 31.77ms │ no change │
│ QQuery 16 │ 46.67ms │ 47.47ms │ no change │
│ QQuery 17 │ 139.18ms │ 139.44ms │ no change │
│ QQuery 18 │ 512.08ms │ 590.62ms │ 1.15x slower │
│ QQuery 19 │ 64.66ms │ 63.61ms │ no change │
│ QQuery 20 │ 121.75ms │ 119.54ms │ no change │
│ QQuery 21 │ 332.45ms │ 340.13ms │ no change │
│ QQuery 22 │ 39.49ms │ 39.01ms │ no change │
└──────────────┴──────────┴──────────┴───────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━┓
┃ Benchmark Summary ┃ ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━┩
│ Total Time (4bd7c13) │ 2672.63ms │
│ Total Time (dbc5020) │ 2695.85ms │
│ Average Time (4bd7c13) │ 121.48ms │
│ Average Time (dbc5020) │ 122.54ms │
│ Queries Faster │ 3 │
│ Queries Slower │ 1 │
│ Queries with No Change │ 18 │
└────────────────────────┴───────────┘
--------------------
Benchmark tpch_sf1.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━┳━━━━━━━━━━┳━━━━━━━━━━━━━━┓
┃ Query ┃ 4bd7c13 ┃ dbc5020 ┃ Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━╇━━━━━━━━━━╇━━━━━━━━━━━━━━┩
│ QQuery 1 │ 434.65ms │ 438.47ms │ no change │
│ QQuery 2 │ 55.96ms │ 56.92ms │ no change │
│ QQuery 3 │ 143.14ms │ 146.84ms │ no change │
│ QQuery 4 │ 86.33ms │ 95.97ms │ 1.11x slower │
│ QQuery 5 │ 197.43ms │ 202.88ms │ no change │
│ QQuery 6 │ 107.99ms │ 108.68ms │ no change │
│ QQuery 7 │ 274.78ms │ 293.95ms │ 1.07x slower │
│ QQuery 8 │ 190.78ms │ 195.93ms │ no change │
│ QQuery 9 │ 288.65ms │ 300.95ms │ no change │
│ QQuery 10 │ 231.57ms │ 234.50ms │ no change │
│ QQuery 11 │ 62.39ms │ 62.23ms │ no change │
│ QQuery 12 │ 124.30ms │ 125.27ms │ no change │
│ QQuery 13 │ 172.85ms │ 177.04ms │ no change │
│ QQuery 14 │ 128.32ms │ 126.50ms │ no change │
│ QQuery 15 │ 190.29ms │ 195.11ms │ no change │
│ QQuery 16 │ 51.30ms │ 50.32ms │ no change │
│ QQuery 17 │ 307.81ms │ 298.61ms │ no change │
│ QQuery 18 │ 442.69ms │ 443.12ms │ no change │
│ QQuery 19 │ 230.46ms │ 229.31ms │ no change │
│ QQuery 20 │ 189.53ms │ 191.92ms │ no change │
│ QQuery 21 │ 321.44ms │ 328.69ms │ no change │
│ QQuery 22 │ 53.32ms │ 51.77ms │ no change │
└──────────────┴──────────┴──────────┴──────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━┓
┃ Benchmark Summary ┃ ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━┩
│ Total Time (4bd7c13) │ 4285.97ms │
│ Total Time (dbc5020) │ 4354.99ms │
│ Average Time (4bd7c13) │ 194.82ms │
│ Average Time (dbc5020) │ 197.95ms │
│ Queries Faster │ 0 │
│ Queries Slower │ 2 │
│ Queries with No Change │ 20 │
└────────────────────────┴───────────┘
--------------------
Benchmark tpch_sf10.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━━┳━━━━━━━━━━━┳━━━━━━━━━━━┓
┃ Query ┃ 4bd7c13 ┃ dbc5020 ┃ Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━━╇━━━━━━━━━━━╇━━━━━━━━━━━┩
│ QQuery 1 │ 4249.17ms │ 4276.34ms │ no change │
│ QQuery 2 │ 470.59ms │ 485.87ms │ no change │
│ QQuery 3 │ 1718.23ms │ 1705.55ms │ no change │
│ QQuery 4 │ 824.64ms │ 828.50ms │ no change │
│ QQuery 5 │ 2155.19ms │ 2168.08ms │ no change │
│ QQuery 6 │ 1037.84ms │ 1037.32ms │ no change │
│ QQuery 7 │ 3553.98ms │ 3693.11ms │ no change │
│ QQuery 8 │ 2402.73ms │ 2441.64ms │ no change │
│ QQuery 9 │ 4135.17ms │ 4112.04ms │ no change │
│ QQuery 10 │ 2548.32ms │ 2545.29ms │ no change │
│ QQuery 11 │ 570.79ms │ 585.43ms │ no change │
│ QQuery 12 │ 1185.51ms │ 1187.38ms │ no change │
│ QQuery 13 │ 2316.59ms │ 2316.11ms │ no change │
│ QQuery 14 │ 1288.98ms │ 1288.92ms │ no change │
│ QQuery 15 │ 1963.60ms │ 1952.05ms │ no change │
│ QQuery 16 │ 516.67ms │ 516.08ms │ no change │
│ QQuery 17 │ 5240.30ms │ 5188.41ms │ no change │
│ QQuery 18 │ 6835.60ms │ 6946.31ms │ no change │
│ QQuery 19 │ 2234.32ms │ 2240.05ms │ no change │
│ QQuery 20 │ 2556.45ms │ 2588.11ms │ no change │
│ QQuery 21 │ 4333.60ms │ 4301.07ms │ no change │
│ QQuery 22 │ 574.15ms │ 553.15ms │ no change │
└──────────────┴───────────┴───────────┴───────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━┓
┃ Benchmark Summary ┃ ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━┩
│ Total Time (4bd7c13) │ 52712.46ms │
│ Total Time (dbc5020) │ 52956.79ms │
│ Average Time (4bd7c13) │ 2396.02ms │
│ Average Time (dbc5020) │ 2407.13ms │
│ Queries Faster │ 0 │
│ Queries Slower │ 0 │
│ Queries with No Change │ 22 │
└────────────────────────┴────────────┘
```
</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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change the sentinel value's type for COUNT(*) to Int64 [arrow-datafusion]
Posted by "gruuya (via GitHub)" <gi...@apache.org>.
gruuya commented on PR #9944:
URL: https://github.com/apache/arrow-datafusion/pull/9944#issuecomment-2036842516
Might as well try whether /benchmark works as intended 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
Re: [PR] Change the sentinel value's type for COUNT(*) to Int64 [arrow-datafusion]
Posted by "gruuya (via GitHub)" <gi...@apache.org>.
gruuya commented on code in PR #9944:
URL: https://github.com/apache/arrow-datafusion/pull/9944#discussion_r1551563337
##########
datafusion/expr/src/utils.rs:
##########
@@ -42,7 +42,7 @@ use sqlparser::ast::{ExceptSelectItem, ExcludeSelectItem, WildcardAdditionalOpti
/// The value to which `COUNT(*)` is expanded to in
/// `COUNT(<constant>)` expressions
-pub const COUNT_STAR_EXPANSION: ScalarValue = ScalarValue::UInt8(Some(1));
+pub const COUNT_STAR_EXPANSION: ScalarValue = ScalarValue::Int64(Some(1));
Review Comment:
This does however result in `count(1)` being auto-named to `count(*)` now.
```bash
❯ explain select count(1) from hits;
+---------------+---------------------------------------------------+
| plan_type | plan |
+---------------+---------------------------------------------------+
| logical_plan | Aggregate: groupBy=[[]], aggr=[[COUNT(Int64(1))]] |
| | TableScan: hits projection=[] |
| physical_plan | ProjectionExec: expr=[99997497 as COUNT(*)] |
| | PlaceholderRowExec |
| | |
+---------------+---------------------------------------------------+
2 row(s) fetched.
Elapsed 0.263 seconds.
❯ select count(1) from hits;
+----------+
| COUNT(*) |
+----------+
| 99997497 |
+----------+
1 row(s) fetched.
Elapsed 0.015 seconds.
```
On the other hand that might be fine as it reduces ambiguity.
--
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