Skip to content

Forward SegmentDirectoryLoaderContext in DefaultSegmentDirectoryLoader#18863

Merged
xiangfu0 merged 1 commit into
apache:masterfrom
raghavyadav01:fix-default-segment-loader-context
Jun 27, 2026
Merged

Forward SegmentDirectoryLoaderContext in DefaultSegmentDirectoryLoader#18863
xiangfu0 merged 1 commit into
apache:masterfrom
raghavyadav01:fix-default-segment-loader-context

Conversation

@raghavyadav01

@raghavyadav01 raghavyadav01 commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

What

#18806 ("Replace segmentDirectoryConfigs map with ReadMode in SegmentDirectoryLoaderContext") refactored DefaultSegmentDirectoryLoader.load and inadvertently dropped the segmentLoaderContext argument when constructing SegmentLocalFSDirectory:

// Before #18806
return new SegmentLocalFSDirectory(directory, segmentLoaderContext,
    ReadMode.valueOf(segmentDirectoryConfigs.getProperty(IndexLoadingConfig.READ_MODE_KEY)));

// After #18806 (current master)
return new SegmentLocalFSDirectory(directory, segmentLoaderContext.getReadMode());

The 2-arg SegmentLocalFSDirectory(File, ReadMode) constructor sets the underlying SingleFileIndexDirectory#_segmentDirectoryLoaderContext to null. This silently breaks the task-config propagation introduced in #18264SingleFileIndexDirectory#serializeTaskConfigToJsonFromContext() short-circuits at its first guard:

private String serializeTaskConfigToJsonFromContext() {
  if (_segmentDirectoryLoaderContext == null) {
    return null;     // ← #18806 makes this always true on the default loader path
  }
  ...
}

As a result, task.config.json is never injected into EmptyIndexBuffer.properties, and downstream consumers that depend on the table's task configuration to resolve their own configuration silently fall back to ambient defaults

PR apache#18806 inadvertently dropped the segmentLoaderContext argument when
constructing SegmentLocalFSDirectory, leaving SingleFileIndexDirectory's
_segmentDirectoryLoaderContext field null. This causes the task-config
propagation logic added in PR apache#18264
(SingleFileIndexDirectory#serializeTaskConfigToJsonFromContext) to
short-circuit at its first guard, so task.config.json is never injected
into EmptyIndexBuffer.properties and downstream consumers silently fall
back to ambient credential defaults.

Restore the prior behavior by constructing SegmentMetadataImpl and
passing both the metadata and the loader context to SegmentLocalFSDirectory's
4-arg constructor.
@raghavyadav01 raghavyadav01 added bug Something is not working as expected and removed bug Something is not working as expected labels Jun 26, 2026

@Jackie-Jiang Jackie-Jiang 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.

Good catch

@Jackie-Jiang Jackie-Jiang added the bug Something is not working as expected label Jun 26, 2026
@codecov-commenter

codecov-commenter commented Jun 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 37.18%. Comparing base (3be6236) to head (a09daac).

Additional details and impacted files
@@             Coverage Diff              @@
##             master   #18863      +/-   ##
============================================
+ Coverage     37.16%   37.18%   +0.01%     
  Complexity     1321     1321              
============================================
  Files          3393     3393              
  Lines        211305   211306       +1     
  Branches      33226    33226              
============================================
+ Hits          78541    78578      +37     
+ Misses       125557   125519      -38     
- Partials       7207     7209       +2     
Flag Coverage Δ
custom-integration1 100.00% <ø> (ø)
integration 100.00% <ø> (ø)
integration1 100.00% <ø> (ø)
integration2 0.00% <ø> (ø)
java-21 37.18% <100.00%> (+0.01%) ⬆️
temurin 37.18% <100.00%> (+0.01%) ⬆️
unittests 37.18% <100.00%> (+0.01%) ⬆️
unittests2 37.18% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@xiangfu0 xiangfu0 merged commit bec3dcc into apache:master Jun 27, 2026
16 of 20 checks passed
@xiangfu0 xiangfu0 deleted the fix-default-segment-loader-context branch June 27, 2026 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something is not working as expected

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants