Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
d23c224 to
c0fe46d
Compare
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Static review of exact head c0fe46df32ebe1aaaff325d230eb9fb6c3ba14eb against base 5799884109545ec74e2858bf01eab1d926bdae33: requesting changes for three P2 correctness issues.
- The score-specific overflow fallback now loses the intended diagnostic and contradicts an existing regression oracle.
- The new nested-Limit merge exception rejects individually valid inputs whose result is provably empty, including the equivalent Exchange fold.
- The new compatible-TopN merge exception has the same optimizer-induced rejection on a distinct ordered-query path.
I reviewed all 24 changed files plus the relevant rewrite ordering, implementation/translation paths, downstream sort/exchange behavior, constructor invariants, specialized score/vector paths, and regression coverage. Three bounded normal/risk review rounds converged on this exact comment set; all final-round lanes returned NO_NEW_VALUABLE_FINDINGS, and the live inline-review context contains no duplicates. Other changed overflow guards and pushdown fallbacks did not yield another substantiated PR-introduced issue.
Validation was static-only as required: no build or test was run. git diff --check passed. The existing score regression was inspected but not executed, and new coverage does not exercise the two nested-merge boundaries. No additional user-provided review focus was present.
| // unmaterialized and reporting an unrelated score() usage error. | ||
| if (topN.getLimit() > Long.MAX_VALUE - topN.getOffset()) { | ||
| throw new AnalysisException("limit + offset overflows the long range"); | ||
| // negative value; skip the push-down and let the TopN above the scan apply limit/offset. |
There was a problem hiding this comment.
[P2] Preserve the overflow error for score() queries
Returning null here leaves score() unmaterialized. The same rewrite batch immediately runs CheckScoreUsage, so a query that has MATCH, ORDER BY, and LIMIT now fails with the unrelated "score() function requires ..." message before LogicalTopNToPhysicalTopN can report the overflow. This also contradicts the existing regression at test_search_score_topn_predicates.groovy:191-201, and the base version of this hunk explicitly threw the standard overflow error to avoid exactly this path. Please keep the fail-fast AnalysisException here (or validate overflow before CheckScoreUsage).
| * and an overflowing combined offset cannot be represented as a single offset, so fail fast | ||
| * instead of wrapping to a negative offset. | ||
| */ | ||
| public static long mergeOffset(long upperOffset, long bottomOffset) { |
There was a problem hiding this comment.
[P2] Preserve valid nested LIMITs when only the merged offset overflows
Two nested LIMIT 1 OFFSET 9223372036854775806 nodes are individually representable because each limit + offset equals Long.MAX_VALUE, and the outer node necessarily returns zero rows because its offset exceeds the child's one-row output. This guard nevertheless turns that valid query into an analysis error even though mergeLimit already derives a zero limit. Please canonicalize this case to a representable empty limit; PhysicalPlanTranslator's Exchange fold at lines 2263-2265 needs the same zero-limit handling so it does not call mergeOffset after deriving zero.
| @@ -58,6 +60,10 @@ public Rule build() { | |||
| long limit = topN.getLimit(); | |||
| long childOffset = childTopN.getOffset(); | |||
| long childLimit = childTopN.getLimit(); | |||
There was a problem hiding this comment.
[P2] Keep individually valid nested TopNs executable
Compatible nested TopNs can hit this guard even when each node is valid. For example, two TopNs with limit 1 and offset 9223372036854775806 each have limit + offset == Long.MAX_VALUE; the outer one must return zero rows, but their combined offsets overflow and this code throws before lines 68-73 derive newLimit = 0. The ordered derived-table shape is already known to reach MERGE_TOP_N in test_merge_topn_offset.groovy. Please canonicalize the empty result with a representable offset before this check, and add a boundary case for it.
2 participants
Cherry-picked from #64633