leborchuk commented on code in PR #1742:
URL: https://github.com/apache/cloudberry/pull/1742#discussion_r3280632092


##########
src/test/regress/expected/bfv_index_optimizer.out:
##########
@@ -1317,26 +1317,22 @@ insert into ao_tbl select 'abc' from 
generate_series(1,20) i;
 analyze ao_tbl;
 -- identical plans
 explain select * from ao_tbl where path_hash = 'ABC'; 
-                                  QUERY PLAN                                  
-------------------------------------------------------------------------------
- Gather Motion 1:1  (slice1; segments: 1)  (cost=0.00..387.96 rows=1 width=4)
-   ->  Bitmap Heap Scan on ao_tbl  (cost=0.00..387.96 rows=1 width=4)
-         Recheck Cond: ((path_hash)::text = 'ABC'::text)
-         ->  Bitmap Index Scan on ao_idx  (cost=0.00..0.00 rows=0 width=0)
-               Index Cond: ((path_hash)::text = 'ABC'::text)
- Optimizer: Pivotal Optimizer (GPORCA)
-(6 rows)
+                                  QUERY PLAN                                   
+-------------------------------------------------------------------------------
+ Gather Motion 1:1  (slice1; segments: 1)  (cost=0.00..431.00 rows=20 width=4)
+   ->  Seq Scan on ao_tbl  (cost=0.00..431.00 rows=7 width=4)
+         Filter: ((path_hash)::text = 'ABC'::text)
+ Optimizer: GPORCA
+(4 rows)

Review Comment:
   It looks like a regression. `Bitmap Index Scan` switched to `Seq Scan`. But 
it's a wrong decision. 
   
   Earlier we performed:
   ```
   insert into ao_tbl select 'abc' from generate_series(1,20) i;
   ```
   So `ao_tbl` contains only lowecased strings. And when we search `ABC` in 
uppercase it returns no rows. Using index to check that there actually no rows 
looks preferable. Ad old cost is lower then new cost. 



##########
src/test/regress/expected/stats_ext_optimizer.out:
##########
@@ -1947,25 +1947,25 @@ SELECT * FROM check_estimated_rows('SELECT * FROM 
mcv_lists WHERE 1 = a AND ''1'
 SELECT * FROM check_estimated_rows('SELECT * FROM mcv_lists WHERE a < 1 AND b 
< ''1''');
  estimated | actual 
 -----------+--------
-        36 |     50
+         2 |     50

Review Comment:
   Here the second problem. The new estimation is worse then old algorithm. 
   
   | Query | Old estimate | New estimate | Actual |
   |-------|-------------|-------------|--------|
   | `a < 1 AND b < '1'` | 36 | 2 | 50 |
   | `a < 5 AND b < '1' AND c < 5` | 85 | 5 | 50 |
   
   As I understand here we just have a correlation between predicates:
   ```
   postgres=# SELECT count(*) FROM mcv_lists WHERE a < 1;
    count
   -------
       50
   (1 row)
   
   postgres=# SELECT count(*) FROM mcv_lists WHERE b < '1';
    count
   -------
      100
   (1 row)
   
   postgres=# SELECT count(*) FROM mcv_lists WHERE a < 1 AND b < '1';
    count
   -------
       50
   (1 row)
   
   postgres=# SELECT count(*) FROM mcv_lists;
    count
   -------
     5000
   (1 row)
   ```
   
   So the overall cardinality should be like 5000 * 50/5000 * 100/5000 == 1. 
The old approach used default cardinality values and it appears closer to the 
actual cardinality for skew. 
   
   I do not know what to do with it. It's not a flaw in the current approach, 
but it is inspired by changes. But anyway, maybe we can improve it somehow.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to