Analyze flightsql - #362
Open
matthewmturner wants to merge 22 commits into
Open
Conversation
Drop the committed notes scratch file and .claude/settings.local.json (now gitignored); open items from notes moved to the spec's Future Work section. Fix copy-pasted 'benchmarked' error message on the analyze path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…categories Replaces the name-keyed operator hierarchy with stable node_id / parent_node_id assigned by pre-order traversal, so plans with repeated operator types (partial+final aggregates, self joins) round-trip correctly. One plan walk now collects identity, I/O, and compute metrics per node. I/O collection is rewritten for DataFusion 51: scans are detected via DataSourceExec -> FileScanConfig -> FileSource downcasts (the old CsvExec/ParquetExec name list matched nothing, so no I/O metrics were ever collected), pruning metrics use PruningMetrics::pruned()/matched() with the correct metric names (as_usize() on PruningMetrics always returns 0), bloom/page-index metrics are actually wired up, and each scan node reports separately instead of overwriting the last one. A single OperatorCategory enum now drives classification, display, and wire encoding, fixing the four compute categories (window, distinct, limit, union) that were collected but never serialized. The wire schema drops the unusable 'ratio' value type, deserialization rebuilds the hierarchy instead of discarding it, reconstructed stats no longer carry a dummy EmptyExec plan, and derived ratios print N/A instead of NaN/Inf. serde_json is gated behind the flightsql feature. Adds unit tests: metrics-table round-trip equality on plans with duplicate operators, value-asserting Parquet pruning on a multi row-group fixture, per-scan I/O separation, category emission, and node-id assignment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ient The analyze request now carries protocol_version; the server rejects incompatible major versions with invalid_argument. Responses carry analyze.protocol_version and a per-request analyze.query_id UUID in the schema metadata so concurrent clients can correlate responses. A new analyze_query_capabilities action lets clients probe support before issuing an expensive analyze call. The client now accepts schema + N data batches per the FlightData framing contract (concatenating them) instead of assuming exactly one data message, and validates the server's protocol version. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Local --analyze-raw now prints or writes the raw metrics table (it previously fell through to the formatted summary, contradicting the docs). --run-before gains parity with --bench on the analyze path. The duplicated exactly-one-file-or-command checks collapse into a single validator so the feature-gated match arms can't drift again. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The FlightSQL analyze tests now assert I/O namespaces unconditionally (the old 'if output contains io.' hedge passed while zero I/O metrics were emitted) and verify partial/final AggregateExec keep distinct node ids over the wire. New cli_cases::analyze covers local --analyze, --analyze-raw, output-to-file, Parquet I/O metrics via --run-before, and arg validation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Spec: node_id/parent_node_id replace name-based operator identity; NULL rules and canonical row key defined; ratio value type dropped; protocol_version and query_id response metadata plus the capabilities action specified; related-work, timing-semantics, query.bytes definition, and operational considerations (re-execution cost, payload size, authz routing) added; unimplemented csv/json metric names marked reserved; implementation-agnostic claim reframed as modeled on DataFusion. cli.md and flightsql_server.md drop the stale two-table response description and document the derived ratios, --run-before, and --output for raw metrics. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Resolves conflicts from the DataFusion 54 upgrade and new features on main:
- Cargo.toml / datafusion-app Cargo.toml: union of both sides' deps and
features (keep analyze's serde_json/prost, main's strum derive, rustls,
clickhouse/functions-arrow features)
- stats.rs: keep the analyze rewrite, migrated to the DF54 downcast API
(ExecutionPlan/DataSource/FileSource downcast_ref instead of as_any)
- flightsql tests: keep both sides' new tests; invalid-SQL expectations
updated for the real FlightSqlServiceImpl error ("error parsing SQL query")
- Cargo.lock regenerated from the merged manifests
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
TODO - Aliases? I.e FileScanExec + StorageExec = IoExec to hide implementation details