fix: Add partial support for recursive queries instrumentation #6
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
geoffreyclaude
force-pushed
the
fix/recursive_query
branch
from
b6be176 to
e8120ee
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
geoffreyclaude
force-pushed
the
fix/recursive_query
branch
from
e8120ee to
2ca3fea
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
geoffreyclaude
force-pushed
the
fix/recursive_query
branch
from
2ca3fea to
5d0b88e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This change introduces partial support for tracing and metrics instrumentation of recursive queries in DataFusion. Recursive queries now spawn a new span group for each recursive call, as the recursive implementation recreates the recursive nodes for each loop iteration. This is enabled by supporting the `with_new_inner` method on `InstrumentedExec`, which allows the optimizer to rebuild instrumented plans as needed. However, due to current limitations in DataFusion, nodes of type `WorkTableExec` are not instrumented, as wrapping them would break recursive query execution. This limitation will be revisited once upstream changes allow `WorkTableExec` to be safely instrumented. See also: apache/datafusion#16469 and related discussions.
geoffreyclaude
force-pushed
the
fix/recursive_query
branch
from
5d0b88e to
98bc696
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR partially addresses the issue of recursive query instrumentation by avoiding the wrapping of WorkTableExec nodes with InstrumentedExec, thereby preventing the “Unexpected empty work table” error. Key changes include:
- Adding a with_new_inner method to InstrumentedExec to rebuild instrumented plans.
- Updating the instrumentation rule to skip WorkTableExec nodes.
- Introducing new integration tests and snapshot updates for recursive query scenarios.
Reviewed Changes
Copilot reviewed 28 out of 30 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/snapshots/*.snap | Added new snapshots for various execution plans to capture recursive query behavior. |
| tests/integration_tests.rs | Introduced new async tests to verify recursive query execution and instrumentation. |
| integration-utils/queries/recursive.sql | Added a recursive SQL query to exercise recursive query instrumentation. |
| datafusion-tracing/src/lib.rs | Updated documentation to explain limitations with WorkTableExec instrumentation. |
| datafusion-tracing/src/instrumented.rs | Added the with_new_inner method and new delegate methods to rewrap inner plans. |
| datafusion-tracing/src/instrument_rule.rs | Modified the PhysicalOptimizerRule to skip wrapping WorkTableExec nodes. |
| README.md | Updated documentation regarding the known recursive query limitations. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
Recursive queries previously failed with
Unexpected empty work tablewhen instrumentation was enabled, due to the wayInstrumentedExecwrapped all nodes, includingWorkTableExec, which is not safe to instrument. This PR introduces a partial fix that allows recursive queries to be instrumented, while explicitly avoiding instrumentation ofWorkTableExecnodes. This enables tracing and metrics for recursive queries, even though full support is pending upstream changes.What changes are included in this PR?
with_new_innermethod onInstrumentedExec, allowing the optimizer to rebuild instrumented plans as needed for recursive queries.WorkTableExecnodes, preventing the "Unexpected empty work table" error.Are these changes tested?
Yes, new and updated integration tests and snapshots are included to verify that recursive queries are now instrumented correctly and do not fail with the previous error. The tests cover both the presence of instrumentation and the correct execution of recursive queries.
Are there any user-facing changes?
Yes:
WorkTableExecare not instrumented, so some internal operations in recursive queries will not be traced until upstream support is available.