Repository navigation
[df] Consider quoted strings in collection size short syntax - #23804
Conversation
Test Results 24 files 24 suites 3d 21h 45m 8s ⏱️ For more details on these failures, see this check. Results for commit 55eb492. ♻️ This comment has been updated with latest results. |
dpiparo
left a comment
dpiparo
left a comment
There was a problem hiding this comment.
Thanks. To be backported to 6.40?
dpiparo
left a comment
dpiparo
left a comment
There was a problem hiding this comment.
Thanks. To be backported to 6.40?
The regex pattern present before was not refined enough to consider the case where the target #varname could happen to be within a quoted string. Improve the regex pattern in ParseRDFExpression and add a more complicated test case in dataframe_interface.cxx Fixes root-project#23790
Also add a test where the user defines a column of type std::string with a value that could be ambiguous w.r.t. the short syntax for collection sizes. RDataFrame should not substitute the string value in this case, it should be returned as-is to downstream actions. Co-authored-by: muntasir-396 <muntasirmamun396@gmail.com>
|
Many tanks for providing a patch do quickly! Is there any chance this could be also backported to 6.32? |
6.32 is a few releases back already, and we don't backport unless necessary as shown here https://root.cern/install/all_releases/ . Can you elaborate on how this issue impacts your workflow and in what area, so we can gauge the urgency? If possible it would be anyway better to upgrade to a more recent version of ROOT. |
|
I agree that my use case is likely a fringe scenario, given that no one has reported this issue since the feature was introduced. That said, the change is quite localized, and the relevant code hasn’t been modified in the last five years. So there should be no side effects. Ultimately, it's your call whether backporting to 5.32 is worth the effort. |
|
@bgrube that's why I asked. Backporting is just one ingredient. If we merge this commit in the 6.32 patches branch, that doesn't mean you will get it immediately in ROOT. A new release must be tagged, the various package processes have to be started, and they all differ in length and complexity. Knowing your use case better, how you obtain ROOT, how much you're impacted by this, would help the project gauge the relevance and urgency of such a move. |
|
I have a workaround by reverting to ROOT 6.24 for the affected data processing step. The only drawback is that none of the 6.24 releases includes the patch to read 6.30 trees without error messages. But things seem to work nevertheless. |
|
/backport to 6.40 |
|
Preparing to backport PR #23804 to branch 6.40 requested by vepadulano |
|
This PR has been backported to branch 6.40: #23826 |
The regex pattern present before was not refined enough to consider the case where the target #varname could happen to be within a quoted string. Improve the regex pattern in ParseRDFExpression and add a more complicated test case in dataframe_interface.cxx
Fixes #23790