Skip to content

chore: better error message when writing TBaskets that are too large#1415

Merged
ariostas merged 5 commits intomainfrom
ariostas/tbasket_error_message
Apr 9, 2025
Merged

chore: better error message when writing TBaskets that are too large#1415
ariostas merged 5 commits intomainfrom
ariostas/tbasket_error_message

Conversation

@ariostas
Copy link
Copy Markdown
Member

@ariostas ariostas commented Apr 7, 2025

In #1135 there are some discussion about writing batches of data that are too big to fit in a TBasket. I added more detailed error messages so that it is clear what went wrong, since the way it currently crashes is not very informative.

@ariostas
Copy link
Copy Markdown
Member Author

ariostas commented Apr 7, 2025

@ikrommyd what do you think of these messages?

@ikrommyd
Copy link
Copy Markdown
Collaborator

ikrommyd commented Apr 7, 2025

@ikrommyd what do you think of these messages?

I like them, I would probably also mention that the max size is 2GB if that's always the hard limit. I'm not aware of root file internals so I'm not sure if it's 100% correct but if it is, it's useful so that the user knows what they need to at least reduce the size to.

@ariostas ariostas requested a review from ianna April 8, 2025 20:37
Copy link
Copy Markdown
Member

@ianna ianna left a comment

Choose a reason for hiding this comment

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

@ariostas - I think, it's better to define a constant to rely on. I feel the calculations can be confusing.

Copy link
Copy Markdown
Member

@ianna ianna left a comment

Choose a reason for hiding this comment

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

@ariostas - Thanks! Looks great, please, merge it if you finished with it. Thanks!

@ariostas ariostas merged commit 6d51243 into main Apr 9, 2025
26 checks passed
@ariostas ariostas deleted the ariostas/tbasket_error_message branch April 9, 2025 14:31
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