avamingli commented on PR #1932: URL: https://github.com/apache/cloudberry/pull/1932#issuecomment-5466752293
> > The original PR is still open, see my comments: [#1762 (comment)](https://github.com/apache/cloudberry/pull/1762#issuecomment-5448086971) Suggest holding this PR to avoid divergence. > > Fair point on the heads-up - that's on me, I should have pinged you before opening #1932. With no reply on #1762 for weeks, I assumed the PR had been left behind, and I didn't want the work to go to waste. It wasn't meant to take the work over: authorship is yours, the six original commits are kept separate and attributed, changed only where the PG16 rebase required it. > > I'm not ignoring your PR - I'm trying to get your ideas committed. Two things prevent that. First, the PR is very large, 98 commits, so it's very difficult to review and check that everything is fine. I only took the first point, and already there I found bugs, which I then fixed, plus a build issue in the form of a complaining compiler. One of those bugs is still present in your branch: quals are pushed into a CTE that contains volatile functions, while the inlined path in the same function refuses to do that. WITH x AS (SELECT a, nextval('ts') FROM vol_test) SELECT * FROM x WHERE a = 3 evaluates nextval() once instead of once per row, so the value of the CTE's column changes. Another one only shows up once point #1 is taken on its own: rows are lost when one CTE reference has no pushdown-safe qual, because the guard for that sits in [5493025](https://github.com/apache/cloudberry/commit/54930251e4cc894fe5d29283f3009b3ce8e84f0f), which belongs to point #2. Same story with the ex pected files - tpcds_q04.out and the singlenode copy of subselect.out are only updated at the very end of the series, in [81d2f57](https://github.com/apache/cloudberry/commit/81d2f57e4d3a51248a99abd0403c3d4b8827962c) and [b1f84b8](https://github.com/apache/cloudberry/commit/b1f84b8902ede6131ba5ccc01327de66bccf7488), and the pax copy isn't updated anywhere, so point #1 on its own leaves them stale. I've only just noticed some of your own later fixes (described above). > > Second, all the commits have fallen behind the main branch and need rebasing. I can't do this in your branch, which is why this PR appeared. Without resolving these issues, we'll never push anything. > > As I see you want to rebase and split #1762 yourself, so I close #1932 - you can take these five commits if you want. You should fix somehow problem above. On the two technical items: Volatile functions in the CTE OR-pushdown — isn't this the same class of concern @leborchuk raised on the sublink-to-join path? https://github.com/apache/cloudberry/pull/1762#issuecomment-4876762835 > Except maybe IMMUTABLE or STABLE one. My experience tells me they are safe and could be moved. Row loss when point #1 is taken alone, and stale expected files at intermediate commits — aren't these observations about a partial slice of the branch rather than the branch itself? I addressed the review methodology earlier: https://github.com/apache/cloudberry/pull/1762#issuecomment-4818492376 . > 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. Later commits in this series are deliberate fixes for issues introduced or surfaced by earlier ones; asking each intermediate to be correct in isolation will keep producing that list. End-to-end (branch HEAD) is the reviewable unit here. On the rebase behind main: the divergence isn't drift. When #1760 (PG14→16 kernel upgrade, 5700+ planner-touching commits) was landing, I explicitly deferred this PR so the kernel upgrade could go in first > A quick update on https://github.com/apache/cloudberry/pull/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 see https://github.com/apache/cloudberry/pull/1762#issuecomment-4539709031. -- 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]
