ExecutionPlan visitor example documentation
#10286
Conversation
|
@alamb FYI |
matthewmturner
changed the title
ExecutionPlan visitor example documentation
There was a problem hiding this comment.
Thank you @matthewmturner -- this looks great and I think will really help.
It looks like clippy is complaining in CI: https://github.com/apache/datafusion/actions/runs/8878883209/job/24375602460?pr=10286 (maybe you could print the batch count)
Once the CI is fixed I think we can merge this PR, though I had a few suggestions for improvmements:
- I left some suggestions for additional comments for context
- I think we should also add an entry to the example readme explaining what this shows (e.g. "Extract statistics by visiting an
ExecutionPlanafter execution" or something)
|
Thanks @alamb, I have updated. |
| ); | ||
| } | ||
|
|
||
| /// Define a struct with fields to hold the information you want to collect |
There was a problem hiding this comment.
This comment is very chatgpt-like :))
There was a problem hiding this comment.
Aha I promise I did not use ChatGPT to make this.
Maybe need to add fine print like "LLMs were not used in the generation of this code" 😆
There was a problem hiding this comment.
:) Lets refine struct doc to be more specific
There was a problem hiding this comment.
This was intentionally general, because its up to the user, and it's not really obvious to me that making it more specific would add value to the example but I can add something if you really think its needed.
There was a problem hiding this comment.
mm. perhaps we can rephrase it something like
struct for storing parquet execution metrics, (scanned bytes) in this example
There was a problem hiding this comment.
I have updated
There was a problem hiding this comment.
lgtm @matthewmturner thanks for addressing the comments. Im planning to merge it once CI completed
Which issue does this PR close?
Closes #10012
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?