-
Notifications
You must be signed in to change notification settings - Fork 535
Feature/parquet arrow renderer #1861
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Feature/parquet arrow renderer #1861
Conversation
- Use string type annotations for pa.* types in Arrow/Parquet renderers. - Ensure pyarrow dependency is checked only in __init__.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks very much, this looks really nice. I still want to check through a few more bits and pieces, but generally it looks good.
At the moment text_renderer
is looking a little full though, I was wondering if you could test whether this still works, if all the new classes are moved into a file like volatility3/plugins/parquet_renderer.py
or similar? We should automatically import all python files under plugins, and I believe we do it before we choose any renderers, which means it should still show up in the available options. We'll probably want to make a renderers directory that people can drop things into, this would just allow users to drop in their own renderers (and might therefore act as a good example of how to write new ones when people want).
Anyway, just some thoughts. I'll give it a full review over the coming week and add in any comments of anything else, but I'm really pleased someone else has written a renderer for a very different format finally! Thanks! 5:)
I really like the structure you're proposing here. However, it does not work at the moment because the plugins are imported after the list of available renderers is generated: volatility3/volatility3/cli/__init__.py Lines 109 to 115 in 997f2a4
volatility3/volatility3/cli/__init__.py Lines 196 to 203 in 997f2a4
and then volatility3/volatility3/cli/__init__.py Lines 336 to 338 in 997f2a4
|
Ok, thanks for investigating, I'll take a look into it... 5:)
…On Mon, 30 Jun 2025, 10:34 Kyrre Wahl Kongsgård, ***@***.***> wrote:
*kyrre* left a comment (volatilityfoundation/volatility3#1861)
<#1861 (comment)>
At the moment text_renderer is looking a little full though, I was
wondering if you could test whether this still works, if all the new
classes are moved into a file like volatility3/plugins/parquet_renderer.py
or similar? We should automatically import all python files under plugins,
and I believe we do it before we choose any renderers, which means it
should still show up in the available options. We'll probably want to make
a renderers directory that people can drop things into, this would just
allow users to drop in their own renderers (and might therefore act as a
good example of how to write new ones when people want).
I really like the structure you're proposing here. However, it does not
work at the moment because the plugins are imported after the list of
available renderers is generated:
https://github.com/volatilityfoundation/volatility3/blob/997f2a4f4c32cbb268ae7236ab3a7e24a3ec9bb2/volatility3/cli/__init__.py#L109-L115
https://github.com/volatilityfoundation/volatility3/blob/997f2a4f4c32cbb268ae7236ab3a7e24a3ec9bb2/volatility3/cli/__init__.py#L196-L203
and then
https://github.com/volatilityfoundation/volatility3/blob/997f2a4f4c32cbb268ae7236ab3a7e24a3ec9bb2/volatility3/cli/__init__.py#L336-L338
—
Reply to this email directly, view it on GitHub
<#1861 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AALIZVNJJF4OCEYJABADI2L3GEACFAVCNFSM6AAAAACAJHP2PKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTAMJYGQ3DCNBSGE>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
I created a new file in the plugin directory
How do you feel about this @ikelos - is this something you can work with? I agree that it is cleaner to create a renderer directory, but maybe this is OK for now? 🤔 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to be so indecisive (and to take so long to rereview it). I think it's much closer and I'm pretty happy with the changes to the main CLI, which is likely the biggest hurdle. The trick now is to get this renderer right (since it'll likely be used as the template for everybody's future renderers). As such, could we try putting it inside of volatility3/plugins/renderers please? I don't think there's any functionality that will decide it's only for renderer
OSes, but best to check it now rather than trying to change it later. Other than that and the other comment I posted, I think it could be ready for prime time?
I think we should be set! What do you think @ikelos ? Another thing we could add is a test suite perhaps 🤔 |
This adds Parquet and arrow renderers and closes #1860