Skip to content

Retry aggregate-diff review experiment for apache/doris#64080 - #138

Closed
zclllyybb wants to merge 1 commit into
codex-aggregate-64080-base-20260710from
codex-retry-64080-head-20260711
Closed

zclllyybb wants to merge 1 commit into
codex-aggregate-64080-base-20260710from
codex-retry-64080-head-20260711

Conversation

@zclllyybb

Copy link
Copy Markdown
Owner

Fresh replay PR for validating the current aggregate-diff review workflow. The base and head reproduce the referenced Apache Doris PR snapshot.

### What problem does this PR solve?

Issue Number: close #xxx

Related PR: #xxx

Problem Summary: PushProjectThroughUnion allowed cast projections to be pushed below UNION DISTINCT. For non-injective casts, distinct was then evaluated after the cast instead of before the cast, which could incorrectly collapse rows that were distinct in the original union output. Restrict cast project pushdown to UNION ALL and add tests for the qualifier guard and the DORIS-26135 datetime-to-date regression case.

### Release note

None

### Check List (For Author)

- Test: Regression test / Unit Test
    - ./run-fe-ut.sh --run org.apache.doris.nereids.rules.rewrite.PushProjectThroughUnionTest
    - ./run-regression-test.sh --run -d nereids_syntax_p0 -s set_operation
    - mvn checkstyle:check -pl fe-core
- Behavior changed: No
- Does this need documentation: No

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review result: request changes.

Critical checkpoint conclusions:

  • Goal and behavior: the PR fixes the non-injective cast case for UNION DISTINCT and the added datetime-to-date regression captures that path, but the same rule still allows a reachable non-injective bare-slot projection.
  • Scope and clarity: the change is narrow, but the safety check is incomplete for DISTINCT set operations because it only guards cast aliases.
  • Parallel paths: PushProjectThroughUnion applies through LogicalSetOperation/ProjectProcessor, and the duplicate bare-slot issue is the same root cause for DISTINCT set operations; unsupported INTERSECT ALL / EXCEPT ALL is rejected during binding, so that risk was dismissed.
  • Tests: the new unit/regression tests cover cast aliases, but they do not cover duplicate slot projections such as SELECT a, a over a distinct set operation.
  • Concurrency, lifecycle, config, persistence, FE/BE protocol, observability: not involved in this PR.

No builds or tests were run because this review environment is instructed to perform review-only operations.

@@ -59,10 +60,11 @@ public static boolean canPushProject(List<NamedExpression> projects, LogicalSetO
return projects.size() == logicalSetOperation.getOutput().size() && projects.stream().allMatch(e -> {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still allows a non-injective projection through UNION DISTINCT when the projection is made only of bare slots. For example, SELECT a, a FROM (SELECT 1 a, 2 b UNION SELECT 1, 3) t should run the distinct union on (a,b) first, preserving two rows, then return two identical (1,1) rows. If this rule pushes [a, a] into both union children, the union sees (1,1) from both sides and can collapse them to one row. For DISTINCT set operations, the slot-only path also needs to prove the projection is one-to-one over the set-operation outputs, not just reject cast aliases.

zclllyybb added a commit to apache/doris that referenced this pull request Jul 12, 2026
…ability (#65178)

The Codex GitHub Actions review pipeline needed stronger guarantees that
agents review the intended PR snapshot, use one authoritative diff,
pursue high-risk cross-module mechanisms, and expose the work of spawned
subagents in Litefuse.

This PR:

- verifies the checked-out PR head and rechecks the live base/head after
context preparation;
- prepares an authoritative aggregate PR patch and changed-file list,
with atomic bounded retry (`2s`, `4s`, `8s`) for transient GitHub API
failures;
- directs the main agent to perform an initial risk scan, dispatch
focused subagents for suspicious mechanisms, maintain a shared
append-only ledger, validate/deduplicate every candidate, and converge
for at most three rounds;
- requires paths outside the authoritative diff/changed-file context to
be confirmed with `rg --files` before reading;
- records separate Litefuse traces for each Codex subagent session,
including command/tool I/O and non-empty encrypted-reasoning status,
instead of mixing subagent activity into the main trace;
- makes Litefuse ingestion more resilient to transport timeouts while
keeping telemetry upload non-blocking for the review outcome.

The repository review skill is kept focused on review invariants;
orchestration and completion policy live in the workflow prompt, where
they are available to the GitHub Actions invocation.

### Validation and experiment material

- Static checks: `python3 -m py_compile
.github/scripts/emit_litefuse_otel_io.py` and `git diff --check`.
- Litefuse uploader validation: real/synthetic subagent session
conversion, payload-limit coverage, and readback of independent subagent
traces.
- End-to-end replayed review experiments against fresh fork PRs built
from the original PR base/head commits:

| Replay PR | GitHub Actions run | Result |
| --- | --- | --- |
| [#136](zclllyybb#136) |
[29112823062](https://github.com/zclllyybb/doris/actions/runs/29112823062)
| Success; 2 inline comments |
| [#137](zclllyybb#137) |
[29114260631](https://github.com/zclllyybb/doris/actions/runs/29114260631)
| Success; 3 inline comments |
| [#138](zclllyybb#138) |
[29115946034](https://github.com/zclllyybb/doris/actions/runs/29115946034)
| Success; 1 inline comment |
| [#139](zclllyybb#139) |
[29150372758](https://github.com/zclllyybb/doris/actions/runs/29150372758)
| Success; 1 inline comment |
| [#140](zclllyybb#140) |
[29151057520](https://github.com/zclllyybb/doris/actions/runs/29151057520)
| Success; 1 inline comment |

The latest replay trace, `86be226391a40e5992bb8d3b4e125eca`, confirms
one authoritative `pr.diff` read, two changed-file-list reads, no `gh pr
diff` or `git diff` calls from the reviewing agent, and separate
subagent sessions. It also exposed one remaining prompt-compliance gap:
a guessed adjacent `.cpp` path was read without a preceding `rg --files`
lookup; this is documented test evidence rather than hidden by the
validation.
@zclllyybb
zclllyybb deleted the branch codex-aggregate-64080-base-20260710 July 13, 2026 00:31
@zclllyybb zclllyybb closed this Jul 13, 2026
@zclllyybb
zclllyybb deleted the codex-retry-64080-head-20260711 branch July 13, 2026 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants