This is an automated email from the ASF dual-hosted git repository. morningman pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/doris-cli.git
commit 758c8fb80a909b838f7268a9662df731d09b5f88 Author: Mingyu Chen (Rayner) <[email protected]> AuthorDate: Sat May 30 10:01:25 2026 +0800 fix(profile): parse section names & session vars across Doris versions Doris spells profile section headers differently across versions, and the parser recognized only one spelling, corrupting the parsed result on clusters using the other: - split_sections now matches spaced and non-spaced spellings ("ChangedSessionVariables"/"Changed Session Variables", "PhysicalPlan"/"Physical Plan") and normalizes them to one key, and treats DetailProfile(...) and Appendix as section boundaries so their bodies no longer bleed into MergedProfile and fabricate empty fragments (which inflated fragment_count, e.g. 3 -> 6). - parse_session_vars now accepts the JSON-array form emitted by newer versions, falling back to the legacy pipe-delimited table. Without these, fragment_count was inflated, the Physical Plan section was dropped, and changed_session_vars came back empty. Adds an offline unit test for the section boundaries/spellings and strengthens the real-profile regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> --- src/parser/profile_parser.rs | 144 +++++++++++++++++++++++++++++++++++++++++++ src/parser/section_parser.rs | 64 ++++++++++++++++++- 2 files changed, 206 insertions(+), 2 deletions(-) diff --git a/src/parser/profile_parser.rs b/src/parser/profile_parser.rs index 0a5f2dc..b8aba9c 100644 --- a/src/parser/profile_parser.rs +++ b/src/parser/profile_parser.rs @@ -73,6 +73,44 @@ pub fn parse(raw_text: &str) -> DorisProfile { } fn parse_session_vars(text: &str) -> Vec<SessionVar> { + let trimmed = text.trim(); + + // Newer Doris emits this section as a JSON array of + // [{VarName, CurrentValue, DefaultValue}, ...]; older versions use a + // pipe-delimited table. Try JSON first, then fall back to the table parser. + if trimmed.starts_with('[') { + if let Ok(serde_json::Value::Array(items)) = + serde_json::from_str::<serde_json::Value>(trimmed) + { + let vars: Vec<SessionVar> = items + .iter() + .filter_map(|item| { + let name = item.get("VarName").and_then(|v| v.as_str())?; + if name.is_empty() { + return None; + } + Some(SessionVar { + name: name.to_string(), + current_value: item + .get("CurrentValue") + .and_then(|v| v.as_str()) + .unwrap_or_default() + .to_string(), + default_value: item + .get("DefaultValue") + .and_then(|v| v.as_str()) + .unwrap_or_default() + .to_string(), + }) + }) + .collect(); + if !vars.is_empty() { + return vars; + } + } + } + + // Legacy pipe-delimited table: "VarName | CurrentValue | DefaultValue". let mut vars = Vec::new(); for line in text.lines() { let line = line.trim(); @@ -306,3 +344,109 @@ pub fn flatten_operators(profile: &DorisProfile) -> Vec<FlatOperator> { fn round2(v: f64) -> f64 { (v * 100.0).round() / 100.0 } + +#[cfg(test)] +mod real_profile_tests { + use super::*; + + /// A REAL Doris profile captured by the e2e harness: `suite_profile.sh` writes + /// it on a successful `profile get --raw`. Committing that file turns this into + /// an offline regression guard for the whole parse pipeline; until it exists the + /// test is a visible no-op (it prints how to produce the fixture and returns). + fn fixture_path() -> std::path::PathBuf { + std::path::Path::new(env!("CARGO_MANIFEST_DIR")) + .join("tests/e2e/fixtures/sample_profile.txt") + } + + /// WHY this test exists: the parser's contract is to turn an opaque profile + /// text into a structured tree the diagnosis commands can rely on. A unit test + /// over a hand-written snippet can't catch a real-profile-only regression (a + /// section header that moved, a counter format that changed). This parses an + /// actual captured profile and asserts the load-bearing invariants every + /// downstream command depends on — so it FAILS if any of them break. + #[test] + fn parses_a_real_captured_profile() { + let path = fixture_path(); + let text = match std::fs::read_to_string(&path) { + Ok(t) if !t.trim().is_empty() => t, + _ => { + eprintln!( + "SKIP parses_a_real_captured_profile: no fixture at {}.\n \ + Run ./start-testing.sh against a cluster, then commit\n \ + tests/e2e/fixtures/sample_profile.txt to activate this test.", + path.display() + ); + return; + } + }; + + let profile = parse(&text); + + // Summary: a real profile must yield a non-empty Profile ID and a positive + // total time (proves the Summary section + duration parsing both worked). + assert!( + !profile.summary.query_id.is_empty(), + "summary.query_id must be parsed from a real profile" + ); + assert!( + profile.summary.total_time_ms.unwrap_or(0.0) > 0.0, + "summary.total_time_ms must parse to a positive number, got {:?}", + profile.summary.total_time_ms + ); + + // Structure: at least one fragment with at least one pipeline of operators + // (proves MergedProfile -> fragment -> pipeline -> operator parsing worked). + assert!( + !profile.fragments.is_empty(), + "a real profile must yield at least one fragment" + ); + assert!( + profile + .fragments + .iter() + .any(|f| f.pipelines.iter().any(|p| !p.operators.is_empty())), + "at least one fragment must contain operators" + ); + + // Flattening: the diagnosis surface must be non-empty, every operator named, + // and sorted by exec_time descending (the head is the slowest). + let flat = flatten_operators(&profile); + assert!( + !flat.is_empty(), + "flatten_operators must yield operators for a real profile" + ); + assert!( + flat.iter().all(|op| !op.name.is_empty()), + "every flattened operator must carry a name" + ); + assert!( + flat.windows(2) + .all(|w| w[0].exec_time_avg_ms >= w[1].exec_time_avg_ms), + "flattened operators must be sorted by exec_time_avg_ms descending" + ); + + // Regression: DetailProfile / Appendix must be section boundaries, not bleed + // into MergedProfile and fabricate empty fragments — every parsed fragment has + // real pipelines. + assert!( + profile.fragments.iter().all(|f| !f.pipelines.is_empty()), + "no parsed fragment may be empty (DetailProfile/Appendix must terminate MergedProfile)" + ); + + // Regression: the Changed Session Variables section is parsed (JSON-array or + // pipe-table form). A profile captured by the e2e harness ran with --profile, + // which always changes at least `enable_profile`. + assert!( + !profile.changed_session_vars.is_empty(), + "changed_session_vars must be parsed from a real profile" + ); + + // Regression: the Physical Plan section is recognized regardless of spelling + // ("PhysicalPlan" vs "Physical Plan"). + let sections = section_parser::split_sections(§ion_parser::normalize_text(&text)); + assert!( + sections.contains_key("Physical Plan"), + "the Physical Plan section must be extracted" + ); + } +} diff --git a/src/parser/section_parser.rs b/src/parser/section_parser.rs index d76e0ee..b8a9c63 100644 --- a/src/parser/section_parser.rs +++ b/src/parser/section_parser.rs @@ -29,8 +29,15 @@ pub fn normalize_text(input: &str) -> String { /// Split a normalized profile text into named sections. pub fn split_sections(text: &str) -> HashMap<String, String> { static SECTION_RE: Lazy<Regex> = Lazy::new(|| { + // Top-level profile section headers. Doris spells these differently across + // versions: with or without internal spaces ("Changed Session Variables" vs + // "ChangedSessionVariables", "Physical Plan" vs "PhysicalPlan"), and some carry a + // parenthesized suffix ("DetailProfile(<query_id>):"). Match every spelling and + // normalize to a canonical key (see canonical_section). DetailProfile and Appendix + // are matched purely as boundaries so their bodies can't bleed into the preceding + // MergedProfile section — which would fabricate empty fragments. Regex::new( - r"(?m)^\s*(Summary|Execution Summary|Changed Session Variables|Physical Plan|MergedProfile)\s*:?\s*$", + r"(?m)^\s*(Summary|Execution ?Summary|Changed ?Session ?Variables|Physical ?Plan|MergedProfile|DetailProfile|Appendix)\b.*:\s*$", ) .unwrap() }); @@ -46,7 +53,7 @@ pub fn split_sections(text: &str) -> HashMap<String, String> { sections.insert(name, current_content.trim().to_string()); current_content.clear(); } - current_name = Some(caps[1].to_string()); + current_name = Some(canonical_section(&caps[1])); } else if current_name.is_some() { current_content.push_str(line); current_content.push('\n'); @@ -61,6 +68,19 @@ pub fn split_sections(text: &str) -> HashMap<String, String> { sections } +/// Normalize a section header to a canonical lookup key, so spelling variants +/// across Doris versions (with/without internal spaces) resolve to the one key +/// the parsers query for. +fn canonical_section(raw_name: &str) -> String { + let compact: String = raw_name.split_whitespace().collect(); + match compact.as_str() { + "ExecutionSummary" => "Execution Summary".to_string(), + "ChangedSessionVariables" => "Changed Session Variables".to_string(), + "PhysicalPlan" => "Physical Plan".to_string(), + _ => compact, + } +} + /// Parse key-value pairs from Summary or Execution Summary sections. /// Lines look like: " - Key: Value" pub fn parse_kv_section(text: &str) -> HashMap<String, String> { @@ -95,4 +115,44 @@ mod tests { assert_eq!(kv.get("Total"), Some(&"32sec281ms".to_string())); assert_eq!(kv.get("Task State"), Some(&"OK".to_string())); } + + #[test] + fn test_split_sections_spellings_and_boundaries() { + // No-space spellings + a parenthesized DetailProfile header that must act as a + // boundary so its "Fragment 0:" cannot leak into MergedProfile. + let text = "\ +MergedProfile: + Fragments: + Fragment 0: + Pipeline 0(instance_num=1): +DetailProfile(abc-123): + Fragments: + Fragment 0: + Pipeline 0(host=h): +ChangedSessionVariables: +[ { \"VarName\": \"enable_profile\" } ] +PhysicalPlan: +PhysicalResultSink[1] +"; + let sections = split_sections(text); + + // Canonical keys exist for the no-space spellings. + assert!(sections.contains_key("Changed Session Variables")); + assert!(sections.contains_key("Physical Plan")); + + // DetailProfile is its own section, so MergedProfile stops before it and does + // not contain the DetailProfile body. + let merged = sections.get("MergedProfile").expect("MergedProfile present"); + assert!(merged.contains("Pipeline 0(instance_num=1)")); + assert!( + !merged.contains("DetailProfile") && !merged.contains("host=h"), + "DetailProfile body must not bleed into MergedProfile, got: {merged}" + ); + + // Physical Plan body is captured, not swallowed by the previous section. + assert!(sections + .get("Physical Plan") + .map(|p| p.contains("PhysicalResultSink")) + .unwrap_or(false)); + } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
