[perf](s3) push down expandable S3 glob prefixes#64684
Merged
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review result: requesting changes. I found one correctness issue in the new S3-compatible glob-prefix expansion.
Critical checkpoint conclusions:
- Goal/test: the PR aims to reduce object-store listing scope while preserving glob result correctness. The new tests cover the intended bounded date/character-class case and next-cursor discovery across expanded prefixes, but they do not cover wildcard-bearing brace alternatives.
- Scope: the code change is focused, but it is in the shared S3-compatible filesystem base used by S3/OSS/COS/OBS.
- Concurrency/lifecycle/config/compatibility: no new concurrency, special lifecycle, configuration item, or serialization/protocol compatibility concern found.
- Parallel paths: HDFS/Azure implementations are separate and unchanged; the changed shared base covers the S3-compatible providers consistently.
- Error handling/observability/performance: existing IOException wrapping and logging behavior are unchanged. The performance optimization is valid for safely enumerable prefixes, but one unsafe expansion path needs correction.
- Data correctness: one case can skip objects that match the glob before regex filtering, so this review requests changes.
- Tests: I did not run FE UT locally because
thirdparty/installed/bin/protocis missing, and the standardrun-fe-ut.shpath invokesgenerated-source.sh.
Existing review context: .code-review.Uw9GFW/pr_review_threads.md and pr_review_comments.json showed no existing inline comments or replies.
User focus: no additional user-provided review focus was present.
Subagent conclusions:
optimizer-rewritereported OR-1, the same wildcard-bearing brace expansion bug; it was merged into M-1 and became inline comment C-1.tests-session-configreported TSC-1 as the missing regression-test facet of M-1; it was merged as a duplicate/coverage note, not a separate inline issue.- Convergence round 1 ended with both live subagents replying
NO_NEW_VALUABLE_FINDINGSfor the same current ledger/comment set.
### What problem does this PR solve? Issue Number: None Related PR: apache#64684 Problem Summary: The S3 glob prefix pushdown can expand bounded brace alternatives before listing objects. Before this change, when a brace arm contained an unbounded wildcard such as data/{foo*,bar*}/part.parquet, the recursive expansion returned the partial arm prefixes data/foo and data/bar even though those prefixes were not complete enumerable alternatives. The caller then appended the outer suffix and listed data/foo/part.parquet and data/bar/part.parquet, which could miss valid objects such as data/foobar/part.parquet. Now wildcard characters inside a non-partial recursive expansion abort the expansion, so the outer call falls back to the conservative common prefix data/. This preserves correctness while still allowing fully bounded alternatives to be pushed down. ### Release note Fix S3 glob listing correctness for brace alternatives whose arms contain wildcard characters. ### Check List (For Author) - Test: Unit Test - mvn -f fe/pom.xml -pl fe-filesystem/fe-filesystem-s3 -am -Dtest=S3FileSystemTest -DfailIfNoTests=false test - Behavior changed: Yes. S3 glob prefix pushdown now falls back to the conservative common prefix instead of listing incorrect suffixed partial prefixes when a brace arm contains unbounded wildcards. - Does this need documentation: No
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28886 ms |
Contributor
TPC-DS: Total hot run time: 172490 ms |
Contributor
There was a problem hiding this comment.
Reviewed the S3-compatible glob prefix expansion changes. I do not have additional inline findings beyond the existing thread about brace arms containing unbounded wildcards; the current head addresses that case by aborting non-partial brace-arm expansion on */? and falling back to the conservative prefix.
Checkpoint conclusions:
- Goal and coverage: the PR narrows object-store listing for safely enumerable glob prefixes while still filtering with the full glob regex. The added S3 tests cover the date-prefix expansion, fallback for wildcard brace arms, and finding the next match across expanded prefixes after a page limit.
- Scope: the change is focused on the S3-compatible filesystem helper/listing path plus S3 unit test exposure.
- Correctness: multi-prefix scans are sorted and compacted before listing, keep
startAfteras the global lower bound for each first page, and continue into later prefixes after hitting a limit until the next matching key is found. - Parallel paths: S3/OSS/COS/OBS share
S3CompatibleFileSystem; Azure/HDFS/broker keep separate glob-list implementations and are not required to receive this optimization. - Config, compatibility, concurrency, lifecycle, persistence: no new config/session variables, protocol/storage-format changes, concurrency surfaces, or lifecycle/persistence changes were introduced.
- Testing/style: test coverage is appropriate for the changed helper and listing behavior. No additional regression issue was found.
Subagent conclusions: optimizer-rewrite and tests-session-config both converged on NO_NEW_VALUABLE_FINDINGS; their only candidates were duplicate/dismissed notes, so the final inline comment set is empty.
Contributor
ClickBench: Total hot run time: 25.35 s |
Contributor
TPC-H: Total hot run time: 28918 ms |
Contributor
TPC-DS: Total hot run time: 172474 ms |
Contributor
ClickBench: Total hot run time: 25.04 s |
Contributor
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
liaoxin01
reviewed
Jun 22, 2026
liaoxin01
left a comment
Contributor
There was a problem hiding this comment.
I found three additional correctness/compatibility issues in the current glob-prefix expansion: Unicode code-point handling, cross-prefix ordering, and S3 directory-bucket restrictions.
### What problem does this PR solve? Issue Number: None Related PR: apache#64684 Problem Summary: The S3 glob prefix pushdown expanded character classes and sorted expanded prefixes using Java UTF-16 char/String semantics. Before this change, a character class containing a supplementary Unicode code point such as an emoji could be expanded into unpaired surrogate prefixes, so the object listing could miss keys that the final glob regex would match. Expanded prefixes were also compacted in Java String order, which can differ from S3 UTF-8 binary listing order and make cross-prefix pagination unstable for supplementary characters. In addition, S3 directory bucket endpoints can reject expanded prefixes that do not end in '/', and their listing model is not compatible with the multi-prefix optimization. Now character classes containing surrogate code units fall back to the conservative static prefix, expanded prefixes are ordered by unsigned UTF-8 bytes, and S3 Express directory bucket endpoints use the slash-terminated static prefix instead of expanded multi-prefix listing. ### Release note Fix S3 glob listing correctness for supplementary Unicode characters and avoid expanded glob prefix listing on S3 Express directory bucket endpoints. ### Check List (For Author) - Test: Unit Test - mvn -f fe/pom.xml -pl fe-filesystem/fe-filesystem-s3 -am -Dtest=S3FileSystemTest -DfailIfNoTests=false test - Behavior changed: Yes. Unsafe S3 glob prefix expansion now falls back to conservative listing, expanded prefix order follows S3 UTF-8 binary order, and S3 Express directory bucket endpoints do not use expanded multi-prefix listing. - Does this need documentation: No
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Reviewed the current head against the changed S3 filesystem files and the existing inline review context. I did not find a new, non-duplicate issue to submit. The existing inline threads cover the previously substantiated risks around brace arms with unbounded wildcards, supplementary character classes, UTF-8 prefix ordering, and directory-bucket listing semantics.
Checkpoint conclusions:
- Goal/test coverage: The PR improves S3-compatible glob listing by expanding bounded prefixes and adds focused S3 unit coverage for the reported edge cases. I could not run the targeted FE unit test because
./run-fe-ut.sh --run org.apache.doris.filesystem.s3.S3FileSystemTestfails during generated-source setup:thirdparty/installed/bin/protocis missing in this runner. - Scope: The implementation is localized to S3-compatible glob prefix expansion, S3 directory-bucket fallback, and matching unit tests.
- Concurrency/lifecycle: No new shared mutable state, threads, locks, or lifecycle-sensitive static initialization beyond immutable constants/comparators.
- Configuration/compatibility: No user-facing config item is added. Production S3 provider construction passes typed
S3FileSystemProperties, so the directory-bucket endpoint check is available on the normal SPI path. Non-directory S3-compatible providers retain the conservative fallback when expansion is unsafe or too large. - Parallel paths: S3, OSS, COS, and OBS share the S3-compatible base path; their object-list adapters honor continuation/start-after style options. AWS S3 directory-bucket behavior remains the already-raised existing thread rather than a new comment.
- Tests/style: New unit tests cover the fixed edge cases, and
git diff --checkpassed. Targeted FE UT execution was blocked by missing thirdparty dependencies. - Observability/transactions/persistence/FE-BE protocol: Not applicable to this filesystem-only change.
User focus: No additional user-provided review focus was supplied.
Subagent conclusions: optimizer-rewrite reported NO_NEW_VALUABLE_FINDINGS; tests-session-config reported NO_NEW_VALUABLE_FINDINGS and identified only the duplicate directory-bucket StartAfter concern already covered by discussion_r3453830420. Convergence round 1 ended with both live subagents reporting NO_NEW_VALUABLE_FINDINGS for the same ledger/comment set.
Contributor
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
Contributor
TPC-H: Total hot run time: 29171 ms |
Contributor
TPC-DS: Total hot run time: 172828 ms |
Contributor
ClickBench: Total hot run time: 25.18 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
|
PR approved by anyone and no changes requested. |
Contributor
FE Regression Coverage ReportIncrement line coverage |
CalvinKirs
approved these changes
Jun 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Before this change, S3-compatible glob listing derived the object-store
ListObjectsprefix by stopping at the first glob metacharacter. For a path like:s3://bucket/asin_trend/sale/month/date=2025-{0[3-9],1[0-2]}-01/mp_id=8/0/0/436/*the old behavior listed the broad prefix:
asin_trend/sale/month/date=2025-and then filtered all returned object keys in FE. If many unrelated objects existed under
date=2025-*, for example other dates,mp_ids, or deeper paths, S3 TVF planning could spend a long time listing and filtering files before query execution started.After this change, Doris expands safely enumerable glob fragments before issuing object-store list requests. The same path is now listed through narrower prefixes such as:
asin_trend/sale/month/date=2025-03-01/mp_id=8/0/0/436/...
asin_trend/sale/month/date=2025-12-01/mp_id=8/0/0/436/Doris still applies the full glob regex after listing, so result correctness is unchanged. The optimization only reduces the remote listing scope. Expansion is limited to bounded brace alternations and positive character classes, with a hard cap to avoid generating too many prefixes. Existing pagination behavior through
startAfterandmaxFileis preserved.