Add projection to mongo sampling - #344
Conversation
WalkthroughSampling logic in the MongoDB connector now enforces a stricter sample-size threshold, projects only Changes
Sequence DiagramsequenceDiagram
participant Caller
participant SamplingLogic
participant MongoDB
participant ErrorHandler
Caller->>SamplingLogic: Request sampling
SamplingLogic->>SamplingLogic: Compute numSamples
alt numSamples > 1_000_000
SamplingLogic->>SamplingLogic: Log warning (large sample)
end
alt numSamples*21 >= count
SamplingLogic->>ErrorHandler: Build formatted error (adjust factor/target)
ErrorHandler-->>Caller: Return error
else Threshold OK
SamplingLogic->>MongoDB: Run aggregation (project _id → $sample → $sort)
alt aggregation succeeds
MongoDB-->>SamplingLogic: Return sampled ids
SamplingLogic-->>Caller: Return samples
else aggregation fails
MongoDB-->>SamplingLogic: Error
SamplingLogic->>ErrorHandler: Wrap error with context (%w)
ErrorHandler-->>Caller: Return error immediately
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
connectors/mongo/conn.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
connectors/mongo/conn.go (3)
238-240: Good observability improvement.The warning log for large sample sizes helps operators identify potentially expensive operations before they impact performance.
241-241: Excellent performance optimization.Adding the
$projectstage to emit only the_idfield before sampling significantly reduces data transfer overhead, as the downstream code (lines 247-260) only requires the_idfield.
243-243: LGTM!The improved error wrapping with
%wmaintains the error chain while adding valuable context about the number of samples requested.
| if numSamples*21 >= count { | ||
| return fmt.Errorf("too many samples requested (%v of %v)- adjust sample factor or target documents per partition", numSamples, count) | ||
| } |
There was a problem hiding this comment.
Behavioral change: error instead of auto-adjustment.
The sampling logic now returns an error when numSamples*21 >= count instead of adjusting numSamples downward. This is a breaking change that could affect existing configurations that previously relied on auto-adjustment.
Additionally, there's a minor formatting issue in the error message.
Apply this diff to fix the formatting:
- return fmt.Errorf("too many samples requested (%v of %v)- adjust sample factor or target documents per partition", numSamples, count)
+ return fmt.Errorf("too many samples requested (%v of %v) - adjust sample factor or target documents per partition", numSamples, count)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if numSamples*21 >= count { | |
| return fmt.Errorf("too many samples requested (%v of %v)- adjust sample factor or target documents per partition", numSamples, count) | |
| } | |
| if numSamples*21 >= count { | |
| return fmt.Errorf("too many samples requested (%v of %v) - adjust sample factor or target documents per partition", numSamples, count) | |
| } |
| } | ||
| res, err := col.Aggregate(ctx, mongo.Pipeline{{{"$sample", bson.D{{"size", numSamples}}}}, {{"$sort", bson.D{{"_id", 1}}}}}) | ||
| if numSamples > 1000000 { | ||
| slog.Warn("More than 1000000 samples requested", "samples", numSamples) |
There was a problem hiding this comment.
Maybe elaborate on why it's bad (can cause the sampling query to run long or fail) and what to do (adjust the sampling factor or increase task size
9080151 to
f0765f8
Compare
Co-authored-by: Mark C <mark@Marks-MacBook-Pro.local>
2 participants
Summary by CodeRabbit