You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "msirek (via GitHub)" <gi...@apache.org> on 2023/11/04 06:34:00 UTC
[PR] Fix incorrect results in COUNT(*) queries with LIMIT [arrow-datafusion]
msirek opened a new pull request, #8049:
URL: https://github.com/apache/arrow-datafusion/pull/8049
## Which issue does this PR close?
Closes #8048.
## Rationale for this change
While testing #8038, I ran into some incorrect results cases in `COUNT(*)` queries
from a `LIMIT`ed relation, related to [exact statistics](https://github.com/apache/arrow-datafusion/pull/7793).
The issue is due to use of the `fetch` value plus the `skip` value in
stats for `GlobalLimitExec` as the output stats `num_rows`, plus use of `Exact`
statistics for the output in cases where the input has `Inexact` statistics.
## What changes are included in this PR?
#### Fix incorrect results in COUNT(*) queries with LIMIT
This commit reworks the cases in `GlobalLimitExec::statistics` to cap output
stats `num_rows` at the `fetch` value instead of the `fetch+skip` value.
Also, the following cases are modified:
- Output stats are copied from input stats when # of input rows is less than fetch rows, and `skip` is 0
- if (# of input rows - skip) <= fetch, output `num_rows` = input `num_rows` - `skip`
- if input stats are `Inexact` or `Absent`, output stats are `Inexact`
- if (# of input rows - skip) > usize::MAX and `fetch` value is `None`, output stats are `Inexact`
## Are these changes tested?
- [x] unit tests for `GlobalLimitExec` statistics, both `Exact` and `Inexact`.
- [x] sqllogictests for `GlobalLimitExec` statistics
## 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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix incorrect results in COUNT(*) queries with LIMIT [arrow-datafusion]
Posted by "msirek (via GitHub)" <gi...@apache.org>.
msirek commented on PR #8049:
URL: https://github.com/apache/arrow-datafusion/pull/8049#issuecomment-1795080770
@berkaysynnada Does this change look OK to you?
--
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] Fix incorrect results in COUNT(*) queries with LIMIT [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8049:
URL: https://github.com/apache/arrow-datafusion/pull/8049
--
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] Fix incorrect results in COUNT(*) queries with LIMIT [arrow-datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on code in PR #8049:
URL: https://github.com/apache/arrow-datafusion/pull/8049#discussion_r1383816071
##########
datafusion/sqllogictest/test_files/limit.slt:
##########
@@ -294,6 +294,91 @@ query T
SELECT c1 FROM aggregate_test_100 LIMIT 1 OFFSET 101
----
+#
+# global limit statistics test
+#
+
+statement ok
+CREATE TABLE IF NOT EXISTS t1 (a INT) AS VALUES(1),(2),(3),(4),(5),(6),(7),(8),(9),(10);
+
+# The aggregate does not need to be computed because the input statistics are exact and
+# the number of rows is less than the skip value (OFFSET).
+query TT
+EXPLAIN SELECT COUNT(*) FROM (SELECT a FROM t1 LIMIT 3 OFFSET 11);
+----
+logical_plan
+Aggregate: groupBy=[[]], aggr=[[COUNT(UInt8(1)) AS COUNT(*)]]
+--Limit: skip=11, fetch=3
+----TableScan: t1 projection=[], fetch=14
+physical_plan
+ProjectionExec: expr=[0 as COUNT(*)]
+--EmptyExec: produce_one_row=true
+
+query I
+SELECT COUNT(*) FROM (SELECT a FROM t1 LIMIT 3 OFFSET 11);
+----
+0
+
+# The aggregate does not need to be computed because the input statistics are exact and
+# the number of rows is less than or equal to the the "fetch+skip" value (LIMIT+OFFSET).
+query TT
+EXPLAIN SELECT COUNT(*) FROM (SELECT a FROM t1 LIMIT 3 OFFSET 8);
+----
+logical_plan
+Aggregate: groupBy=[[]], aggr=[[COUNT(UInt8(1)) AS COUNT(*)]]
+--Limit: skip=8, fetch=3
+----TableScan: t1 projection=[], fetch=11
+physical_plan
+ProjectionExec: expr=[2 as COUNT(*)]
+--EmptyExec: produce_one_row=true
+
+query I
+SELECT COUNT(*) FROM (SELECT a FROM t1 LIMIT 3 OFFSET 8);
+----
+2
+
+# The aggregate does not need to be computed because the input statistics are exact and
+# an OFFSET, but no LIMIT, is specified.
+query TT
+EXPLAIN SELECT COUNT(*) FROM (SELECT a FROM t1 OFFSET 8);
+----
+logical_plan
+Aggregate: groupBy=[[]], aggr=[[COUNT(UInt8(1)) AS COUNT(*)]]
+--Limit: skip=8, fetch=None
+----TableScan: t1 projection=[]
+physical_plan
+ProjectionExec: expr=[2 as COUNT(*)]
+--EmptyExec: produce_one_row=true
+
+query I
+SELECT COUNT(*) FROM (SELECT a FROM t1 LIMIT 3 OFFSET 8);
+----
+2
+
+# The aggregate needs to be computed because the input statistics are inexact.
+query TT
+EXPLAIN SELECT COUNT(*) FROM (SELECT a FROM t1 WHERE a > 3 LIMIT 3 OFFSET 6);
+----
+logical_plan
+Aggregate: groupBy=[[]], aggr=[[COUNT(UInt8(1)) AS COUNT(*)]]
+--Limit: skip=6, fetch=3
+----Filter: t1.a > Int32(3)
+------TableScan: t1 projection=[a]
+physical_plan
+AggregateExec: mode=Final, gby=[], aggr=[COUNT(*)]
+--CoalescePartitionsExec
+----AggregateExec: mode=Partial, gby=[], aggr=[COUNT(*)]
+------RepartitionExec: partitioning=RoundRobinBatch(4), input_partitions=1
+--------GlobalLimitExec: skip=6, fetch=3
+----------CoalesceBatchesExec: target_batch_size=8192
+------------FilterExec: a@0 > 3
+--------------MemoryExec: partitions=1, partition_sizes=[1]
+
+query I
+SELECT COUNT(*) FROM (SELECT a FROM t1 WHERE a > 3 LIMIT 3 OFFSET 6);
+----
+1
+
Review Comment:
👍
##########
datafusion/physical-plan/src/limit.rs:
##########
@@ -188,21 +188,11 @@ impl ExecutionPlan for GlobalLimitExec {
fn statistics(&self) -> Result<Statistics> {
Review Comment:
Thanks for noticing the error and fixing it. I think the new logic here is quite reasonable.
##########
datafusion/sqllogictest/test_files/explain.slt:
##########
@@ -273,7 +273,7 @@ query TT
EXPLAIN SELECT a, b, c FROM simple_explain_test limit 10;
----
physical_plan
-GlobalLimitExec: skip=0, fetch=10, statistics=[Rows=Exact(10), Bytes=Absent]
+GlobalLimitExec: skip=0, fetch=10, statistics=[Rows=Inexact(10), Bytes=Absent]
Review Comment:
The old version is definitely misleading, nice improvement 👍
##########
datafusion/sqllogictest/test_files/limit.slt:
##########
@@ -294,6 +294,91 @@ query T
SELECT c1 FROM aggregate_test_100 LIMIT 1 OFFSET 101
----
+#
+# global limit statistics test
+#
+
+statement ok
+CREATE TABLE IF NOT EXISTS t1 (a INT) AS VALUES(1),(2),(3),(4),(5),(6),(7),(8),(9),(10);
+
+# The aggregate does not need to be computed because the input statistics are exact and
+# the number of rows is less than the skip value (OFFSET).
+query TT
+EXPLAIN SELECT COUNT(*) FROM (SELECT a FROM t1 LIMIT 3 OFFSET 11);
+----
+logical_plan
+Aggregate: groupBy=[[]], aggr=[[COUNT(UInt8(1)) AS COUNT(*)]]
+--Limit: skip=11, fetch=3
+----TableScan: t1 projection=[], fetch=14
+physical_plan
+ProjectionExec: expr=[0 as COUNT(*)]
+--EmptyExec: produce_one_row=true
+
+query I
+SELECT COUNT(*) FROM (SELECT a FROM t1 LIMIT 3 OFFSET 11);
+----
+0
+
+# The aggregate does not need to be computed because the input statistics are exact and
+# the number of rows is less than or equal to the the "fetch+skip" value (LIMIT+OFFSET).
+query TT
+EXPLAIN SELECT COUNT(*) FROM (SELECT a FROM t1 LIMIT 3 OFFSET 8);
+----
+logical_plan
+Aggregate: groupBy=[[]], aggr=[[COUNT(UInt8(1)) AS COUNT(*)]]
+--Limit: skip=8, fetch=3
+----TableScan: t1 projection=[], fetch=11
+physical_plan
+ProjectionExec: expr=[2 as COUNT(*)]
+--EmptyExec: produce_one_row=true
+
+query I
+SELECT COUNT(*) FROM (SELECT a FROM t1 LIMIT 3 OFFSET 8);
+----
+2
+
+# The aggregate does not need to be computed because the input statistics are exact and
+# an OFFSET, but no LIMIT, is specified.
+query TT
+EXPLAIN SELECT COUNT(*) FROM (SELECT a FROM t1 OFFSET 8);
+----
+logical_plan
+Aggregate: groupBy=[[]], aggr=[[COUNT(UInt8(1)) AS COUNT(*)]]
+--Limit: skip=8, fetch=None
+----TableScan: t1 projection=[]
+physical_plan
+ProjectionExec: expr=[2 as COUNT(*)]
+--EmptyExec: produce_one_row=true
+
+query I
+SELECT COUNT(*) FROM (SELECT a FROM t1 LIMIT 3 OFFSET 8);
+----
+2
+
+# The aggregate needs to be computed because the input statistics are inexact.
+query TT
+EXPLAIN SELECT COUNT(*) FROM (SELECT a FROM t1 WHERE a > 3 LIMIT 3 OFFSET 6);
+----
+logical_plan
+Aggregate: groupBy=[[]], aggr=[[COUNT(UInt8(1)) AS COUNT(*)]]
+--Limit: skip=6, fetch=3
+----Filter: t1.a > Int32(3)
+------TableScan: t1 projection=[a]
+physical_plan
+AggregateExec: mode=Final, gby=[], aggr=[COUNT(*)]
+--CoalescePartitionsExec
+----AggregateExec: mode=Partial, gby=[], aggr=[COUNT(*)]
+------RepartitionExec: partitioning=RoundRobinBatch(4), input_partitions=1
+--------GlobalLimitExec: skip=6, fetch=3
+----------CoalesceBatchesExec: target_batch_size=8192
+------------FilterExec: a@0 > 3
+--------------MemoryExec: partitions=1, partition_sizes=[1]
+
+query I
+SELECT COUNT(*) FROM (SELECT a FROM t1 WHERE a > 3 LIMIT 3 OFFSET 6);
+----
+1
+
Review Comment:
👍
##########
datafusion/sqllogictest/test_files/explain.slt:
##########
@@ -273,7 +273,7 @@ query TT
EXPLAIN SELECT a, b, c FROM simple_explain_test limit 10;
----
physical_plan
-GlobalLimitExec: skip=0, fetch=10, statistics=[Rows=Exact(10), Bytes=Absent]
+GlobalLimitExec: skip=0, fetch=10, statistics=[Rows=Inexact(10), Bytes=Absent]
Review Comment:
The old version is definitely misleading, nice improvement 👍
--
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] Fix incorrect results in COUNT(*) queries with LIMIT [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8049:
URL: https://github.com/apache/arrow-datafusion/pull/8049#discussion_r1384954660
##########
datafusion/physical-plan/src/limit.rs:
##########
@@ -748,7 +773,58 @@ mod tests {
assert_eq!(row_count, Precision::Exact(10));
let row_count = row_number_statistics_for_global_limit(5, Some(10)).await?;
- assert_eq!(row_count, Precision::Exact(15));
+ assert_eq!(row_count, Precision::Exact(10));
+
+ let row_count = row_number_statistics_for_global_limit(400, Some(10)).await?;
+ assert_eq!(row_count, Precision::Exact(0));
+
+ let row_count = row_number_statistics_for_global_limit(398, Some(10)).await?;
+ assert_eq!(row_count, Precision::Exact(2));
+
+ let row_count = row_number_statistics_for_global_limit(398, Some(1)).await?;
+ assert_eq!(row_count, Precision::Exact(1));
+
+ let row_count = row_number_statistics_for_global_limit(398, None).await?;
+ assert_eq!(row_count, Precision::Exact(2));
+
+ let row_count =
+ row_number_statistics_for_global_limit(0, Some(usize::MAX)).await?;
+ assert_eq!(row_count, Precision::Exact(400));
+
+ let row_count =
+ row_number_statistics_for_global_limit(398, Some(usize::MAX)).await?;
+ assert_eq!(row_count, Precision::Exact(2));
+
+ let row_count =
Review Comment:
```suggestion
// test inexact input statistics
let row_count =
```
This threw me for quite a while while trying to figure out why a fetch of 10 resulted in inexact statistics. Now that I see the difference is the input statistics are inexact it makes much more sense
--
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