Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Enable logical property propagation by default" #22171

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

feilong-liu
Copy link
Contributor

Reverts #22013

We found a correctness bug for the logical property propagation framework, a reproduce query is as follows:

Returns 0 rows when the framework is on:

presto:tpch> set session exploit_constraints=true;
SET SESSION
presto:tpch> with t as (select orderkey, count(1) cnt from (select * from (select * from orders where 1=0) left join (select partkey, suppkey from lineitem where 1=0) on partkey=10 where suppkey is not null) group by rollup(orderkey)) select t1.orderkey, t1.cnt from t t1 cross join t t2;
 orderkey | cnt 
----------+-----
(0 rows)

Returns 1 row when the framework is off:

presto:tpch> set session exploit_constraints=false;
SET SESSION
presto:tpch> with t as (select orderkey, count(1) cnt from (select * from (select * from orders where 1=0) left join (select partkey, suppkey from lineitem where 1=0) on partkey=10 where suppkey is not null) group by rollup(orderkey)) select t1.orderkey, t1.cnt from t t1 cross join t t2;
 orderkey | cnt 
----------+-----
 NULL     |   0 
(1 row)

Looking into the query plan, looks like that the framework incorrectly oversimplify the plan
When framework is on:

presto:tpch> set session exploit_constraints=true;
SET SESSION
presto:tpch> explain (type distributed) with t as (select orderkey, count(1) cnt from (select * from (select * from orders where 1=0) left join (select partkey, suppkey from lineitem where 1=0) on partkey=10 where suppkey is not null) group by rollup(orderkey)) select t1.orderkey, t1.cnt from t t1 cross join t t2;
                                                                                                   Query Plan                                                                                                   
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 Fragment 0 [SINGLE]                                                                                                                                                                                            
     Output layout: [orderkey$gid, count]                                                                                                                                                                       
     Output partitioning: SINGLE []                                                                                                                                                                             
     Stage Execution Strategy: UNGROUPED_EXECUTION                                                                                                                                                              
     - Output[PlanNodeId 63][orderkey, cnt] => [orderkey$gid:bigint, count:bigint]                                                                                                                              
             orderkey := orderkey$gid (1:257)                                                                                                                                                                   
             cnt := count (1:270)                                                                                                                                                                               
         - CrossJoin[PlanNodeId 370] => [orderkey$gid:bigint, count:bigint]                                                                                                                                     
                 Distribution: REPLICATED                                                                                                                                                                       
             - Project[PlanNodeId 24][projectLocality = LOCAL] => [orderkey$gid:bigint, count:bigint]                                                                                                           
                 - Aggregate(FINAL)[orderkey$gid, groupid][$hashvalue][PlanNodeId 23] => [orderkey$gid:bigint, groupid:bigint, $hashvalue:bigint, count:bigint]                                                 
                         count := "presto.default.count"((count_246)) (1:56)                                                                                                                                    
                     - LocalExchange[PlanNodeId 1597][HASH][$hashvalue] (orderkey$gid, groupid) => [orderkey$gid:bigint, groupid:bigint, count_246:bigint, $hashvalue:bigint]                                   
                         - Aggregate(PARTIAL)[orderkey$gid, groupid][$hashvalue_247][PlanNodeId 1595] => [orderkey$gid:bigint, groupid:bigint, $hashvalue_247:bigint, count_246:bigint]                         
                                 count_246 := "presto.default.count"((expr_93)) (1:56)                                                                                                                          
                             - Project[PlanNodeId 1624][projectLocality = LOCAL] => [orderkey$gid:bigint, expr_93:integer, groupid:bigint, $hashvalue_247:bigint]                                               
                                     Estimates: {source: CostBasedSourceInfo, rows: 0 (0B), cpu: 0.00, memory: 0.00, network: 0.00}                                                                             
                                     $hashvalue_247 := combine_hash(combine_hash(BIGINT'0', COALESCE($operator$hash_code(orderkey$gid), BIGINT'0')), COALESCE($operator$hash_code(groupid), BIGINT'0')) (1:239) 
                                 - Values[PlanNodeId 1474] => [orderkey$gid:bigint, expr_93:integer, groupid:bigint]                                                                                            
                                         Estimates: {source: CostBasedSourceInfo, rows: 0 (0B), cpu: 0.00, memory: 0.00, network: 0.00}                                                                         
             - Values[PlanNodeId 1478] => []                                                                                                                                                                    
                     Estimates: {source: CostBasedSourceInfo, rows: 0 (0B), cpu: 0.00, memory: 0.00, network: 0.00}                                                                                             
                                                                                                                                                                                                                
                                                                                                                                                                                                                
(1 row)

When framework is off:

presto:tpch> set session exploit_constraints=false;
SET SESSION
presto:tpch> explain (type distributed) with t as (select orderkey, count(1) cnt from (select * from (select * from orders where 1=0) left join (select partkey, suppkey from lineitem where 1=0) on partkey=10 where suppkey is not null) group by rollup(orderkey)) select t1.orderkey, t1.cnt from t t1 cross join t t2;
                                                                                                         Query Plan                                                                                                >
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------->
 Fragment 0 [SINGLE]                                                                                                                                                                                               >
     Output layout: [orderkey$gid, count]                                                                                                                                                                          >
     Output partitioning: SINGLE []                                                                                                                                                                                >
     Stage Execution Strategy: UNGROUPED_EXECUTION                                                                                                                                                                 >
     - Output[PlanNodeId 63][orderkey, cnt] => [orderkey$gid:bigint, count:bigint]                                                                                                                                 >
             orderkey := orderkey$gid (1:257)                                                                                                                                                                      >
             cnt := count (1:270)                                                                                                                                                                                  >
         - CrossJoin[PlanNodeId 370] => [orderkey$gid:bigint, count:bigint]                                                                                                                                        >
                 Distribution: REPLICATED                                                                                                                                                                          >
             - Project[PlanNodeId 24][projectLocality = LOCAL] => [orderkey$gid:bigint, count:bigint]                                                                                                              >
                 - Aggregate(FINAL)[orderkey$gid, groupid][$hashvalue][PlanNodeId 23] => [orderkey$gid:bigint, groupid:bigint, $hashvalue:bigint, count:bigint]                                                    >
                         count := "presto.default.count"((count_246)) (1:56)                                                                                                                                       >
                     - LocalExchange[PlanNodeId 1619][HASH][$hashvalue] (orderkey$gid, groupid) => [orderkey$gid:bigint, groupid:bigint, count_246:bigint, $hashvalue:bigint]                                      >
                         - Aggregate(PARTIAL)[orderkey$gid, groupid][$hashvalue_247][PlanNodeId 1617] => [orderkey$gid:bigint, groupid:bigint, $hashvalue_247:bigint, count_246:bigint]                            >
                                 count_246 := "presto.default.count"((expr_93)) (1:56)                                                                                                                             >
                             - Project[PlanNodeId 1673][projectLocality = LOCAL] => [orderkey$gid:bigint, expr_93:integer, groupid:bigint, $hashvalue_247:bigint]                                                  >
                                     Estimates: {source: CostBasedSourceInfo, rows: 0 (0B), cpu: 0.00, memory: 0.00, network: 0.00}                                                                                >
                                     $hashvalue_247 := combine_hash(combine_hash(BIGINT'0', COALESCE($operator$hash_code(orderkey$gid), BIGINT'0')), COALESCE($operator$hash_code(groupid), BIGINT'0')) (1:239)    >
                                 - Values[PlanNodeId 1474] => [orderkey$gid:bigint, expr_93:integer, groupid:bigint]                                                                                               >
                                         Estimates: {source: CostBasedSourceInfo, rows: 0 (0B), cpu: 0.00, memory: 0.00, network: 0.00}                                                                            >
             - LocalExchange[PlanNodeId 1602][SINGLE] () => []                                                                                                                                                     >
                 - Project[PlanNodeId 53][projectLocality = LOCAL] => []                                                                                                                                           >
                     - Aggregate(FINAL)[orderkey$gid_225, groupid_226][$hashvalue_248][PlanNodeId 52] => [orderkey$gid_225:bigint, groupid_226:bigint, $hashvalue_248:bigint]                                      >
                         - LocalExchange[PlanNodeId 1629][HASH][$hashvalue_248] (orderkey$gid_225, groupid_226) => [orderkey$gid_225:bigint, groupid_226:bigint, $hashvalue_248:bigint]                            >
                             - Aggregate(PARTIAL)[orderkey$gid_225, groupid_226][$hashvalue_249][PlanNodeId 1627] => [orderkey$gid_225:bigint, groupid_226:bigint, $hashvalue_249:bigint]                          >
                                 - Project[PlanNodeId 1674][projectLocality = LOCAL] => [orderkey$gid_225:bigint, groupid_226:bigint, $hashvalue_249:bigint]                                                       >
                                         Estimates: {source: CostBasedSourceInfo, rows: 0 (0B), cpu: 0.00, memory: 0.00, network: 0.00}                                                                            >
                                         $hashvalue_249 := combine_hash(combine_hash(BIGINT'0', COALESCE($operator$hash_code(orderkey$gid_225), BIGINT'0')), COALESCE($operator$hash_code(groupid_226), BIGINT'0'))>
                                     - Values[PlanNodeId 1478] => [orderkey$gid_225:bigint, groupid_226:bigint]                                                                                                    >
                                             Estimates: {source: CostBasedSourceInfo, rows: 0 (0B), cpu: 0.00, memory: 0.00, network: 0.00}                                                                        >
                                                                                                                                                                                                                   >
                                                                                                                                                                                                                   >
(1 row)

Copy link

Codenotify: Notifying subscribers in CODENOTIFY files for diff 88cda16...520f939.

Notify File(s)
@steveburnett presto-docs/src/main/sphinx/admin/properties.rst
presto-docs/src/main/sphinx/optimizer.rst
presto-docs/src/main/sphinx/optimizer/logical-properties.rst

@feilong-liu feilong-liu merged commit 4d6b143 into master Mar 12, 2024
57 checks passed
@feilong-liu feilong-liu deleted the revert-22013-enablelogicalprop branch March 12, 2024 20:13
@rschlussel
Copy link
Contributor

@feilong-liu a bit late seeing this, but can you add the failing query as a test in ATQ so the regression doesn't get reintroduced?

feilong-liu added a commit to feilong-liu/presto that referenced this pull request Mar 15, 2024
Add test mentioned in prestodb#22171
so as to avoid reintroducing the same problem.
@feilong-liu
Copy link
Contributor Author

@feilong-liu a bit late seeing this, but can you add the failing query as a test in ATQ so the regression doesn't get reintroduced?

@rschlussel Add in #22219

feilong-liu added a commit to feilong-liu/presto that referenced this pull request Mar 15, 2024
Add test mentioned in prestodb#22171
so as to avoid reintroducing the same problem.
feilong-liu added a commit that referenced this pull request Mar 15, 2024
Add test mentioned in #22171
so as to avoid reintroducing the same problem.
@ClarenceThreepwood
Copy link
Contributor

@feilong-liu - We would like to try enabling this again and want to minimize disruption.
How did you discover this bug - is there a test harness we can try this change on before enabling on master again?
Would it be better to create a build and shadow test on Meta workload before another attempt? Wdyt?

efrancisworks pushed a commit to efrancisworks/presto that referenced this pull request Mar 31, 2024
Add test mentioned in prestodb#22171
so as to avoid reintroducing the same problem.
gupteaj pushed a commit to gupteaj/presto that referenced this pull request Apr 17, 2024
Add test mentioned in prestodb#22171
so as to avoid reintroducing the same problem.
@wanglinsong wanglinsong mentioned this pull request May 1, 2024
48 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants