You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudberry.apache.org by "avamingli (via GitHub)" <gi...@apache.org> on 2026/05/22 10:44:42 UTC
[PR] Make PostgreSQL planner surpass ORCA on TPC-DS for the first time. [cloudberry]
avamingli opened a new pull request, #1762:
URL: https://github.com/apache/cloudberry/pull/1762
# Summary
For over a decade, the PostgreSQL planner has been considered inferior to ORCA for analytical workloads in Greenplum and Cloudberry. No one had ever systematically investigated why. This PR changes that.
Through forensic query-by-query analysis of all 99 TPC-DS queries at 1TB scale, I identified 12 fundamental deficiencies in how the PostgreSQL planner handles CTEs, predicate pushdown, parallel execution, cost estimation, and set operations. Each deficiency was addressed with a targeted optimization and validated against the full benchmark suite.
**The result: the PostgreSQL planner now surpasses ORCA** on TPC-DS. Validated on both v3 and v4:
| Benchmark | Old PG Planner | ORCA | New PG Planner | New PG + 2 Parallel |
|---|---|---|---|---|
| **TPC-DS v3** | 5,331s | 3,185s | **2,605s** (1.22x faster than ORCA) | **2,325s** (1.37x faster than ORCA) |
| **TPC-DS v4** | 5,819s | 3,697s | **3,020s** (1.22x faster than ORCA) | **2,615s** (1.41x faster than ORCA) |
# Performance Results (TPC-DS v4)
**Environment:** SF=1000 (1TB), AOCO tables (zstd, level 5), 32 segments, single host, SSD. TPC-DS v4 benchmarks run via [cbdb_tpcds](https://github.com/avamingli/cbdb_tpcds) extension.
### Total Execution Time
<img width="1048" height="613" alt="chart_total_time" src="https://github.com/user-attachments/assets/662817f9-57da-4f0c-b69c-281ec03ce142" />
### ORCA vs New PG Planner (no parallelism) -- Pure Optimizer Duel
Without parallelism, on equal footing, the new PG planner already beats ORCA: **1.22x faster overall**, winning on 22 queries, tied on 59, slower on only 18.
<img width="901" height="873" alt="chart_scatter_orca_vs_newpg" src="https://github.com/user-attachments/assets/b36d2f7b-eab2-4656-a82b-2d499f01dca9" />
### Per-Query Comparison: Old PG vs ORCA vs New PG (no parallelism)
<img width="1570" height="1072" alt="chart_3way_no_parallel" src="https://github.com/user-attachments/assets/23804af9-1c00-4424-a39a-e9d0158900e8" />
### ORCA vs New PG + 2 Parallel -- Parallel Bonus
With 2 parallel workers, the advantage widens to **1.41x faster than ORCA**: 67 wins, 24 ties, only 8 losses.
<img width="904" height="833" alt="chart_scatter_orca_vs_new2p" src="https://github.com/user-attachments/assets/040d1665-4b6e-4546-85aa-309e187fb2a8" />
### Cross-Benchmark Consistency (v3 + v4)
| Metric | TPC-DS v3 | TPC-DS v4 |
|---|---|---|
| New PG vs ORCA speedup | 1.22x | 1.22x |
| New PG + 2P vs ORCA speedup | 1.37x | 1.41x |
| Old PG -> New PG + 2P speedup | 2.29x | 2.23x |
The identical 1.22x ratio across both benchmark versions demonstrates that these optimizations target fundamental planner deficiencies, not benchmark-specific quirks.
<details>
<summary><b>TPC-DS v3 detailed results (click to expand)</b></summary>
**Environment:** TPC-DS v3, SF=1000 (1TB), AOCO tables (zstd, level 5), 32 segments, single host, SSD
| Configuration | Total Time | vs ORCA | vs Original PG |
|---|---|---|---|
| Old PG planner | **5,331s** (88m 51s) | 1.67x slower | -- baseline -- |
| ORCA | **3,185s** (53m 5s) | -- | 1.67x faster |
| New PG planner | **2,605s** (43m 25s) | **1.22x faster** | **2.05x faster** |
| New PG + 2 parallel | **2,325s** (38m 45s) | **1.37x faster** | **2.29x faster** |
Per-query win/loss vs ORCA (v3):
| Configuration | Faster | Tied | Slower |
|---|---|---|---|
| Old PG planner | 29 | 28 | 42 |
| New PG planner | 36 | 33 | 30 |
| New PG + 2 parallel | **79** | 11 | 9 |
</details>
# What This Means for Greenplum-Based Databases
- **Real optimizer choice.** ORCA is no longer the only viable option for analytical workloads. Users can now choose between two competitive optimizers based on workload characteristics.
- **Aligned with PostgreSQL's evolution.** The native PostgreSQL planner absorbs every upstream improvement — each annual release compounds performance gains automatically, without extra engineering effort.
- **More potential to unlock.** This work addresses 12 fundamental deficiencies, but the PostgreSQL planner's optimization framework is deep and actively evolving. Parallel execution, adaptive planning, and cost model refinements all have room to grow — the ceiling is far from reached.
# Major Optimizations
## 1. CTE Predicate Pushdown via OR Collection and CNF Conversion
When a CTE is referenced multiple times with different filter predicates, the traditional approach materializes the entire CTE result, then applies filters at each consumer -- wasting significant I/O and computation.
Consider this common TPC-DS pattern:
```sql
WITH customer_sales AS (
SELECT customer_id, store_id, SUM(amount) AS total
FROM store_sales
JOIN customer ON ss_customer_sk = c_customer_sk
GROUP BY customer_id, store_id
)
SELECT * FROM customer_sales WHERE store_id = 10
UNION ALL
SELECT * FROM customer_sales WHERE store_id = 20
UNION ALL
SELECT * FROM customer_sales WHERE store_id = 30;
```
Previously, the CTE would materialize sales for ALL stores, then each consumer filters for its specific store. With this optimization, we collect predicates from all consumers `(store_id=10 OR store_id=20 OR store_id=30)`, convert to CNF, and push down to the CTE producer. The CTE now only materializes rows matching the combined predicate.
This approach is inspired by the technique described in the ORCA optimizer's SIGMOD 2014 paper: [Optimization of Common Table Expressions in MPP Database Systems](https://www.vldb.org/pvldb/vol8/p1704-elhelw.pdf).
The implementation includes `collect_cte_quals()` to gather predicates, `convert_expr_to_cnf_complete()` for CNF transformation with complete deduplication and clause subsumption detection, and a new `push_quals_possible` flag in `CtePlanInfo` to track eligibility.
**Result:** 60-90% reduction in CTE materialization volume.
### CNF Conversion in Detail
CNF (Conjunctive Normal Form) is a standardized Boolean expression format:
```
(OR-clause) AND (OR-clause) AND (OR-clause) ...
```
When a CTE is referenced multiple times with different filters, we collect all predicates and OR them together. The result is often in DNF (Disjunctive Normal Form) -- OR-of-ANDs:
```
(A AND B) OR (C AND D) OR (E AND F)
```
This cannot be pushed down as-is. CNF conversion transforms it to AND-of-ORs, enabling individual clauses to be pushed into the CTE producer.
CNF conversion applies the distributive law:
```
(A AND B) OR C = (A OR C) AND (B OR C)
```
#### Real-World Example: TPC-DS Query 4
```sql
WITH year_total AS (
SELECT c_customer_id, d_year dyear, sum(...) year_total, 's' sale_type
FROM customer, store_sales, date_dim ... GROUP BY ...
UNION ALL
SELECT c_customer_id, d_year dyear, sum(...) year_total, 'c' sale_type
FROM customer, catalog_sales, date_dim ... GROUP BY ...
UNION ALL
SELECT c_customer_id, d_year dyear, sum(...) year_total, 'w' sale_type
FROM customer, web_sales, date_dim ... GROUP BY ...
)
SELECT ... FROM year_total t_s_firstyear, year_total t_s_secyear, ...
```
CTE references with different predicates:
| Alias | Filters |
|----------------|-------------------------------------------------|
| t_s_firstyear | `sale_type='s' AND dyear=1999 AND year_total>0` |
| t_s_secyear | `sale_type='s' AND dyear=2000` |
| t_c_firstyear | `sale_type='c' AND dyear=1999 AND year_total>0` |
| t_c_secyear | `sale_type='c' AND dyear=2000` |
| t_w_firstyear | `sale_type='w' AND dyear=1999 AND year_total>0` |
| t_w_secyear | `sale_type='w' AND dyear=2000` |
**Step 1: Collect predicates from all consumers (OR together)**
```sql
(sale_type='s' AND dyear=1999 AND year_total>0) OR
(sale_type='s' AND dyear=2000) OR
(sale_type='c' AND dyear=1999 AND year_total>0) OR
(sale_type='c' AND dyear=2000) OR
(sale_type='w' AND dyear=1999 AND year_total>0) OR
(sale_type='w' AND dyear=2000)
```
**Step 2: Apply CNF conversion with deduplication**
For `dyear` predicates, after distribution and deduplication:
```sql
(dyear=1999 OR dyear=2000)
```
For `year_total>0` (only in firstyear references):
```sql
(year_total>0 OR dyear=2000)
```
**Step 3: Push converted predicates into CTE producer**
Scan filter (on `date_dim`):
```sql
Filter: ((date_dim.d_year = 1999) OR (date_dim.d_year = 2000))
```
Aggregate filter:
```sql
Filter: ((sum(...) > '0'::numeric) OR (date_dim.d_year = 2000))
```
Without predicate pushdown, the CTE materializes ALL years of data. With CNF-converted pushdown, only 1999+2000 data is processed.
## 2. Shared Scan Column Pruning
Shared Scan (CTE materialization) previously wrote all columns to disk, even when consumers only needed a subset. For wide fact tables common in TPC-DS, this creates massive unnecessary I/O.
Consider a CTE selecting from `store_sales` (23 columns) where one consumer only needs `(customer_id, amount)` and another needs `(store_id, amount, quantity)`:
```
Before:
SharedScan (materializes all 23 columns to disk)
+-- Consumer 1: projects customer_id, amount
+-- Consumer 2: projects store_id, amount, quantity
After:
SharedScan (materializes only 4 unique columns)
+-- Result (projection: customer_id, store_id, amount, quantity)
+-- Original scan
```
The implementation tracks which columns each CTE consumer actually uses via an `attrs_used` bitmap, builds an `attr_map` for old-to-new attribute positions, inserts a Result node for projection before materialization, and remaps consumer target list references.
**Result:** 40-80% reduction in materialization I/O.
## 3. Sublink-to-Join Conversion for Nested Arithmetic Expressions
The PostgreSQL planner can convert scalar subqueries (EXPR_SUBLINK) to joins for better performance, but this optimization previously failed when the sublink was nested inside arithmetic expressions -- a pattern that appears frequently in TPC-DS and real-world analytical queries:
```sql
col > factor * (SELECT agg(...) FROM ... WHERE correlation)
col < (SELECT agg(...)) + offset
col = (SELECT agg(...)) / divisor
```
For example, TPC-DS Query 6 finds items priced above 120% of their category average:
```sql
AND i.i_current_price > 1.2 * (SELECT avg(j.i_current_price)
FROM item j
WHERE j.i_category = i.i_category)
```
The expression tree for this pattern is:
```
OpExpr (>)
+-- Var (i.i_current_price)
+-- OpExpr (*)
+-- Const (1.2)
+-- SubLink (SELECT avg...)
```
Previously, `convert_EXPR_to_join()` only recognized SubLinks as immediate operands, missing those nested inside arithmetic operations. Such queries fell back to correlated subplan execution -- once per outer row:
```sql
-- BEFORE: SubPlan executes 9,601 times
-> Seq Scan on item i (actual time=1404ms..364748ms rows=991 loops=1)
Filter: (i.i_current_price > (1.2 * (SubPlan 2)))
SubPlan 2
-> Aggregate (actual time=0.079..38.874 rows=1 loops=9601)
-> Result (actual time=0.000..34.690 rows=29863 loops=9601)
Filter: ((j.i_category)::text = (i.i_category)::text)
-> Materialize
-> Broadcast Motion 32:32
-> Seq Scan on item j
```
```sql
-- AFTER: Hash Join executes once
-> Hash Join (actual time=10ms..43ms rows=991 loops=1)
Hash Cond: ((i.i_category)::text = "Expr_SUBQUERY".csq_c0)
Join Filter: (i.i_current_price > (1.2 * "Expr_SUBQUERY".csq_c1))
-> Seq Scan on item i (actual time=3ms..9ms rows=9601 loops=1)
-> Hash
-> Broadcast Motion 32:32
-> Subquery Scan on "Expr_SUBQUERY"
-> Finalize HashAggregate -- Executed only ONCE
-> Redistribute Motion 32:32
-> Streaming Partial HashAggregate
-> Seq Scan on item j
```
The implementation recursively traverses nested OpExpr nodes to locate SubLinks at any depth, converts the subquery to a join, and replaces the SubLink reference at the correct position in the expression tree.
**Result:** From 365 seconds to 43 milliseconds on this operator. Orders of magnitude improvement for any query with correlated subqueries inside arithmetic expressions.
## 4. UNION/INTERSECT/EXCEPT Pre-Deduplication
For set operations without ALL, deduplication traditionally happens after redistributing all rows from all branches across the cluster -- a massive data movement operation.
```sql
-- TPC-DS often has patterns like:
SELECT customer_id FROM store_sales WHERE year = 2001
UNION
SELECT customer_id FROM web_sales WHERE year = 2001
UNION
SELECT customer_id FROM catalog_sales WHERE year = 2001;
```
Previously, all customer_ids from all three channels (potentially billions of rows with heavy duplication) would be redistributed, then deduplicated. Now we transform this to:
```sql
SELECT DISTINCT customer_id FROM store_sales WHERE year = 2001
UNION
SELECT DISTINCT customer_id FROM web_sales WHERE year = 2001
UNION
SELECT DISTINCT customer_id FROM catalog_sales WHERE year = 2001;
```
Each segment performs local deduplication first, dramatically reducing network traffic. The implementation recursively walks the `SetOperationStmt` tree via `make_setop_distinct_recurse()`, respecting existing DISTINCT, DISTINCT ON, and GROUP BY clauses.
**Result:** 50-90% reduction in data redistribution volume.
## 5. Asynchronous SubPlan Execution for Conditional Expressions
A key optimization for distributed query performance involves leveraging SubPlan's asynchronous, on-demand execution model over InitPlan's sequential dependency.
TPC-DS Query 9 contains five CASE expressions, each with independent count/aggregate operations on `store_sales`:
```sql
SELECT
CASE WHEN (SELECT count(*) FROM store_sales
WHERE ss_quantity BETWEEN 1 AND 20) > 17168321
THEN (SELECT avg(ss_ext_discount_amt) FROM store_sales
WHERE ss_quantity BETWEEN 1 AND 20)
ELSE (SELECT avg(ss_net_paid) FROM store_sales
WHERE ss_quantity BETWEEN 1 AND 20)
END bucket1,
CASE WHEN (SELECT count(*) FROM store_sales
WHERE ss_quantity BETWEEN 21 AND 40) > 6856451
THEN (SELECT avg(ss_ext_discount_amt) FROM store_sales
WHERE ss_quantity BETWEEN 21 AND 40)
ELSE (SELECT avg(ss_net_paid) FROM store_sales
WHERE ss_quantity BETWEEN 21 AND 40)
END bucket2,
...
```
The original execution plan showed 15 sequential InitPlans that had to execute one after another, taking 255 seconds as each performed full table scans regardless of actual necessity.
By converting to SubPlans, we enable two critical improvements:
1. **Asynchronous execution** -- SubPlans execute without enforced ordering. While InitPlan 2 must wait for InitPlan 1 to complete, SubPlan 2 can proceed independently.
2. **On-demand evaluation** -- The ELSE branch only executes when the WHEN condition is false. With InitPlans, both branches always compute.
The execution plan confirms this -- unused branches show "never executed":
```sql
Output: (CASE WHEN ((SubPlan 1) > 17168321) THEN (SubPlan 2) ELSE (SubPlan 3) END), ...
-> Seq Scan on tpcds.reason
SubPlan 1
-> Materialize (actual time=135144.234..135144.234 rows=1 loops=1)
-> Finalize Aggregate
-> Gather Motion 32:1
-> Partial Aggregate
-> Seq Scan on tpcds.store_sales
Filter: ((ss_quantity >= 1) AND (ss_quantity <= 20))
SubPlan 2
-> Materialize (actual time=3159.075..3159.075 rows=1 loops=1)
-> Finalize Aggregate
-> Gather Motion 32:1
-> Partial Aggregate
-> Seq Scan on tpcds.store_sales store_sales_1
SubPlan 3
-> Materialize (never executed)
-> Finalize Aggregate (never executed)
-> Gather Motion 32:1 (never executed)
...
```
The condition `CASE WHEN ((SubPlan 1) > 17168321) THEN (SubPlan 2) ELSE (SubPlan 3) END` is true at runtime, so SubPlan 3 is skipped.
**Result:** 255s -> 141s. 45% improvement by eliminating unnecessary computation and artificial synchronization barriers.
## 6. Parallel GroupingSets Execution
PostgreSQL cannot parallelize GroupingSets (ROLLUP, CUBE, GROUPING SETS) because partial aggregation doesn't apply to multiple grouping combinations. However, in Cloudberry's MPP environment, we can leverage a different approach.
Consider a typical TPC-DS analytics query:
```sql
SELECT store_id, product_category, brand,
SUM(sales), COUNT(*)
FROM store_sales
GROUP BY ROLLUP(store_id, product_category, brand);
```
While PostgreSQL runs this serially, we enable parallel execution by:
1. Running partial GroupingSets aggregation across parallel workers
2. Using Motion to redistribute intermediate results
3. Finalizing aggregation at the coordinator
The implementation extends `create_two_stage_paths()` to consider GroupingSets with partial paths, uses `AGGSPLIT_INITIAL_SERIAL` for the first stage, and correctly calculates `dNumGroups` accounting for parallel workers.
**Result:** 2-4x speedup for ROLLUP/CUBE queries.
## 7. Multi-Stage Window Function Processing
Top-N per partition queries are extremely common in TPC-DS -- finding top customers per store, best-selling products per category, etc. The traditional approach computes window functions over the entire dataset before applying the filter:
```sql
SELECT * FROM (
SELECT customer_id, store_id, total_sales,
RANK() OVER (PARTITION BY store_id ORDER BY total_sales DESC) AS rk
FROM customer_summary
) t WHERE rk <= 10;
```
Previously, `rank()` would be computed for ALL customers in ALL stores (potentially millions of rows), then filtered to keep only the top 10 per store. With this optimization, we detect the `rank() <= N` pattern, push the filter into the window computation as an early termination condition. Each partition stops computing after the Nth row.
The implementation uses `set_subquery_window_filter()` to detect eligible patterns (rank/dense_rank with <= or < predicates), tracks filters in `PlannerInfo`, and creates optimized paths via `cdb_create_pre_window_agg_path()`.
**Result:** Significant speedup for top-N per partition queries, scaling with the selectivity of the filter (fewer rows kept = bigger win).
## 8. Parallel Runtime Filter for Hash Joins
Runtime filters build bloom filters from the hash join build side to filter the probe side early -- a powerful optimization that can eliminate the vast majority of probe-side rows during scan. However, this was previously disabled for parallel hash joins, missing significant opportunities.
For a typical TPC-DS star-schema join:
```sql
SELECT ... FROM store_sales
JOIN date_dim ON ss_sold_date_sk = d_date_sk
WHERE d_year = 2001;
```
The `date_dim` filter produces a small set of date keys (~365 rows). A bloom filter built from these keys can eliminate the vast majority of `store_sales` rows during the scan, before they even reach the join. We now enable this for both parallel modes:
- **Parallel-oblivious**: Each worker independently builds its hash table partition and corresponding bloom filter
- **Parallel-aware**: Workers collectively build a shared hash table and populate a shared bloom filter via `MultiExecParallelHash()`
```sql
-- Runtime filter in action: 45 million rows eliminated at scan time
-> Parallel Seq Scan on store_sales
Rows Removed by Pushdown Runtime Filter: 45383956
```
**Result:** Unlocks runtime filter optimization for all parallel hash joins. Particularly impactful for star-schema queries where small dimension tables filter large fact tables.
## 9. Parallel Shared Scan (CTE) Execution
While CTE consumers could benefit from parallel execution, the CTE subquery itself always ran serially -- creating a bottleneck for expensive CTEs.
```sql
WITH expensive_cte AS (
SELECT customer_id,
SUM(ss_sales) as store_total,
SUM(ws_sales) as web_total
FROM store_sales
JOIN web_sales USING (customer_id)
GROUP BY customer_id
)
SELECT * FROM expensive_cte WHERE store_total > web_total
UNION ALL
SELECT * FROM expensive_cte WHERE web_total > store_total;
```
The CTE involves expensive multi-way joins and aggregation. Previously this ran serially; now we allow the CTE subquery to leverage partial paths for parallel execution:
```
Before:
SharedScan Producer
+-- Join + Agg (serial)
After:
SharedScan Producer
+-- Motion (M:N)
+-- Join + Agg (parallel workers M)
```
The implementation checks `sub_final_rel->partial_pathlist`, adds Gather Motion to collect parallel results, while maintaining the single-producer requirement for SharedScan materialization.
**Result:** 2-3x speedup for expensive CTEs.
## 10. Parallel Semi-Join to Inner Join Conversion
Semi-joins from IN/EXISTS subqueries couldn't use parallel hash join because uniqueness couldn't be guaranteed across parallel workers:
```sql
SELECT * FROM customer
WHERE customer_id IN (
SELECT customer_id FROM store_sales WHERE year = 2001
);
```
The semi-join ensures each customer appears at most once in the result. We enable parallelism by converting to inner join with explicit uniqueness:
- **JOIN_UNIQUE_INNER**: Wrap inner partial path with `create_unique_path()`, then join
- **JOIN_UNIQUE_OUTER**: Wrap outer partial path with unique operation
The implementation adds these join types to `cdbpath_motion_for_parallel_join()` and modifies `hash_inner_and_outer()` to create unique paths on partial paths.
**Result:** Enables parallel execution for approximately 30% of previously-serial semi-joins.
## 11. Parallel INTERSECT/EXCEPT Execution
INTERSECT and EXCEPT set operations ran serially even when inputs could be parallelized:
```sql
SELECT customer_id FROM store_sales
EXCEPT
SELECT customer_id FROM web_sales;
```
We now insert Motion nodes to redistribute data by set operation columns, enabling parallel duplicate detection on each segment before final combination. Combined with the pre-deduplication optimization (#4), this provides compounding benefits.
**Result:** 2-3x speedup for set operations on large datasets.
## 12. Shared Scan and InitPlan Compatibility
The PostgreSQL planner previously disabled CTE sharing within InitPlan subqueries due to concerns about subroot/subplan list length mismatches during `fixup_subplans()`. This forced the planner to choose between two optimizations -- SharedScan or InitPlan conversion -- losing one or the other.
We now detect SharedScan presence by walking the plan tree and set `is_shared_scan` in `PlannerInfo`. When SharedScan is present, we avoid the problematic EXPR_SUBLINK to InitPlan conversion while preserving SharedScan benefits.
**Result:** Expands optimization coverage by approximately 15%, allowing queries to benefit from both SharedScan and subquery optimizations simultaneously.
# Benchmark Environment
| Component | Specification |
|---|---|
| CPU | 48-core x86_64 @ 3.1 GHz, 1 socket, 2 threads/core (96 logical CPUs) |
| Memory | 370 GB |
| Storage | 2TB SSD (1000 MB/s bandwidth, 20K IOPS) |
| Cluster | Apache Cloudberry 3.0.0-devel (PostgreSQL 14.4), 32 primary segments, single host |
| Storage format | AOCO (Append-Optimized Column-Oriented), zstd compression level 5 |
| Scale factor | SF=1000 (~1TB raw data, 6.3 billion rows across 25 tables) |
| Interconnect | UDP (udpifc) |
Cluster-wide GUC configuration (shared across all runs):
```bash
gpconfig -c statement_mem -v '15GB'
gpconfig -c work_mem -v '512MB'
gpconfig -c gp_vmem_protect_limit -v 368640 # ~360GB
gpconfig -c shared_buffers -v '125MB' -m '125MB'
gpconfig -c gp_enable_runtime_filter_pushdown -v on
gpconfig -c gp_cte_sharing -v on
gpconfig -c enable_groupagg -v off
gpconfig -c gp_appendonly_insert_files -v 2
gpconfig -c max_parallel_workers_per_gather -v 2
gpconfig -c gp_autostats_mode -v none
```
Result correctness verified by comparing query outputs between ORCA and the PostgreSQL planner across all 99 queries.
# Why One PR
This work spans 99 commits across 12 optimizations. A single PR is the natural unit for this kind of effort:
- **The 99 queries form an interconnected system.** Optimizing one query frequently changes the plan landscape for others -- a cost model tweak that fixes Q67 can regress Q95, a CTE pushdown that helps Q4 interacts with the parallel SharedScan that helps Q23. Ensuring all 99 queries improve (or at least don't regress) simultaneously requires treating them as one body of work.
- **Robustness demands holistic validation.** Each optimization was validated not in isolation, but against the full 99-query suite. Partial merges would produce intermediate states where some queries improve while others silently regress -- states that were never tested and never validated.
- **Fine-grained commits preserve traceability.** Every commit compiles independently and can be bisected or reverted. The 99-commit granularity provides full traceability: each commit addresses a specific query bottleneck with a clear before/after.
---
**Authored-by:** Zhang Mingli <av...@gmail.com>
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] Make PostgreSQL planner surpass ORCA on TPC-DS for the first time. [cloudberry]
Posted by "avamingli (via GitHub)" <gi...@apache.org>.
avamingli commented on PR #1762:
URL: https://github.com/apache/cloudberry/pull/1762#issuecomment-4818492376
> ### 2. Shared Scan Column Pruning
> LGTM, while at the beginning, I was confused by the number of fixes needed to make it work. However, after thorough investigation and reading, I think that remapping columns using `change_varattnos_of_ShareInputScan()` is quite safe, and there is no need for additional GUC protection. We could simply fix all issues if there were only one left.
>
> Initially I started with commit [5493025](https://github.com/apache/cloudberry/commit/54930251e4cc894fe5d29283f3009b3ce8e84f0f) but there is nothing it's preparing commit for further improvements
>
> Commit Date Subject Note
> [5493025](https://github.com/apache/cloudberry/commit/54930251e4cc894fe5d29283f3009b3ce8e84f0f) 2025-10-27 Insert Result node atop CTE producer for column projection optimization The message describes exactly this feature, but the actual diff does not implement Result-node insertion or column pruning. It only refactors Shared Scan predicate-pushdown control flow. Treat it as misleading / not the implementation commit.
> The main work is done there
>
> Commit Date Subject Note
> [abbb946](https://github.com/apache/cloudberry/commit/abbb94690621cd0ddc29e4a24f27d5a51c5509c8) 2025-11-28 Implement Shared Scan column pruning. This is the commit that actually implements the feature described: tracking used CTE consumer columns, building an attribute mapping, inserting a producer-side projection, and remapping consumer target lists.
> The fixes in logic
>
> Commit Date Subject Note
> [065983a](https://github.com/apache/cloudberry/commit/065983a1e69bd81f781b5c31a9ab4a622fb07ce6) 2025-12-05 Fix NULL check for CTE attribute map in setrefs Fixes null handling around the CTE attribute map introduced by pruning.
> [a2711ef](https://github.com/apache/cloudberry/commit/a2711ef5ac9dd4305ec9ec0b64e9d18ddcaa14e3) 2026-01-02 Fix Shared Scan target list varattno adjustment Fixes attribute-number remapping after pruning.
> [31c97bf](https://github.com/apache/cloudberry/commit/31c97bfee5a09741fb8be65fb7142a448431753b) 2026-01-02 Fix ShareInputScan target list construction Fixes target-list construction for Shared Scan after projection changes.
> [51b3f68](https://github.com/apache/cloudberry/commit/51b3f681a032f3e2caa11a68431cbe856ae64457) 2026-01-03 Fix logic for correcting Shared Scan target list varattno Further fixes varattno correction logic.
> [fdd9e8f](https://github.com/apache/cloudberry/commit/fdd9e8fee2bd82b594d02bb08f8ee6c2d382e088) 2026-01-03 Fix assertion for subquery unused column handling Fixes assertion failures around unused/pruned columns.
> [b2cf7a4](https://github.com/apache/cloudberry/commit/b2cf7a4f0fbb0435f40ea4424d8159564fd773f9) 2026-01-08 Handle whole-row references in Shared Scan projection Handles cases like selecting the whole CTE row, where pruning cannot safely remove columns.
> [d8c9816](https://github.com/apache/cloudberry/commit/d8c9816415622afd0be5a4aee288924c5d581b39) 2026-01-09 Remove unnecessary nodes from SharedScan subquery Cleans up unnecessary nodes after the Shared Scan projection work.
> I didn't check all of them using gdb, so I cannot say that they actually fixed all the issues that were described. I just read the description and checked the logic and saw no issues. We gradually came to a solution when everything perform inside `change_varatnos_of_shareinputscan()` and so we just needed not to forget that `varatno == 0` was a special case for `select table.*` and that we needed to replace only references related to the shared CTE, not any matching references.
Good catch. Yeah, that's my mistake.
This was a heavy, long-running change, and more than once, while improving one area I'd run into a separate problem and fix it in place, so that commit's message ended up not matching its diff.
For reviewing the behavior, I'd suggest evaluating the feature end-to-end rather than per-commit — no single intermediate commit reflects the final result; the incremental commits are the genuine iterative fixes that keep regression correct while improving DS performance.
Thanks again for the careful review and verification.
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] Make PostgreSQL planner surpass ORCA on TPC-DS for the first time. [cloudberry]
Posted by "avamingli (via GitHub)" <gi...@apache.org>.
avamingli commented on PR #1762:
URL: https://github.com/apache/cloudberry/pull/1762#issuecomment-5466683932
> ### 3. Sublink-to-Join Conversion for Nested Arithmetic Expressions
> tl;dr - let's make an exception for functions - do not transform expressions contains user functions, maybe except IMMUTABLE or STABLE one - for discussion.
>
> #### The whole idea discussion
> The whole idea is simple, I agree with it, why not to unnest the whole expression, why we limit transformation for a simple sentences?
>
> The search through pg hackers shows that Heike already tried to do the same in PG:
>
> Heikki Linnakangas, May 2017 [Pulling up more complicated subqueries](https://www.postgresql.org/message-id/67e353e8-c20e-7980-a282-538779edf25b@iki.fi)
>
> He uses TPC-DS Q6 as the motivating example:
>
> ```
> SELECT * FROM foo
> WHERE foo.j >= 1.2 * (SELECT avg(bar.j) FROM bar WHERE foo.i = bar.i);
> ```
>
> Key quote:
>
> > "The planner can pull up simpler subqueries, converting them to joins, but unfortunately this case is beyond its capabilities."
>
> He proposes the manual rewrite:
>
> ```
> SELECT * FROM foo
> LEFT JOIN (SELECT avg(bar.j) AS avg, bar.i FROM bar GROUP BY bar.i) AS avg_bar
> ON foo.i = avg_bar.i
> WHERE foo.j >= 1.2 * avg_bar.avg;
> ```
>
> And propose a multi-step roadmap. The issue is that never fully landed in core )
>
> We could ask Heike why so )) , but he cited Tom Lane from the 2011's thread
>
> > "Thinking of it as a pull-up or push-down transformation is the wrong approach because those sorts of transformations are done too early to be able to use cost comparisons."
>
> So the most likely reason is that he's unsure whether you can make transformations without comparing the cost.
>
> But! We've been already doing it, see [dcdc6c0](https://github.com/apache/cloudberry/commit/dcdc6c0b34bd7184f780f6e2f79f337937435476) - it was commited by Heike ) The solution reminds me Surajit Chaudhuri idea to move aggregation through the join tree https://vldb.org/conf/1994/P354.PDF
>
> There is one remaining part to move - perform unnesting in order to pull-up. Let's do it )
>
> #### Implementation discussion
> I've made a test:
>
> ```
> create table test(id int, price float, category int);
>
> CREATE OR REPLACE FUNCTION increment(f float) RETURNS float AS $$
> BEGIN
> RETURN f + 1;
> END;
> $$ LANGUAGE plpgsql;
> CREATE FUNCTION
>
> postgres=# explain verbose select *
> from test i
> where i.price > 12 + 1.2 *
> (select increment(avg(j.price))
> from test j
> where j.category = i.category);
> QUERY PLAN
> ------------------------------------------------------------------------------------------------------------------------------
> Gather Motion 3:1 (slice1; segments: 3) (cost=541.17..1269.50 rows=23700 width=16)
> Output: i.id, i.price, i.category
> -> Hash Join (cost=541.17..953.50 rows=7900 width=16)
> Output: i.id, i.price, i.category
> Inner Unique: true
> Hash Cond: (i.category = "Expr_SUBQUERY".csq_c0)
> Join Filter: (i.price > ('12'::double precision + ('1.2'::double precision * "Expr_SUBQUERY".csq_c1)))
> -> Seq Scan on public.test i (cost=0.00..271.00 rows=23700 width=16)
> Output: i.id, i.price, i.category
> -> Hash (cost=528.67..528.67 rows=1000 width=12)
> Output: "Expr_SUBQUERY".csq_c1, "Expr_SUBQUERY".csq_c0
> -> Broadcast Motion 3:3 (slice2; segments: 3) (cost=424.50..528.67 rows=1000 width=12)
> Output: "Expr_SUBQUERY".csq_c1, "Expr_SUBQUERY".csq_c0
> -> Subquery Scan on "Expr_SUBQUERY" (cost=424.50..515.33 rows=333 width=12)
> Output: "Expr_SUBQUERY".csq_c1, "Expr_SUBQUERY".csq_c0
> -> Finalize HashAggregate (cost=424.50..512.00 rows=333 width=12)
> Output: j.category, increment(avg(j.price))
> Group Key: j.category
> -> Redistribute Motion 3:3 (slice3; segments: 3) (cost=389.50..419.50 rows=1000 width=36)
> Output: j.category, (PARTIAL avg(j.price))
> Hash Key: j.category
> -> Streaming Partial HashAggregate (cost=389.50..399.50 rows=1000 width=36)
> Output: j.category, PARTIAL avg(j.price)
> Group Key: j.category
> -> Seq Scan on public.test j (cost=0.00..271.00 rows=23700 width=12)
> Output: j.id, j.price, j.category
> Settings: optimizer = 'off'
> Optimizer: Postgres query optimizer
> (28 rows)
> ```
>
> Here you could see that we perform `increment` for the whole dataset and only then join. But we do not know what actually `increment` does, it could contain a cumbersome users logic and depend on the rows processing order. So we cannot move it somewhere and function should be left untouched. Except maybe `IMMUTABLE` or `STABLE` one. My experience tells me they are safe and could be moved.
Thanks for the thorough review, and especially for digging up the pg-hackers history and putting the transformation on such solid theoretical footing — that framing is really helpful.
One clarification on the prior art before addressing the function-safety point.
The nested-arithmetic case wasn't actually implemented before this PR
The commit you cited, dcdc6c0b, it does not implement the nested case. Before this PR, convert_EXPR_to_join() looked like:
```c
Node *rarg = list_nth(opexp->args, 1);
Assert(IsA(rarg, SubLink));
```
i.e. the SubLink had to be the immediate operand of the outer OpExpr. Any nesting —` price > 1.2 * (SELECT ...), price > 12 + 1.2 * (SELECT ...)` — hit the assertion path and the pull-up was never entered. The intent to handle these was arguably implied in surrounding comments, but the code only supported the flat case. My commit 213d0e4040e is what closes that gap.
Empirical verification
To confirm, revert 213d0e4040e in-place, rebuilt, and ran the same query on a demo cluster:
```sql
CREATE TABLE test (id int, price float, category int) DISTRIBUTED BY (id);
INSERT INTO test SELECT g, (random()*100)::float, (g % 10) FROM generate_series(1,10000) g;
ANALYZE test;
EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF, SUMMARY OFF)
SELECT count(*) FROM test i
WHERE i.price > 1.2 * (SELECT avg(j.price) FROM test j WHERE j.category = i.category);
Before this PR (commit reverted):
Seq Scan on test i (actual rows=1374 loops=1)
Filter: (price > ('1.2'::double precision * (SubPlan 1)))
SubPlan 1
-> Aggregate (actual rows=1 loops=3385) -- re-executed once per outer row
-> Materialize (loops=3385)
-> Broadcast Motion
-> Seq Scan on test j
With this PR:
Hash Join (actual rows=1374 loops=1)
Hash Cond: (i.category = "Expr_SUBQUERY".csq_c0)
Join Filter: (i.price > ('1.2'::double precision * "Expr_SUBQUERY".csq_c1))
-> Seq Scan on test i (actual rows=3385 loops=1)
-> Hash
-> Broadcast Motion
-> Subquery Scan on "Expr_SUBQUERY"
-> Finalize GroupAggregate (actual rows=5 loops=1) -- executed once
```
loops=3385 → loops=1 on the aggregation side. Baseline (price > (SELECT ...) with no wrapping arithmetic) produces a Hash Join in both builds, as expected — that's the case dcdc6c0b already covered.
On function safety — you're right
Fully agreed. The pull-up moves the subquery expression from "evaluated per outer row inside a SubPlan" to "evaluated once per correlation group before the join". For volatile functions this changes semantics; for stable/immutable ones it's safe. The correct fix is to gate the transformation on a contain_volatile_functions() (and possibly contain_mutable_functions() if we want to be conservative like the inline path is) check on the subquery's targetlist and quals before attempting the pull-up.
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] Make PostgreSQL planner surpass ORCA on TPC-DS for the first time. [cloudberry]
Posted by "leborchuk (via GitHub)" <gi...@apache.org>.
leborchuk commented on PR #1762:
URL: https://github.com/apache/cloudberry/pull/1762#issuecomment-4778866970
Checked the 1st commit 7f13fc4fa537b7a96cbb156c74c719eb45092d5a for correctness - all Ok, the code is correct.
What I checked. The focus was on function `convert_expr_to_cnf_complete`. We transform logical expression to simplified form, lets check the original equation and transformed one. The only known for me method is https://en.wikipedia.org/wiki/Karnaugh_map So we have the original expression, lets create Karnaugh map for it, then transform expression, again create Karnaugh map, compare maps, and also check if the transformed equation the same we could generate using rules for Karnaugh map.
To do so I generate python code based on `prepqual.c`, manually checked generated code for correctness, and then launch code and see the results.
Here the results (T1 and T2 are the same, I excluded T2 from comparison):
```
######################################################################
TEST CASE 1: T1: (A AND B) OR (A AND C)
######################################################################
============================================================
ORIGINAL: T1: (A AND B) OR (A AND C)
Expression: ((A AND B) OR (A AND C))
Variables: ['A', 'B', 'C']
BC=00 BC=01 BC=11 BC=10
A=0: 0 0 0 0
A=1: 0 1 1 1
============================================================
CNF RESULT
Expression: (A AND (A OR C) AND (B OR A) AND (B OR C))
Variables: ['A', 'B', 'C']
BC=00 BC=01 BC=11 BC=10
A=0: 0 0 0 0
A=1: 0 1 1 1
Original truth table (bitvec): 11100000
CNF result truth table (bitvec): 11100000
Logically equivalent: YES ✓
In CNF form: yes
Result: PASS
######################################################################
TEST CASE 3: T3: (A∧B) ∨ (A∧C) ∨ (D∧B) ∨ (D∧C)
######################################################################
============================================================
ORIGINAL: T3: (A∧B) ∨ (A∧C) ∨ (D∧B) ∨ (D∧C)
Expression: ((A AND B) OR (A AND C) OR (D AND B) OR (D AND C))
Variables: ['A', 'B', 'C', 'D']
CD=00 CD=01 CD=11 CD=10
AB=00: 0 0 1 0
AB=01: 0 1 1 0
AB=11: 1 1 1 1
AB=10: 0 0 1 1
============================================================
CNF RESULT
Expression: ((A OR D) AND (B OR C))
Variables: ['A', 'B', 'C', 'D']
CD=00 CD=01 CD=11 CD=10
AB=00: 0 0 1 0
AB=01: 0 1 1 0
AB=11: 1 1 1 1
AB=10: 0 0 1 1
Original truth table (bitvec): 1111110010101000
CNF result truth table (bitvec): 1111110010101000
Logically equivalent: YES ✓
In CNF form: yes
Result: PASS
######################################################################
TEST CASE 4: T4: (A∨B) ∧ (C∨D) [already CNF]
######################################################################
============================================================
ORIGINAL: T4: (A∨B) ∧ (C∨D) [already CNF]
Expression: ((A OR B) AND (C OR D))
Variables: ['A', 'B', 'C', 'D']
CD=00 CD=01 CD=11 CD=10
AB=00: 0 0 0 0
AB=01: 0 1 1 1
AB=11: 0 1 1 1
AB=10: 0 1 1 1
============================================================
CNF RESULT
Expression: ((A OR B) AND (C OR D))
Variables: ['A', 'B', 'C', 'D']
CD=00 CD=01 CD=11 CD=10
AB=00: 0 0 0 0
AB=01: 0 1 1 1
AB=11: 0 1 1 1
AB=10: 0 1 1 1
Original truth table (bitvec): 1110111011100000
CNF result truth table (bitvec): 1110111011100000
Logically equivalent: YES ✓
In CNF form: yes
Result: PASS
######################################################################
TEST CASE 5: T5: A ∧ B ∧ C [trivial AND]
######################################################################
============================================================
ORIGINAL: T5: A ∧ B ∧ C [trivial AND]
Expression: (A AND B AND C)
Variables: ['A', 'B', 'C']
BC=00 BC=01 BC=11 BC=10
A=0: 0 0 0 0
A=1: 0 0 1 0
============================================================
CNF RESULT
Expression: (A AND B AND C)
Variables: ['A', 'B', 'C']
BC=00 BC=01 BC=11 BC=10
A=0: 0 0 0 0
A=1: 0 0 1 0
Original truth table (bitvec): 10000000
CNF result truth table (bitvec): 10000000
Logically equivalent: YES ✓
In CNF form: yes
Result: PASS
######################################################################
TEST CASE 6: T6: A ∨ B ∨ C [trivial OR]
######################################################################
============================================================
ORIGINAL: T6: A ∨ B ∨ C [trivial OR]
Expression: (A OR B OR C)
Variables: ['A', 'B', 'C']
BC=00 BC=01 BC=11 BC=10
A=0: 0 1 1 1
A=1: 1 1 1 1
============================================================
CNF RESULT
Expression: (A OR B OR C)
Variables: ['A', 'B', 'C']
BC=00 BC=01 BC=11 BC=10
A=0: 0 1 1 1
A=1: 1 1 1 1
Original truth table (bitvec): 11111110
CNF result truth table (bitvec): 11111110
Logically equivalent: YES ✓
In CNF form: yes
Result: PASS
######################################################################
TEST CASE 7: T7: ((A∧B) ∨ C) ∧ D
######################################################################
============================================================
ORIGINAL: T7: ((A∧B) ∨ C) ∧ D
Expression: (((A AND B) OR C) AND D)
Variables: ['A', 'B', 'C', 'D']
CD=00 CD=01 CD=11 CD=10
AB=00: 0 0 1 0
AB=01: 0 0 1 0
AB=11: 0 1 1 0
AB=10: 0 0 1 0
============================================================
CNF RESULT
Expression: ((C OR A) AND (C OR B) AND D)
Variables: ['A', 'B', 'C', 'D']
CD=00 CD=01 CD=11 CD=10
AB=00: 0 0 1 0
AB=01: 0 0 1 0
AB=11: 0 1 1 0
AB=10: 0 0 1 0
Original truth table (bitvec): 1010100010001000
CNF result truth table (bitvec): 1010100010001000
Logically equivalent: YES ✓
In CNF form: yes
Result: PASS
######################################################################
TEST CASE 8: T8: (A∧B∧C) ∨ (A∧B∧D)
######################################################################
============================================================
ORIGINAL: T8: (A∧B∧C) ∨ (A∧B∧D)
Expression: ((A AND B AND C) OR (A AND B AND D))
Variables: ['A', 'B', 'C', 'D']
CD=00 CD=01 CD=11 CD=10
AB=00: 0 0 0 0
AB=01: 0 0 0 0
AB=11: 0 1 1 1
AB=10: 0 0 0 0
============================================================
CNF RESULT
Expression: (A AND (A OR B) AND (A OR D) AND B AND (B OR D) AND (C OR A) AND (C OR B) AND (C OR D))
Variables: ['A', 'B', 'C', 'D']
CD=00 CD=01 CD=11 CD=10
AB=00: 0 0 0 0
AB=01: 0 0 0 0
AB=11: 0 1 1 1
AB=10: 0 0 0 0
Original truth table (bitvec): 1110000000000000
CNF result truth table (bitvec): 1110000000000000
Logically equivalent: YES ✓
In CNF form: yes
Result: PASS
######################################################################
TEST CASE 9: T9: (A∧B) ∨ (A∧C) ∨ (A∧D)
######################################################################
============================================================
ORIGINAL: T9: (A∧B) ∨ (A∧C) ∨ (A∧D)
Expression: ((A AND B) OR (A AND C) OR (A AND D))
Variables: ['A', 'B', 'C', 'D']
CD=00 CD=01 CD=11 CD=10
AB=00: 0 0 0 0
AB=01: 0 0 0 0
AB=11: 1 1 1 1
AB=10: 0 1 1 1
============================================================
CNF RESULT
Expression: (A AND (A OR D) AND (A OR C) AND (B OR A) AND (B OR C OR D))
Variables: ['A', 'B', 'C', 'D']
CD=00 CD=01 CD=11 CD=10
AB=00: 0 0 0 0
AB=01: 0 0 0 0
AB=11: 1 1 1 1
AB=10: 0 1 1 1
Original truth table (bitvec): 1111111000000000
CNF result truth table (bitvec): 1111111000000000
Logically equivalent: YES ✓
In CNF form: yes
Result: PASS
######################################################################
TEST CASE 10: T10: (A∧B∧C) ∨ (A∧B∧D) ∨ (¬A∧B∧C) ∨ (¬A∧B∧D)
######################################################################
============================================================
ORIGINAL: T10: (A∧B∧C) ∨ (A∧B∧D) ∨ (¬A∧B∧C) ∨ (¬A∧B∧D)
Expression: ((A AND B AND C) OR (A AND B AND D) OR (NOT(A) AND B AND C) OR (NOT(A) AND B AND D))
Variables: ['A', 'B', 'C', 'D']
CD=00 CD=01 CD=11 CD=10
AB=00: 0 0 0 0
AB=01: 0 1 1 1
AB=11: 0 1 1 1
AB=10: 0 0 0 0
============================================================
CNF RESULT
Expression: ((A OR NOT(A)) AND (A OR B) AND (B OR NOT(A)) AND B AND (B OR D) AND (B OR C) AND (C OR D))
Variables: ['A', 'B', 'C', 'D']
CD=00 CD=01 CD=11 CD=10
AB=00: 0 0 0 0
AB=01: 0 1 1 1
AB=11: 0 1 1 1
AB=10: 0 0 0 0
Original truth table (bitvec): 1110000011100000
CNF result truth table (bitvec): 1110000011100000
Logically equivalent: YES ✓
In CNF form: yes
Result: PASS
======================================================================
SUMMARY
======================================================================
# Status CNF? Description
--- ------ -------------------- ------------------------------
1 PASS yes T1: (A AND B) OR (A AND C)
3 PASS yes T3: (A∧B) ∨ (A∧C) ∨ (D∧B) ∨ (D∧C)
4 PASS yes T4: (A∨B) ∧ (C∨D) [already CNF]
5 PASS yes T5: A ∧ B ∧ C [trivial AND]
6 PASS yes T6: A ∨ B ∨ C [trivial OR]
7 PASS yes T7: ((A∧B) ∨ C) ∧ D
8 PASS yes T8: (A∧B∧C) ∨ (A∧B∧D)
9 PASS yes T9: (A∧B) ∨ (A∧C) ∨ (A∧D)
10 PASS yes T10: (A∧B∧C) ∨ (A∧B∧D) ∨ (¬A∧B∧C) ∨ (¬A∧B∧D)
==================================================
CNF Conversion Results:
==================================================
T1: (A AND (A OR C) AND (B OR A) AND (B OR C))
T3: ((A OR D) AND (B OR C))
T4: ((A OR B) AND (C OR D))
T5: (A AND B AND C)
T6: (A OR B OR C)
T7: ((C OR A) AND (C OR B) AND D)
T8: (A AND (A OR B) AND (A OR D) AND B AND (B OR D) AND (C OR A) AND (C OR B) AND (C OR D))
T9: (A AND (A OR D) AND (A OR C) AND (B OR A) AND (B OR C OR D))
T10: ((A OR NOT(A)) AND (A OR B) AND (B OR NOT(A)) AND B AND (B OR D) AND (B OR C) AND (C OR D))
```
Here the generated python code for check conversion:
```
#!/usr/bin/env python3
"""
Verification of convert_expr_to_cnf_complete() from prepqual.c
using truth tables (Karnaugh maps) to confirm logical equivalence.
We replicate the C algorithm in Python, apply it to 10 test predicates,
and compare input/output truth tables to verify correctness.
"""
import itertools
from dataclasses import dataclass
from typing import List, Optional, Set, Tuple, FrozenSet
from copy import deepcopy
# ─────────────────────────────────────────────────────────────────────
# Expression AST (mirrors BoolExpr / OpExpr from PostgreSQL)
# ─────────────────────────────────────────────────────────────────────
@dataclass(frozen=True)
class Var:
"""A boolean variable, e.g. Var('A')"""
name: str
def __repr__(self):
return self.name
@dataclass(frozen=True)
class NotExpr:
arg: object
def __repr__(self):
return f"NOT({self.arg})"
@dataclass(frozen=True)
class AndExpr:
args: tuple # frozen tuple for hashability
def __repr__(self):
return "(" + " AND ".join(str(a) for a in self.args) + ")"
@dataclass(frozen=True)
class OrExpr:
args: tuple
def __repr__(self):
return "(" + " OR ".join(str(a) for a in self.args) + ")"
@dataclass(frozen=True)
class Const:
value: bool
def __repr__(self):
return "TRUE" if self.value else "FALSE"
# ─────────────────────────────────────────────────────────────────────
# Helper constructors
# ─────────────────────────────────────────────────────────────────────
def AND(*args):
flat = []
for a in args:
if isinstance(a, AndExpr):
flat.extend(a.args)
else:
flat.append(a)
return AndExpr(tuple(flat))
def OR(*args):
flat = []
for a in args:
if isinstance(a, OrExpr):
flat.extend(a.args)
else:
flat.append(a)
return OrExpr(tuple(flat))
def NOT(a):
return NotExpr(a)
# ─────────────────────────────────────────────────────────────────────
# Evaluation
# ─────────────────────────────────────────────────────────────────────
def evaluate(expr, env: dict) -> bool:
if isinstance(expr, Var):
return env[expr.name]
if isinstance(expr, Const):
return expr.value
if isinstance(expr, NotExpr):
return not evaluate(expr.arg, env)
if isinstance(expr, AndExpr):
return all(evaluate(a, env) for a in expr.args)
if isinstance(expr, OrExpr):
return any(evaluate(a, env) for a in expr.args)
raise TypeError(f"Unknown expr type: {type(expr)}")
def collect_vars(expr) -> set:
if isinstance(expr, Var):
return {expr.name}
if isinstance(expr, Const):
return set()
if isinstance(expr, NotExpr):
return collect_vars(expr.arg)
if isinstance(expr, (AndExpr, OrExpr)):
s = set()
for a in expr.args:
s |= collect_vars(a)
return s
raise TypeError(f"Unknown expr type: {type(expr)}")
# ─────────────────────────────────────────────────────────────────────
# Truth table generation
# ─────────────────────────────────────────────────────────────────────
def truth_table(expr, var_names: list) -> list:
"""Return list of (assignment_dict, result_bool) for all combinations."""
rows = []
for vals in itertools.product([False, True], repeat=len(var_names)):
env = dict(zip(var_names, vals))
rows.append((env, evaluate(expr, env)))
return rows
def truth_table_bitvec(expr, var_names: list) -> int:
"""Compact truth table as a bitmask (bit i = row i result)."""
bv = 0
for i, vals in enumerate(itertools.product([False, True], repeat=len(var_names))):
env = dict(zip(var_names, vals))
if evaluate(expr, env):
bv |= (1 << i)
return bv
# ─────────────────────────────────────────────────────────────────────
# Karnaugh map display (2-4 variables)
# ─────────────────────────────────────────────────────────────────────
def print_karnaugh_map(expr, var_names: list, label: str = ""):
n = len(var_names)
tt = truth_table(expr, var_names)
print(f"\n{'=' * 60}")
if label:
print(f" {label}")
print(f" Expression: {expr}")
print(f" Variables: {var_names}")
if n == 2:
_print_kmap_2(tt, var_names)
elif n == 3:
_print_kmap_3(tt, var_names)
elif n == 4:
_print_kmap_4(tt, var_names)
else:
# Fallback: just print truth table
_print_truth_table(tt, var_names)
print()
def _val(tt, env_match):
for env, res in tt:
if all(env[k] == v for k, v in env_match.items()):
return '1' if res else '0'
return '?'
def _print_truth_table(tt, var_names):
header = " | ".join(f"{v:>5}" for v in var_names) + " | OUT"
print(f" {header}")
print(f" {'-' * len(header)}")
for env, res in tt:
row = " | ".join(f"{int(env[v]):>5}" for v in var_names)
print(f" {row} | {'1' if res else '0'}")
def _print_kmap_2(tt, vn):
"""2-var Karnaugh map: rows=vn[0], cols=vn[1]"""
print(f" {vn[1]}=0 {vn[1]}=1")
for r in [0, 1]:
vals = []
for c in [0, 1]:
vals.append(_val(tt, {vn[0]: bool(r), vn[1]: bool(c)}))
print(f" {vn[0]}={r}: {vals[0]} {vals[1]}")
def _print_kmap_3(tt, vn):
"""3-var Karnaugh map: rows=vn[0], cols=vn[1]vn[2] in Gray code"""
gray2 = [(0, 0), (0, 1), (1, 1), (1, 0)]
header = " " + " ".join(f"{vn[1]}{vn[2]}={b1}{b2}" for b1, b2 in gray2)
print(header)
for r in [0, 1]:
vals = []
for b1, b2 in gray2:
vals.append(_val(tt, {vn[0]: bool(r), vn[1]: bool(b1), vn[2]: bool(b2)}))
print(f" {vn[0]}={r}: " + " ".join(vals))
def _print_kmap_4(tt, vn):
"""4-var Karnaugh map: rows=vn[0]vn[1], cols=vn[2]vn[3] in Gray code"""
gray2 = [(0, 0), (0, 1), (1, 1), (1, 0)]
header = " " + " ".join(f"{vn[2]}{vn[3]}={b1}{b2}" for b1, b2 in gray2)
print(header)
for r1, r2 in gray2:
vals = []
for c1, c2 in gray2:
vals.append(_val(tt, {vn[0]: bool(r1), vn[1]: bool(r2),
vn[2]: bool(c1), vn[3]: bool(c2)}))
print(f" {vn[0]}{vn[1]}={r1}{r2}: " + " ".join(vals))
# ─────────────────────────────────────────────────────────────────────
# CNF conversion — faithful port of prepqual.c algorithm
# ─────────────────────────────────────────────────────────────────────
def is_or(e):
return isinstance(e, OrExpr)
def is_and(e):
return isinstance(e, AndExpr)
def remove_duplicates_in_list(clauses: list) -> list:
"""remove_duplicates_in_list() from prepqual.c"""
result = []
for c in clauses:
if c not in result:
result.append(c)
return result
def or_clause_subsumes(or1, or2) -> bool:
"""
or_clause_subsumes() from prepqual.c
(A OR B) subsumes (A OR B OR C) — we can keep the shorter one.
"""
if not is_or(or1) or not is_or(or2):
return False
for a1 in or1.args:
if a1 not in or2.args:
return False
return True
def remove_duplicate_and_subsumed_clauses(clauses: list) -> list:
"""remove_duplicate_and_subsumed_clauses() from prepqual.c"""
result = []
for clause in clauses:
keep = True
new_result = []
for existing in result:
if clause == existing:
keep = False
new_result.append(existing)
continue
if is_or(clause) and is_or(existing):
if or_clause_subsumes(existing, clause):
keep = False
new_result.append(existing)
continue
elif or_clause_subsumes(clause, existing):
# Current subsumes existing — drop existing, keep current
# NOTE: C code does break here, so it only removes one.
# We replicate that behavior.
continue # skip existing
else:
new_result.append(existing)
else:
new_result.append(existing)
result = new_result
if keep:
result.append(clause)
return result
def flatten_or_args_complete(args: list) -> list:
result = []
for a in args:
if is_or(a):
result.extend(flatten_or_args_complete(list(a.args)))
else:
result.append(a)
return remove_duplicates_in_list(result)
def flatten_and_args_complete(args: list) -> list:
result = []
for a in args:
if is_and(a):
result.extend(flatten_and_args_complete(list(a.args)))
else:
result.append(a)
return remove_duplicates_in_list(result)
def deduplicate_cnf_result(expr):
if not is_and(expr):
return expr
unique = remove_duplicate_and_subsumed_clauses(list(expr.args))
if len(unique) == 0:
return Const(True)
elif len(unique) == 1:
return unique[0]
else:
return AndExpr(tuple(unique))
def combine_cnf_clauses_complete(clauses: list):
if len(clauses) == 0:
return Const(True)
if len(clauses) == 1:
return clauses[0]
all_clauses = []
for c in clauses:
if is_and(c):
all_clauses.extend(list(c.args))
else:
all_clauses.append(c)
all_clauses = remove_duplicate_and_subsumed_clauses(all_clauses)
if len(all_clauses) == 0:
return Const(True)
elif len(all_clauses) == 1:
return all_clauses[0]
else:
return AndExpr(tuple(all_clauses))
def distribute_or_over_ands_complete(non_ands: list, and_clauses: list):
first_and = and_clauses[0]
first_and_args = remove_duplicates_in_list(list(first_and.args))
remaining_ands = and_clauses[1:]
base_args = remove_duplicates_in_list(non_ands) + remaining_ands
distributed = []
for subclause in first_and_args:
new_or_args = list(base_args) + [subclause]
new_or_args = remove_duplicates_in_list(new_or_args)
new_or = OrExpr(tuple(new_or_args))
cnf_or = convert_expr_to_cnf_complete(new_or)
distributed.append(cnf_or)
return combine_cnf_clauses_complete(distributed)
def convert_or_to_cnf_complete(expr):
or_args = []
for a in expr.args:
or_args.append(convert_expr_to_cnf_complete(a))
or_args = flatten_or_args_complete(or_args)
or_args = remove_duplicates_in_list(or_args)
and_clauses = []
non_and_clauses = []
has_and = False
for a in or_args:
if is_and(a):
and_clauses.append(a)
has_and = True
else:
non_and_clauses.append(a)
if not has_and:
if len(or_args) == 0:
return Const(True)
elif len(or_args) == 1:
return or_args[0]
else:
return OrExpr(tuple(or_args))
result = distribute_or_over_ands_complete(non_and_clauses, and_clauses)
return deduplicate_cnf_result(result)
def convert_and_to_cnf_complete(expr):
and_args = []
for a in expr.args:
and_args.append(convert_expr_to_cnf_complete(a))
and_args = flatten_and_args_complete(and_args)
and_args = remove_duplicates_in_list(and_args)
if len(and_args) == 0:
return Const(True)
elif len(and_args) == 1:
return and_args[0]
else:
return AndExpr(tuple(and_args))
def convert_expr_to_cnf_complete(expr):
"""Main entry — mirrors convert_expr_to_cnf_complete() from prepqual.c"""
if expr is None:
return None
if not is_or(expr) and not is_and(expr):
return expr
if is_or(expr):
return convert_or_to_cnf_complete(expr)
if is_and(expr):
return convert_and_to_cnf_complete(expr)
return expr
# ─────────────────────────────────────────────────────────────────────
# CNF validation helper
# ─────────────────────────────────────────────────────────────────────
def is_cnf(expr) -> bool:
"""Check if expression is in CNF: AND of (OR of literals)."""
if isinstance(expr, (Var, NotExpr, Const)):
return True # single literal is trivially CNF
if is_or(expr):
# All args must be literals (Var, NotExpr, Const)
return all(isinstance(a, (Var, NotExpr, Const)) for a in expr.args)
if is_and(expr):
for a in expr.args:
if is_and(a):
return False # nested AND — not flat
if is_or(a):
if not all(isinstance(x, (Var, NotExpr, Const)) for x in a.args):
return False
elif not isinstance(a, (Var, NotExpr, Const)):
return False
return True
return False
# ─────────────────────────────────────────────────────────────────────
# Define 10 test predicates
# ─────────────────────────────────────────────────────────────────────
A, B, C, D = Var('A'), Var('B'), Var('C'), Var('D')
test_cases = [
# 1. Simple: (A AND B) OR (A AND C)
# Expected CNF: A AND (B OR C)
(
"T1: (A AND B) OR (A AND C)",
OR(AND(A, B), AND(A, C)),
['A', 'B', 'C'],
),
# 2. From commit message: (s='s' AND year=2001) OR (s='s' AND year=2002)
# Using A=s='s', B=year=2001, C=year=2002
# Expected CNF: A AND (B OR C)
(
"T2: (A AND B) OR (A AND C) [commit example simplified]",
OR(AND(A, B), AND(A, C)),
['A', 'B', 'C'],
),
# 3. Four-term: (A AND B) OR (A AND C) OR (D AND B) OR (D AND C)
# Expected CNF: (A OR D) AND (B OR C)
(
"T3: (A∧B) ∨ (A∧C) ∨ (D∧B) ∨ (D∧C)",
OR(AND(A, B), AND(A, C), AND(D, B), AND(D, C)),
['A', 'B', 'C', 'D'],
),
# 4. Already in CNF: (A OR B) AND (C OR D)
# Should stay the same
(
"T4: (A∨B) ∧ (C∨D) [already CNF]",
AND(OR(A, B), OR(C, D)),
['A', 'B', 'C', 'D'],
),
# 5. Single AND: A AND B AND C
# Already CNF
(
"T5: A ∧ B ∧ C [trivial AND]",
AND(A, B, C),
['A', 'B', 'C'],
),
# 6. Single OR: A OR B OR C
# Already CNF (single clause)
(
"T6: A ∨ B ∨ C [trivial OR]",
OR(A, B, C),
['A', 'B', 'C'],
),
# 7. Nested: ((A AND B) OR C) AND D
# Expected CNF: (A OR C) AND (B OR C) AND D
(
"T7: ((A∧B) ∨ C) ∧ D",
AND(OR(AND(A, B), C), D),
['A', 'B', 'C', 'D'],
),
# 8. Complex from commit: (A AND B AND C) OR (A AND B AND D)
# Expected CNF: A AND B AND (C OR D)
(
"T8: (A∧B∧C) ∨ (A∧B∧D)",
OR(AND(A, B, C), AND(A, B, D)),
['A', 'B', 'C', 'D'],
),
# 9. Three-way OR with overlap:
# (A AND B) OR (A AND C) OR (A AND D)
# Expected CNF: A AND (B OR C OR D)
(
"T9: (A∧B) ∨ (A∧C) ∨ (A∧D)",
OR(AND(A, B), AND(A, C), AND(A, D)),
['A', 'B', 'C', 'D'],
),
# 10. Full commit example:
# (A AND B AND C) OR (A AND B AND D) OR (NOT(A) AND B AND C) OR (NOT(A) AND B AND D)
# = B AND (C OR D) (A cancels out)
(
"T10: (A∧B∧C) ∨ (A∧B∧D) ∨ (¬A∧B∧C) ∨ (¬A∧B∧D)",
OR(AND(A, B, C), AND(A, B, D), AND(NOT(A), B, C), AND(NOT(A), B, D)),
['A', 'B', 'C', 'D'],
),
]
# ─────────────────────────────────────────────────────────────────────
# Run verification
# ─────────────────────────────────────────────────────────────────────
def main():
print("=" * 70)
print(" VERIFICATION OF convert_expr_to_cnf_complete()")
print(" Using Karnaugh Maps / Truth Tables")
print("=" * 70)
all_pass = True
results_summary = []
for i, (label, expr, var_names) in enumerate(test_cases, 1):
print(f"\n{'#' * 70}")
print(f" TEST CASE {i}: {label}")
print(f"{'#' * 70}")
# Show original expression and its Karnaugh map
print_karnaugh_map(expr, var_names, f"ORIGINAL: {label}")
# Apply CNF conversion
cnf_expr = convert_expr_to_cnf_complete(expr)
# Show converted expression and its Karnaugh map
print_karnaugh_map(cnf_expr, var_names, f"CNF RESULT")
# Compare truth tables
orig_bv = truth_table_bitvec(expr, var_names)
cnf_bv = truth_table_bitvec(cnf_expr, var_names)
equivalent = (orig_bv == cnf_bv)
cnf_form = is_cnf(cnf_expr)
status = "PASS" if equivalent else "FAIL"
cnf_status = "yes" if cnf_form else "NO (not strict CNF)"
print(f" Original truth table (bitvec): {orig_bv:0{2**len(var_names)}b}")
print(f" CNF result truth table (bitvec): {cnf_bv:0{2**len(var_names)}b}")
print(f" Logically equivalent: {'YES ✓' if equivalent else 'NO ✗ MISMATCH!'}")
print(f" In CNF form: {cnf_status}")
print(f" Result: {status}")
if not equivalent:
all_pass = False
# Show differing rows
print("\n DIFFERING ROWS:")
tt_orig = truth_table(expr, var_names)
tt_cnf = truth_table(cnf_expr, var_names)
for (env_o, res_o), (env_c, res_c) in zip(tt_orig, tt_cnf):
if res_o != res_c:
assign = ", ".join(f"{v}={'1' if env_o[v] else '0'}" for v in var_names)
print(f" {assign}: original={int(res_o)}, cnf={int(res_c)}")
results_summary.append((i, label, status, cnf_status, str(cnf_expr)))
# ─────────────────────────────────────────────────────────────────
# Summary
# ─────────────────────────────────────────────────────────────────
print(f"\n\n{'=' * 70}")
print(" SUMMARY")
print(f"{'=' * 70}")
print(f" {'#':>3} {'Status':>6} {'CNF?':>20} Description")
print(f" {'-'*3} {'-'*6} {'-'*20} {'-'*30}")
for idx, label, status, cnf_status, cnf_str in results_summary:
print(f" {idx:>3} {status:>6} {cnf_status:>20} {label}")
print()
print(f" {'=' * 50}")
print(f" CNF Conversion Results:")
print(f" {'=' * 50}")
for idx, label, status, cnf_status, cnf_str in results_summary:
print(f" T{idx}: {cnf_str}")
print()
if all_pass:
print(" ✓ ALL 10 TEST CASES PASSED — CNF conversion is logically correct.")
else:
print(" ✗ SOME TEST CASES FAILED — CNF conversion has bugs!")
return all_pass
if __name__ == "__main__":
success = main()
exit(0 if success else 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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] Make PostgreSQL planner surpass ORCA on TPC-DS for the first time. [cloudberry]
Posted by "leborchuk (via GitHub)" <gi...@apache.org>.
leborchuk commented on PR #1762:
URL: https://github.com/apache/cloudberry/pull/1762#issuecomment-4519452328
Thanks! This is without a doubt a landmark work!
I agree with the approach. Yes, GPORCA contains a huge number of logical transformations, but not all of them lead to such changes that the query can be calculated much faster. Let's find out which of the transformations were particularly effective and we will implement them in the PostgreSQL optimizer. Then, the PostgreSQL optimizer will be no worse than GPORCA. Not always, but it can be improved iteratively. Also my experience shows that bottom-up optimizers are faster.
The work is so extensive that it takes time for a thorough review. At the same time, for the most part we are talking about logical transformations of queries, where the price of a mistake is not performance, but a wrong answer. So (take for example the first transformation - CTE Predicate Pushdown via OR Collection and CNF Conversion) I would like to read the original ACMSIGMOD article, its criticism and citations, look at the implementation and tests. It takes time.
Is it OK for you if we take **2 months** to review these changes? For my part, I can promise that we (various guys from our team) will provide details as soon as the review of individual changes is performed, rather than accumulating a list of comments.
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] Make PostgreSQL planner surpass ORCA on TPC-DS for the first time. [cloudberry]
Posted by "leborchuk (via GitHub)" <gi...@apache.org>.
leborchuk commented on PR #1762:
URL: https://github.com/apache/cloudberry/pull/1762#issuecomment-4810240769
### 2. Shared Scan Column Pruning
LGTM, while at the beginning, I was confused by the number of fixes needed to make it work. However, after thorough investigation and reading, I think that remapping columns using `change_varattnos_of_ShareInputScan()` is quite safe, and there is no need for additional GUC protection. We could simply fix all issues if there were only one left.
Initially I started with commit 54930251e4c but there is nothing it's preparing commit for further improvements
Commit | Date | Subject | Note
-- | -- | -- | --
54930251e4c | 2025-10-27 | Insert Result node atop CTE producer for column projection optimization | The message describes exactly this feature, but the actual diff does not implement Result-node insertion or column pruning. It only refactors Shared Scan predicate-pushdown control flow. Treat it as misleading / not the implementation commit.
The main work is done there
Commit | Date | Subject | Note
-- | -- | -- | --
abbb9469062 | 2025-11-28 | Implement Shared Scan column pruning. | This is the commit that actually implements the feature described: tracking used CTE consumer columns, building an attribute mapping, inserting a producer-side projection, and remapping consumer target lists.
The fixes in logic
Commit | Date | Subject | Note
-- | -- | -- | --
065983a1e69 | 2025-12-05 | Fix NULL check for CTE attribute map in setrefs | Fixes null handling around the CTE attribute map introduced by pruning.
a2711ef5ac9 | 2026-01-02 | Fix Shared Scan target list varattno adjustment | Fixes attribute-number remapping after pruning.
31c97bfee5a | 2026-01-02 | Fix ShareInputScan target list construction | Fixes target-list construction for Shared Scan after projection changes.
51b3f681a03 | 2026-01-03 | Fix logic for correcting Shared Scan target list varattno | Further fixes varattno correction logic.
fdd9e8fee2b | 2026-01-03 | Fix assertion for subquery unused column handling | Fixes assertion failures around unused/pruned columns.
b2cf7a4f0fb | 2026-01-08 | Handle whole-row references in Shared Scan projection | Handles cases like selecting the whole CTE row, where pruning cannot safely remove columns.
d8c98164156 | 2026-01-09 | Remove unnecessary nodes from SharedScan subquery | Cleans up unnecessary nodes after the Shared Scan projection work.
I didn't check all of them using gdb, so I cannot say that they actually fixed all the issues that were described. I just read the description and checked the logic and saw no issues. We gradually came to a solution when everything perform inside `change_varatnos_of_shareinputscan()` and so we just needed not to forget that `varatno == 0` was a special case for `select table.*` and that we needed to replace only references related to the shared CTE, not any matching references.
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] Make PostgreSQL planner surpass ORCA on TPC-DS for the first time. [cloudberry]
Posted by "my-ship-it (via GitHub)" <gi...@apache.org>.
my-ship-it commented on PR #1762:
URL: https://github.com/apache/cloudberry/pull/1762#issuecomment-4531040737
Nice job! LGTM
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] Make PostgreSQL planner surpass ORCA on TPC-DS for the first time. [cloudberry]
Posted by "avamingli (via GitHub)" <gi...@apache.org>.
avamingli commented on PR #1762:
URL: https://github.com/apache/cloudberry/pull/1762#issuecomment-4523852543
> nice job ! one question: Is this feature had tested large partitioned table ? any benchmark ?
No partitioned.
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] Make PostgreSQL planner surpass ORCA on TPC-DS for the first time. [cloudberry]
Posted by "IPetrov2013 (via GitHub)" <gi...@apache.org>.
IPetrov2013 commented on PR #1762:
URL: https://github.com/apache/cloudberry/pull/1762#issuecomment-4519014332
Nice work! This is a milestone contribution.
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] Make PostgreSQL planner surpass ORCA on TPC-DS for the first time. [cloudberry]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1762:
URL: https://github.com/apache/cloudberry/pull/1762#discussion_r3407895797
##########
src/include/utils/guc.h:
##########
@@ -247,6 +247,14 @@ typedef enum
#define GUC_GPDB_NEED_SYNC 0x00400000 /* guc value is synced between master and primary */
#define GUC_GPDB_NO_SYNC 0x00800000 /* guc value is not synced between master and primary */
+/*
+ * FIXEM: we have bad logic in get_explain_guc_options
+ * Even GUCs have no GUC_EXPLAIN flag, explain(verbose) still show them.
+ * It's a bug. However, there would be much more plan diffs if we fix it now.
+ * So introduce a temp fix flag to workaround for new added GUCs which are not showed in explain.
+ */
+#define GUC_NO_EXPLAIN 0x01000000 /* guc value is not synced between master and primary */
Review Comment:
The comment introducing GUC_NO_EXPLAIN contains a typo ("FIXEM") and the macro comment is a copy/paste mismatch (it says "not synced" rather than "not shown in EXPLAIN"). This makes it harder to understand what the flag actually does.
##########
src/backend/utils/misc/guc_gp.c:
##########
@@ -1883,8 +1893,7 @@ struct config_bool ConfigureNamesBool_gp[] =
{
{"gp_eager_two_phase_agg", PGC_USERSET, QUERY_TUNING_METHOD,
gettext_noop("Eager two stage agg."),
- NULL,
- GUC_NO_SHOW_ALL | GUC_NOT_IN_SAMPLE
+ NULL
},
Review Comment:
gp_eager_two_phase_agg no longer has the GUC_NO_SHOW_ALL | GUC_NOT_IN_SAMPLE flags, unlike nearby internal tuning GUCs. This changes user-visible behavior (SHOW ALL / sample configs) and is inconsistent with the surrounding conventions.
##########
src/backend/optimizer/util/clauses.c:
##########
@@ -849,7 +849,7 @@ max_parallel_hazard_walker(Node *node, max_parallel_hazard_context *context)
List *save_safe_param_ids;
if (!subplan->parallel_safe &&
- max_parallel_hazard_test(PROPARALLEL_RESTRICTED, context))
+ max_parallel_hazard_test(PROPARALLEL_SAFE, context))
return true;
Review Comment:
In max_parallel_hazard_walker(), this check now uses PROPARALLEL_SAFE, which flips the meaning: it will report a hazard only when the caller is scanning for SAFE hazards, instead of when scanning for RESTRICTED hazards. This can let non-parallel-safe SubPlans be treated as safe/restricted incorrectly during planning.
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] Make PostgreSQL planner surpass ORCA on TPC-DS for the first time. [cloudberry]
Posted by "leborchuk (via GitHub)" <gi...@apache.org>.
leborchuk commented on PR #1762:
URL: https://github.com/apache/cloudberry/pull/1762#issuecomment-4876762835
### 3. Sublink-to-Join Conversion for Nested Arithmetic Expressions
tl;dr - let's make an exception for functions - do not transform expressions contains user functions, maybe except IMMUTABLE or STABLE one - for discussion.
#### The whole idea discussion
The whole idea is simple, I agree with it, why not to unnest the whole expression, why we limit transformation for a simple sentences?
The search through pg hackers shows that Heike already tried to do the same in PG:
Heikki Linnakangas, May 2017 [Pulling up more complicated subqueries](https://www.postgresql.org/message-id/67e353e8-c20e-7980-a282-538779edf25b@iki.fi)
He uses TPC-DS Q6 as the motivating example:
```
SELECT * FROM foo
WHERE foo.j >= 1.2 * (SELECT avg(bar.j) FROM bar WHERE foo.i = bar.i);
```
Key quote:
> "The planner can pull up simpler subqueries, converting them to joins, but unfortunately this case is beyond its capabilities."
He proposes the manual rewrite:
```
SELECT * FROM foo
LEFT JOIN (SELECT avg(bar.j) AS avg, bar.i FROM bar GROUP BY bar.i) AS avg_bar
ON foo.i = avg_bar.i
WHERE foo.j >= 1.2 * avg_bar.avg;
```
And propose a multi-step roadmap. The issue is that never fully landed in core )
We could ask Heike why so )) , but he cited Tom Lane from the 2011's thread
> "Thinking of it as a pull-up or push-down transformation is the wrong approach because those sorts of transformations are done too early to be able to use cost comparisons."
But! We have https://github.com/apache/cloudberry/commit/dcdc6c0b34bd7184f780f6e2f79f337937435476 commited by Heike ) It's remind me Surajit Chaudhuri idea to move aggregation through the join tree https://vldb.org/conf/1994/P354.PDF
There is one remaining part to move - perform unnesting. Let's do it )
#### Implementation discussion
I've made a test:
```
create table test(id int, price float, category int);
CREATE OR REPLACE FUNCTION increment(f float) RETURNS float AS $$
BEGIN
RETURN f + 1;
END;
$$ LANGUAGE plpgsql;
CREATE FUNCTION
postgres=# explain verbose select *
from test i
where i.price > 12 + 1.2 *
(select increment(avg(j.price))
from test j
where j.category = i.category);
QUERY PLAN
------------------------------------------------------------------------------------------------------------------------------
Gather Motion 3:1 (slice1; segments: 3) (cost=541.17..1269.50 rows=23700 width=16)
Output: i.id, i.price, i.category
-> Hash Join (cost=541.17..953.50 rows=7900 width=16)
Output: i.id, i.price, i.category
Inner Unique: true
Hash Cond: (i.category = "Expr_SUBQUERY".csq_c0)
Join Filter: (i.price > ('12'::double precision + ('1.2'::double precision * "Expr_SUBQUERY".csq_c1)))
-> Seq Scan on public.test i (cost=0.00..271.00 rows=23700 width=16)
Output: i.id, i.price, i.category
-> Hash (cost=528.67..528.67 rows=1000 width=12)
Output: "Expr_SUBQUERY".csq_c1, "Expr_SUBQUERY".csq_c0
-> Broadcast Motion 3:3 (slice2; segments: 3) (cost=424.50..528.67 rows=1000 width=12)
Output: "Expr_SUBQUERY".csq_c1, "Expr_SUBQUERY".csq_c0
-> Subquery Scan on "Expr_SUBQUERY" (cost=424.50..515.33 rows=333 width=12)
Output: "Expr_SUBQUERY".csq_c1, "Expr_SUBQUERY".csq_c0
-> Finalize HashAggregate (cost=424.50..512.00 rows=333 width=12)
Output: j.category, increment(avg(j.price))
Group Key: j.category
-> Redistribute Motion 3:3 (slice3; segments: 3) (cost=389.50..419.50 rows=1000 width=36)
Output: j.category, (PARTIAL avg(j.price))
Hash Key: j.category
-> Streaming Partial HashAggregate (cost=389.50..399.50 rows=1000 width=36)
Output: j.category, PARTIAL avg(j.price)
Group Key: j.category
-> Seq Scan on public.test j (cost=0.00..271.00 rows=23700 width=12)
Output: j.id, j.price, j.category
Settings: optimizer = 'off'
Optimizer: Postgres query optimizer
(28 rows)
```
Here you could see that we perform `increment` for the whole dataset and only then join. But we do not know what actually `increment` does, it could contain a cumbersome users logic and . So we cannot move it somewhere and function should be left in place. Except maybe `IMMUTABLE` or `STABLE` one. My experience tells me they are safe and could be moved.
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] Make PostgreSQL planner surpass ORCA on TPC-DS for the first time. [cloudberry]
Posted by "yjhjstz (via GitHub)" <gi...@apache.org>.
yjhjstz commented on PR #1762:
URL: https://github.com/apache/cloudberry/pull/1762#issuecomment-4519311868
nice job !
one question: Is this feature had tested large partitioned table ? any benchmark ?
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] Make PostgreSQL planner surpass ORCA on TPC-DS for the first time. [cloudberry]
Posted by "tuhaihe (via GitHub)" <gi...@apache.org>.
tuhaihe commented on PR #1762:
URL: https://github.com/apache/cloudberry/pull/1762#issuecomment-5201297872
Hi @avamingli, would you like to continue helping resolve the conflicts and improve this PR? If not, I think we can create a new series of PRs if someone is interested in this work.
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] Make PostgreSQL planner surpass ORCA on TPC-DS for the first time. [cloudberry]
Posted by "avamingli (via GitHub)" <gi...@apache.org>.
avamingli commented on PR #1762:
URL: https://github.com/apache/cloudberry/pull/1762#issuecomment-5448086971
> Hi @avamingli, would you like to continue helping resolve the conflicts and improve this PR? If not, I think we can create a new series of PRs if someone is interested in this work.
To be equally direct on process: I was surprised to see this forked into #1932 while this PR was open. I understand the intent after three weeks without a reply from me, but I'd have expected a heads-up to the author before a parallel series was started. Let's not repeat that pattern for the remaining optimizations.
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] Make PostgreSQL planner surpass ORCA on TPC-DS for the first time. [cloudberry]
Posted by "tuhaihe (via GitHub)" <gi...@apache.org>.
tuhaihe commented on PR #1762:
URL: https://github.com/apache/cloudberry/pull/1762#issuecomment-4531427380
Cool, great work!
This PR is indeed a huge one. Let's not rush to merge it. We can review and collect feedback from more community members in the following 1~2 weeks.
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] Make PostgreSQL planner surpass ORCA on TPC-DS for the first time. [cloudberry]
Posted by "avamingli (via GitHub)" <gi...@apache.org>.
avamingli commented on PR #1762:
URL: https://github.com/apache/cloudberry/pull/1762#issuecomment-4539709031
> > One timing constraint I should flag: @chenjinbao1989 is preparing the PostgreSQL 14 → 16 kernel upgrade #1760 (5700+ commits touching the planner). We have discussed about that rebasing this PR across that would be impractical, so landing it first is really the only realistic path. Not sure for the timing, but review doesn't have to end at merge. I'd genuinely welcome the team continuing to dig in afterwards, bugs found later are still bugs found.
>
> Ok, got it. We're going to merge PostgreSQL 14 → 16 kernel upgrade #1760 in two weeks if there are no objections. So, no two months. In one week. I will try my best. You are right, we can continue digging after merge. The main head is here with all the bugs and features. It is important to have time to check before the release.
Hi,
A quick update on #1760. Jinbao and I talked it through, and we agree the kernel upgrade should take priority — see the background discussion here: https://lists.apache.org/thread/pmwp8v6zg7ds1jg4r9lttkoojhjmxy11
On timing: the two-month window originally proposed is fine with me — there's no need to rush this. Once the kernel lands, plans and performance baselines will shift anyway, and re-establishing them will take real time, so please review at whatever depth is useful.
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] Make PostgreSQL planner surpass ORCA on TPC-DS for the first time. [cloudberry]
Posted by "avamingli (via GitHub)" <gi...@apache.org>.
avamingli commented on PR #1762:
URL: https://github.com/apache/cloudberry/pull/1762#issuecomment-4818483941
> ### 1. CTE Predicate Pushdown via OR Collection and CNF Conversion
> Checked the 1st commit [7f13fc4](https://github.com/apache/cloudberry/commit/7f13fc4fa537b7a96cbb156c74c719eb45092d5a) for correctness - all Ok, the code is correct.
>
> The only flaw is in example T10 we have (A OR NOT(A)) predicate - could exclude it, but I think it's not the current implementation task - exclude tautology.
>
> What I checked. The focus was on function `convert_expr_to_cnf_complete`. We transform logical expression to simplified form, lets check the original equation and transformed one. The only known for me method is https://en.wikipedia.org/wiki/Karnaugh_map So we have the original expression, lets create Karnaugh map for it, then transform expression, again create Karnaugh map, compare maps, and also check if the transformed equation the same we could generate using rules for Karnaugh map.
>
> To do so I generate python code based on `prepqual.c`, manually checked generated code for correctness, and then launch code and see the results.
@leborchuk Thank you for the rigorous review and the Karnaugh-map verification!
Really appreciate the depth here.
The conversion is verified in 2-valued logic, which is exactly right.
The reason I deliberately keep `(A OR NOT A)` instead of eliminating it as a tautology is SQL's 3-valued logic: `A OR NOT A` is **not** always TRUE — when `A` is NULL it evaluates to NULL. And here `A` is a general operator expression, not a single column, so I can't fall back on a column-level NOT NULL constraint to recover the law of excluded middle. Eliminating it would only be safe under a *proven* NOT NULL on `A`, which for a general expression I don't have. (PostgreSQL's own const-folding keeps it for the same reason.)
A quick reproduction anyone can run:
```sql
-- ============================================================
-- (A OR NOT A) is NOT always TRUE in SQL's 3-valued logic,
-- so eliminating it as a tautology can change query results.
-- ============================================================
-- Part 1: truth table of (A OR NOT A) for a boolean A, including NULL
SELECT a,
(a OR NOT a) AS "a OR NOT a",
(a OR NOT a) IS TRUE AS "passes a WHERE?"
FROM (VALUES (true), (false), (NULL::bool)) v(a);
-- Part 2: effect on a real query result.
-- A is a general operator expression (x > 5), NOT a bare nullable column.
DROP TABLE IF EXISTS aornota_demo;
CREATE TABLE aornota_demo (id int, x int, b bool);
INSERT INTO aornota_demo VALUES (1, 10, true), -- A = (x>5) = TRUE
(2, NULL, true), -- A = (x>
(3, 3, true); -- A = (x>5) = FALSE
-- (a) keep the tautology clause (what the CNF conversion produces):
SELECT count(*) AS kept_A_OR_NOTA
FROM aornota_demo
WHERE ((x > 5) OR NOT (x > 5)) AND b;
-- (b) drop it as a "tautology" (the suggested simplification)
SELECT count(*) AS dropped
FROM aornota_demo
WHERE b;
-- per-row view: row id=2 (x IS NULL) flips reject -> accept
SELECT id, x,
((x > 5) OR NOT (x > 5)) AS "A OR NOT A",
(((x>5) OR NOT (x>5)) AND b) IS TRUE AS kept_passes,
(b) IS TRUE AS dropped_passes
FROM aornota_demo ORDER BY id;
DROP TABLE aornota_demo;
```
Output (standard SQL 3-valued logic):
```
a | a OR NOT a | passes a WHERE?
---+------------+-----------------
t | t | t
f | t | t
| | f <- A IS NULL: (A OR NOT A) is NUL
kept_a_or_nota -- WHERE ((x>5) OR NOT(x>5)) AND b
----------------
2
dropped -- WHERE b (tautology eliminated)
---------
3
id | x | A OR NOT A | kept_passes | dropped_passes
----+----+------------+-------------+----------------
1 | 10 | t | t | t
2 | | | f | t <- flips reject -> accept
3 | 3 | t | t | t
```
2 rows vs 3 rows: the `x IS NULL` row flips from rejected to accepted once the
clause is dropped. So that clause isn't redundant — it carries
predicate's NULL-rejection into the CNF.
Side note: I actually tried a Karnaugh-map based approach during development but
dropped it — every nested step had to allocate a lot of space
mostly discarded during dedup, so I went with the recursive distributive-law
method instead. Your verification on top of it is a great cross-check. Thanks again!
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] Make PostgreSQL planner surpass ORCA on TPC-DS for the first time. [cloudberry]
Posted by "Alena0704 (via GitHub)" <gi...@apache.org>.
Alena0704 commented on PR #1762:
URL: https://github.com/apache/cloudberry/pull/1762#issuecomment-5449810394
> > Hi @avamingli, would you like to continue helping resolve the conflicts and improve this PR? If not, I think we can create a new series of PRs if someone is interested in this work.
>
> To be equally direct on process: I was surprised to see this forked into #1932 while this PR was open. I understand the intent after three weeks without a reply from me, but I'd have expected a heads-up to the author before a parallel series was started. Let's not repeat that pattern for the remaining optimizations.
I've decided to close it, but you'd better solve the review-complexity and rebase problems here 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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] Make PostgreSQL planner surpass ORCA on TPC-DS for the first time. [cloudberry]
Posted by "avamingli (via GitHub)" <gi...@apache.org>.
avamingli commented on PR #1762:
URL: https://github.com/apache/cloudberry/pull/1762#issuecomment-4523937319
> Thanks! This is without a doubt a landmark work!
Thanks.
> I agree with the approach. Yes, GPORCA contains a huge number of logical transformations, but not all of them lead to such changes that the query can be calculated much faster. Let's find out which of the transformations were particularly effective and we will implement them in the PostgreSQL optimizer. Then, the PostgreSQL optimizer will be no worse than GPORCA. Not always, but it can be improved iteratively. Also my experience shows that bottom-up optimizers are faster.
Agreed. ORCA's optimizer is routinely 100x+ slower than PG's. My take after this work is that ORCA's real edge was never the architecture, it was the features — and once the equivalent features land in PG (CTE handling being the clearest example), PG comes out ahead. Long term, gradually retiring ORCA and consolidating on PG is the right direction.
> The work is so extensive that it takes time for a thorough review. At the same time, for the most part we are talking about logical transformations of queries, where the price of a mistake is not performance, but a wrong answer. So (take for example the first transformation - CTE Predicate Pushdown via OR Collection and CNF Conversion) I would like to read the original ACMSIGMOD article, its criticism and citations, look at the implementation and tests. It takes time.
Agree correctness matters more than speed here.A few things that I hope make an earlier merge less risky:Every transformation here is grounded in techniques ORCA has used in Greenplum production for years. The theory isn't new. What's new is bringing it into the PG planner.Results were verified by diffing against ORCA across all 99 queries, on top of the regression suite.
>
> Is it OK for you if we take **2 months** to review these changes? For my part, I can promise that we (various guys from our team) will provide details as soon as the review of individual changes is performed, rather than accumulating a list of comments.
One timing constraint I should flag: @chenjinbao1989 is preparing the PostgreSQL 14 → 16 kernel upgrade https://github.com/apache/cloudberry/pull/1760 (5700+ commits touching the planner). We have discussed about that rebasing this PR across that would be impractical, so landing it first is really the only realistic path. Not sure for the timing, but review doesn't have to end at merge. I'd genuinely welcome the team continuing to dig in afterwards, bugs found later are still bugs found.
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] Make PostgreSQL planner surpass ORCA on TPC-DS for the first time. [cloudberry]
Posted by "leborchuk (via GitHub)" <gi...@apache.org>.
leborchuk commented on PR #1762:
URL: https://github.com/apache/cloudberry/pull/1762#issuecomment-4526480200
>
> One timing constraint I should flag: @chenjinbao1989 is preparing the PostgreSQL 14 → 16 kernel upgrade #1760 (5700+ commits touching the planner). We have discussed about that rebasing this PR across that would be impractical, so landing it first is really the only realistic path. Not sure for the timing, but review doesn't have to end at merge. I'd genuinely welcome the team continuing to dig in afterwards, bugs found later are still bugs found.
Ok, got it. We're going to merge PostgreSQL 14 → 16 kernel upgrade #1760 in two weeks if there are no objections. So, no two months. In one week. I will try my best. You are right, we can continue digging after merge. The main head is here with all the bugs and features. It is important to have time to check before the release.
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] Make PostgreSQL planner surpass ORCA on TPC-DS for the first time. [cloudberry]
Posted by "avamingli (via GitHub)" <gi...@apache.org>.
avamingli commented on PR #1762:
URL: https://github.com/apache/cloudberry/pull/1762#issuecomment-5466780919
> Except maybe `IMMUTABLE` or `STABLE` one. My experience tells me they are safe and could be moved.
https://github.com/apache/cloudberry/pull/1932#issuecomment-5466752293
> > > Hi @avamingli, would you like to continue helping resolve the conflicts and improve this PR? If not, I think we can create a new series of PRs if someone is interested in this work.
> >
> >
> > To be equally direct on process: I was surprised to see this forked into #1932 while this PR was open. I understand the intent after three weeks without a reply from me, but I'd have expected a heads-up to the author before a parallel series was started. Let's not repeat that pattern for the remaining optimizations.
>
> I've decided to close it, but you'd better solve the review-complexity and rebase problems here somehow.
Responded on 1932 to the substantive points you raised there: https://github.com/apache/cloudberry/pull/1932#issuecomment-5466752293.
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] Make PostgreSQL planner surpass ORCA on TPC-DS for the first time. [cloudberry]
Posted by "avamingli (via GitHub)" <gi...@apache.org>.
avamingli commented on PR #1762:
URL: https://github.com/apache/cloudberry/pull/1762#issuecomment-5466975477
> The solution reminds me Surajit Chaudhuri idea to move aggregation through the join tree https://vldb.org/conf/1994/P354.PDF
thanks, will study it when I get a chance
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] Make PostgreSQL planner surpass ORCA on TPC-DS for the first time. [cloudberry]
Posted by "yjhjstz (via GitHub)" <gi...@apache.org>.
yjhjstz commented on code in PR #1762:
URL: https://github.com/apache/cloudberry/pull/1762#discussion_r3510064155
##########
src/backend/optimizer/prep/prepqual.c:
##########
@@ -1037,13 +1112,27 @@ remove_duplicate_and_subsumed_clauses(List *clauses)
}
else if (or_clause_subsumes(clause, existing))
{
- /* Current subsumes existing, remove existing */
- result = list_delete_cell(result, lc_exist);
- break;
+ /*
+ * Current subsumes existing, mark for removal.
+ * Continue checking other clauses since the current
+ * clause may subsume multiple existing clauses.
+ */
+ to_remove = lappend(to_remove, existing);
Review Comment:
break or not？
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org