Skip to content

Conversation

desmondcheongzx
Copy link
Collaborator

Changes Made

#4772 added a strict mode for filter pushdowns. However, it uses environment variables during the optimizer rule itself to check for strictness. This is a recipe for race conditions (and already causes flakey CI failures), so let's move towards using the planning config instead of environment variables at run time.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Summary

This PR refactors the strict filter pushdown optimization to eliminate race conditions caused by runtime environment variable checks. The changes move the enable_strict_filter_pushdown configuration from DaftExecutionConfig to DaftPlanningConfig, ensuring the setting is determined at planning time rather than during optimization execution.

The key architectural improvement involves passing the strict pushdown flag through the OptimizerConfig structure to the PushDownFilter rule, replacing direct environment variable access within the optimizer. The PushDownFilter struct now stores a strict_pushdown boolean field set during construction, and all optimization logic uses this immutable field instead of checking environment variables at runtime.

This change follows Daft's established pattern of using structured configuration objects for planning-time decisions while reserving execution config for runtime behavior. The refactoring also improves testability by making optimizer configuration more explicit - tests now explicitly call with_default_optimizations() rather than relying on hidden default behavior. All existing functionality remains intact, but the system becomes deterministic and thread-safe by eliminating mutable global state dependencies during optimization.

Confidence score: 4/5

  • This PR safely addresses a real race condition issue by moving configuration from runtime to planning time
  • The refactoring follows established architectural patterns and maintains all existing functionality
  • All files need careful review to ensure the configuration flow is properly maintained across the planning and optimization pipeline

5 files reviewed, no comments

Edit Code Review Bot Settings | Greptile

Copy link
Member

@kevinzwang kevinzwang left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

codecov bot commented Aug 1, 2025

Codecov Report

❌ Patch coverage is 89.67742% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.24%. Comparing base (79c7de3) to head (ebbe771).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/daft-logical-plan/src/builder/mod.rs 33.33% 16 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4888      +/-   ##
==========================================
- Coverage   79.24%   79.24%   -0.01%     
==========================================
  Files         896      896              
  Lines      125004   125019      +15     
==========================================
+ Hits        99065    99070       +5     
- Misses      25939    25949      +10     
Files with missing lines Coverage Δ
src/common/daft-config/src/lib.rs 75.25% <ø> (-3.32%) ⬇️
src/common/daft-config/src/python.rs 66.66% <ø> (+0.65%) ⬆️
...rc/daft-logical-plan/src/optimization/optimizer.rs 95.28% <100.00%> (+0.20%) ⬆️
...al-plan/src/optimization/rules/push_down_filter.rs 94.38% <100.00%> (-0.09%) ⬇️
src/daft-logical-plan/src/builder/mod.rs 82.38% <33.33%> (-1.12%) ⬇️

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@desmondcheongzx desmondcheongzx merged commit aea043e into main Aug 1, 2025
50 checks passed
@desmondcheongzx desmondcheongzx deleted the desmond/fix-strict-pushdown-filter branch August 1, 2025 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants