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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
