Skip to content

Conversation

ntjohnson1
Copy link
Contributor

Which issue does this PR close?

Covers most of #1250

Rationale for this change

Previous PR fixed inability to use python threads around SessionContext #1248

Suggested that we mark all possible classes frozen to be more explicit/better thread support.

What changes are included in this PR?

  • Fix a typos setting that failed for me locally
  • For all classes that don't mutate members just add frozen tag
  • For two classes apply Arc<Mutex as a strategy to manage Sync and frozen
    • Maybe for PyConfig RwLock is better since we might be reading configs more than writing
  • Left todos for remaining pyclasses
    • everything with pyo3[(get,set)] would require new wrapper code for each member if we think frozen makes sense
    • CaseBuilder was a bit messier. If we derive clone upstream then Arc<Mutex works decently.

I don't expect anything here to be a major performance delta. It wasn't clear if there was a good way to check that. The benchmarks look more interested in datafusion vs other tools rather than comparing branches.

Are there any user-facing changes?

There are no user-facing python changes. I'm don't think the wrapper code is considered public apis.

@ntjohnson1 ntjohnson1 changed the title Make Most pyclasses frozen Make most pyclasses frozen Sep 26, 2025
@timsaucer
Copy link
Contributor

This and #1253 appear to be duplicates. @kosiew and @ntjohnson1 do you have thoughts about merging or closing one?

@ntjohnson1
Copy link
Contributor Author

This and #1253 appear to be duplicates. @kosiew and @ntjohnson1 do you have thoughts about merging or closing one?

#1253 looks way more comprehensive so should be merged. Would potentially be nice to cherry-pick my 2nd, 3rd, and 4th commit from here to mark every existing class as frozen to make it less likely we add non-frozen classes in the future

@kosiew
Copy link
Contributor

kosiew commented Sep 28, 2025

@ntjohnson1
Sorry, I did not notice this PR when I worked on #1253

Would potentially be nice to cherry-pick my 2nd, 3rd, and 4th commit from here to mark every existing class as frozen to make it less likely we add non-frozen classes in the future

I'll work on this.

kosiew pushed a commit to kosiew/datafusion-python that referenced this pull request Sep 28, 2025
kosiew pushed a commit to kosiew/datafusion-python that referenced this pull request Sep 28, 2025
kosiew pushed a commit to kosiew/datafusion-python that referenced this pull request Sep 28, 2025
@ntjohnson1
Copy link
Contributor Author

Closed in favor of #1253

@ntjohnson1 ntjohnson1 closed this Sep 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants