Skip to content

Conversation

@rtyler
Copy link
Member

@rtyler rtyler commented Sep 16, 2025

After some debate decided that SessionConfig is sufficient to pull
through rather than SessionContext. Due to the wacky nature of some of
our code-paths in this operation, the signatures of a number of internal
functions had to change to allow SessionConfig to be pulled all the way
down into the ZOrderExecContext

Fixes #3751

@rtyler rtyler requested a review from fvaleye September 16, 2025 13:15
@github-actions github-actions bot added the binding/rust Issues for the Rust crate label Sep 16, 2025
@github-actions
Copy link

ACTION NEEDED

delta-rs follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification.

@rtyler rtyler force-pushed the chore/optional-session-config-optimize-3751 branch from d28c2e1 to ce1f9d4 Compare September 16, 2025 13:20
@codecov
Copy link

codecov bot commented Sep 16, 2025

Codecov Report

❌ Patch coverage is 73.33333% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.69%. Comparing base (0864b21) to head (ff86977).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crates/core/src/operations/optimize.rs 73.33% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3763      +/-   ##
==========================================
- Coverage   75.70%   75.69%   -0.01%     
==========================================
  Files         145      145              
  Lines       44719    44731      +12     
  Branches    44719    44731      +12     
==========================================
+ Hits        33854    33860       +6     
- Misses       9193     9199       +6     
  Partials     1672     1672              

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

🚀 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.

@fvaleye
Copy link
Collaborator

fvaleye commented Sep 16, 2025

Hey @rtyler 👋,

If you would like minimal changes:
You could adaptoptimize.rs:

let ctx = SessionContext::new_with_config_rt(SessionConfig::default(), runtime);
to

// Create SessionConfig from environment variables
let config_options = ConfigOptions::from_env().unwrap_or_default();
let session_config = SessionConfig::from(config_options);
let ctx = SessionContext::new_with_config_rt(session_config, runtime);

For going broader across the codebase, you could adapt DeltaSessionConfig to load the config from env by default on creation:

 pub fn new() -> Self {
      // Start with environment variables if available, otherwise use DataFusion defaults
      let base_config = datafusion::common::config::ConfigOptions::from_env()
          .map(SessionConfig::from)
          .unwrap_or_default();

      // Apply delta-specific overrides
      let inner = base_config.set_bool("datafusion.sql_parser.enable_ident_normalization", false);

      DeltaSessionConfig { inner }
  }

@rtyler rtyler changed the title feature: OptimizeBuilder accepts SessionConfig for finer-grained control of execution feat: OptimizeBuilder accepts SessionConfig for finer-grained control of execution Sep 17, 2025
… of execution

After some debate decided that SessionConfig is sufficient to pull
through rather than SessionContext. Due to the wacky nature of some of
our code-paths in this operation, the signatures of a number of internal
functions had to change to allow SessionConfig to be pulled all the way
down into the ZOrderExecContext

Fixes delta-io#3751

Signed-off-by: R. Tyler Croy <[email protected]>
@rtyler rtyler force-pushed the chore/optional-session-config-optimize-3751 branch from ce1f9d4 to ba976d9 Compare September 17, 2025 12:52
@rtyler
Copy link
Member Author

rtyler commented Sep 18, 2025

@fvaleye I have taken some time to cnosider the suggestion. I am reluctant to add environmental configuration loading into delta-rs from a general standpoint. I think that can lead to surprising behavior for end-users who are not expecting it.

I feel that the builder pattern does give end-users the ability to do that if
they want.

I am not thrilled with how invesaive adding this has to be, but I don't have a
better idea in mind to exposing this API without touching too much

@fvaleye
Copy link
Collaborator

fvaleye commented Sep 18, 2025

@fvaleye I have taken some time to cnosider the suggestion. I am reluctant to add environmental configuration loading into delta-rs from a general standpoint. I think that can lead to surprising behavior for end-users who are not expecting it.

I feel that the builder pattern does give end-users the ability to do that if they want.

I am not thrilled with how invesaive adding this has to be, but I don't have a better idea in mind to exposing this API without touching too much

Fair point!
It would actually change the current behaviour, I agree.

The builder's new option is the more convenient approach.

@rtyler rtyler changed the title feat: OptimizeBuilder accepts SessionConfig for finer-grained control of execution feat: allow OptimizeBuilder to accept SessionConfig for finer-grained control of execution Sep 20, 2025
@rtyler rtyler marked this pull request as ready for review September 20, 2025 00:21
@rtyler rtyler enabled auto-merge (rebase) September 20, 2025 00:21
@rtyler rtyler merged commit 446ba3c into delta-io:main Sep 20, 2025
27 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

binding/rust Issues for the Rust crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow configuring SessionConfig in optimize

3 participants