Skip to content

[RDataFrame] Fix FilterAvailable() on Define Columns - #24055

Open
MSK-005 wants to merge 1 commit into
root-project:masterfrom
MSK-005:fix/filteravailable-define-bug
Open

MSK-005 wants to merge 1 commit into
root-project:masterfrom
MSK-005:fix/filteravailable-define-bug

Conversation

@MSK-005

@MSK-005 MSK-005 commented Nov 8, 2026

Copy link
Copy Markdown

This Pull request:

Changes or fixes:

Adds a separate availability-checking mechanism for Define columns without triggering their evaluation.

Previously, FilterAvailable() used the TryGet() function to check whether a column entry was available. For Define columns, this triggered the evaluation of the Define expression. If any of the dependencies of said column were missing, this would throw an exception.

This PR adds a separate function IsAvailable() that checks the availability of a column's dependencies recursively without triggering the evaluation of any Define expression. Normal behaviour for Define evaluation and its exception-handling is still there.

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

This PR, coupled with PR #23729 (review pending), fixes the TTree case of issue #22759.

@vepadulano vepadulano left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the proposed fix! I do not think this is the right approach: it introduces two new virtual methods (one in RDefineBase, one in RColumnReaderBase) which will need to be kept up to date and in sync with future changes. This should be done with care in general and in this case I don't think it's necessary. The RDefineReader should be able to just tell whether it's available or not. Also, I don't yet understand the premise of the changes: the availability of a reader is a responsibility of the reader itself, the recursiveness was so far not considered. Together with this, a Define is by its own nature always available, so I don't really see where the problem stems from. Will try to read more carefully the reproducer and let you know if my intuition changes.

This branch has not been deployed

No deployments
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.

2 participants