Skip to content

[bug](overwrite) fix s3 not cleanup files cause get old data#65318

Open
zhangstar333 wants to merge 3 commits into
apache:masterfrom
zhangstar333:test_case_overwrite
Open

[bug](overwrite) fix s3 not cleanup files cause get old data#65318
zhangstar333 wants to merge 3 commits into
apache:masterfrom
zhangstar333:test_case_overwrite

Conversation

@zhangstar333

@zhangstar333 zhangstar333 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Problem Summary:

When listing object storage paths, Doris skipped directory marker objects, but the iterator stopped immediately if a listed page became empty after filtering. On COS, the first list page
may contain only the directory marker and still be truncated=true. Doris then failed to fetch the next page, incorrectly treating the directory as empty.

Impact:

For Hive tables on COS, INSERT OVERWRITE cleanup could fail to find existing data files. This could leave old files undeleted, causing overwrite results to include both old and new rows.
In some scan paths, it could also produce empty results because no scan ranges were generated.

Release note

None

Check List (For Author)

  • Test

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason
  • Behavior changed:

    • No.
    • Yes.
  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@zhangstar333

Copy link
Copy Markdown
Contributor Author

/review

@zhangstar333 zhangstar333 force-pushed the test_case_overwrite branch 2 times, most recently from 39b684e to f85513b Compare July 7, 2026 09:17
@zhangstar333

Copy link
Copy Markdown
Contributor Author

/review

@zhangstar333

Copy link
Copy Markdown
Contributor Author

run buildall

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I found one blocking issue in the live PR head f85513bdaa908894cbfc5345b839b4e2f4e72836.

Critical checkpoints: the goal is to make S3-compatible listing continue past marker-only pages and fail on non-advancing pagination; the iterator path is covered by the new unit test, but parallel pagination paths are not covered and still have the old token handling. The change is small and focused. No concurrency, lifecycle, new config, persistence, FE-BE protocol, or observability surface is changed. Compatibility is the main concern: malformed COS/OBS/OSS-style pagination can still leave overwrite cleanup incomplete outside the iterator path. I did not run FE tests because this checkout is missing .worktree_initialized and thirdparty/installed/bin/protoc; validation was static plus GitHub PR metadata/diff review.

User focus: no additional focus points were provided. The bundled regression Groovy path is stale for the current live PR; GitHub currently lists only the two FE filesystem files.

Subagent conclusions: optimizer-rewrite found no candidates; tests-session-config found no candidates. MAIN-001 was accepted as the only inline comment. The prior iterator continuation-token issue is duplicate of the existing thread and is addressed by this live patch. Convergence round 1 ended with both subagents replying NO_NEW_VALUABLE_FINDINGS for the same final ledger/comment set.

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Codex automated review failed and did not complete.

Error: Codex completed, but no new pull request review was submitted for the current head SHA.
Workflow run: https://github.com/apache/doris/actions/runs/28852973277

Please inspect the workflow logs and rerun the review after the underlying issue is resolved.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Automated review for live head f85513bdaa908894cbfc5345b839b4e2f4e72836.

I reviewed the two changed FE filesystem files and the surrounding S3-compatible listing/delete/rename/glob paths. I am not adding a new inline comment in this run: the only substantiated remaining issue I found is already covered by the existing inline thread at S3CompatibleFileSystem.java:583, which points out that the sibling pagination loops still need the same non-null/non-stale continuation-token guard as the iterator path.

Critical checkpoints:

  • Goal and tests: the iterator goal is implemented by draining marker-only pages in hasNext(), and the new unit test covers the marker-only first-page case. The broader malformed-token behavior is fixed only for ObjectStorageFileIterator; the remaining parallel loops are already covered by the existing review thread.
  • Scope: the change is small and focused.
  • Concurrency, lifecycle, config, persistence, FE-BE protocol, and observability: no new surface found in this PR.
  • Compatibility and parallel paths: S3-compatible pagination compatibility remains the main risk, but the remaining instances are duplicate of existing review context and were not resubmitted.
  • Tests/results: I did not run FE tests because this checkout lacks .worktree_initialized, thirdparty/installed, and thirdparty/installed/bin/protoc. I did verify the GitHub PR patch with git apply --check --reverse --whitespace=error.
  • Performance: no material regression found for the changed iterator path.

User focus: no additional user-provided focus points.

Subagent conclusions: optimizer-rewrite appended no candidates; tests-session-config appended no candidates. MAIN-DUP-001 was recorded as duplicate_of_existing_thread and suppressed from the final inline set. Convergence round 1 ended with both live subagents replying exactly NO_NEW_VALUABLE_FINDINGS for the same ledger/comment set.

@zhangstar333 zhangstar333 force-pushed the test_case_overwrite branch from f85513b to 5df7993 Compare July 7, 2026 12:28
@zhangstar333 zhangstar333 force-pushed the test_case_overwrite branch from 5df7993 to 1db5919 Compare July 7, 2026 12:29
@zhangstar333

Copy link
Copy Markdown
Contributor Author

run buildall

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.

3 participants