Skip to content

Commit ed4912e

Browse files
committed
Include joined aliases in warning matching
1 parent 9c85a7c commit ed4912e

1 file changed

Lines changed: 100 additions & 2 deletions

File tree

yardstick-rs/src/sql/measures.rs

Lines changed: 100 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4259,7 +4259,7 @@ fn extract_dimension_exprs_from_query(sql: &str) -> HashMap<String, String> {
42594259
fn extract_from_clause_info_ffi(sql: &str) -> FromClauseInfo {
42604260
let mut info = FromClauseInfo::default();
42614261

4262-
if let Ok(select_info) = parser_ffi::parse_select(sql) {
4262+
if let Ok(Ok(select_info)) = std::panic::catch_unwind(|| parser_ffi::parse_select(sql)) {
42634263
for (i, table_ref) in select_info.tables.iter().enumerate() {
42644264
let effective_name = table_ref
42654265
.alias
@@ -4283,6 +4283,66 @@ fn extract_from_clause_info_ffi(sql: &str) -> FromClauseInfo {
42834283
info
42844284
}
42854285

4286+
fn extract_from_clause_info(sql: &str) -> FromClauseInfo {
4287+
#[cfg(test)]
4288+
let mut info = FromClauseInfo::default();
4289+
#[cfg(not(test))]
4290+
let mut info = extract_from_clause_info_ffi(sql);
4291+
4292+
supplement_from_clause_info_from_sql(&mut info, sql);
4293+
info
4294+
}
4295+
4296+
fn supplement_from_clause_info_from_sql(info: &mut FromClauseInfo, sql: &str) {
4297+
let mut search_pos = 0;
4298+
while let Some((keyword_pos, keyword)) = next_from_or_join_keyword(sql, search_pos) {
4299+
let table_start = skip_ws_and_comments(sql, keyword_pos + keyword.len());
4300+
if table_start >= sql.len() || sql.as_bytes()[table_start] == b'(' {
4301+
search_pos = table_start.saturating_add(1);
4302+
continue;
4303+
}
4304+
4305+
let Some(table_end) = parse_qualified_name_span(sql, table_start) else {
4306+
search_pos = table_start.saturating_add(1);
4307+
continue;
4308+
};
4309+
let raw_table = sql[table_start..table_end].trim();
4310+
let table_name =
4311+
extract_last_qualified_identifier(raw_table).unwrap_or_else(|| raw_table.to_string());
4312+
4313+
let alias_start = skip_ws_and_comments(sql, table_end);
4314+
let (effective_name, has_alias, next_pos) =
4315+
if let Some((alias_end, alias)) = parse_alias_span(sql, alias_start) {
4316+
(alias, true, alias_end)
4317+
} else {
4318+
(table_name.clone(), false, table_end)
4319+
};
4320+
4321+
let table_info = TableInfo {
4322+
name: table_name,
4323+
effective_name: effective_name.clone(),
4324+
has_alias,
4325+
};
4326+
if keyword.eq_ignore_ascii_case("FROM") && info.primary_table.is_none() {
4327+
info.primary_table = Some(table_info.clone());
4328+
}
4329+
info.tables.entry(effective_name).or_insert(table_info);
4330+
search_pos = next_pos;
4331+
}
4332+
}
4333+
4334+
fn next_from_or_join_keyword(sql: &str, search_pos: usize) -> Option<(usize, &'static str)> {
4335+
let from_pos = find_top_level_keyword(sql, "FROM", search_pos);
4336+
let join_pos = find_top_level_keyword(sql, "JOIN", search_pos);
4337+
match (from_pos, join_pos) {
4338+
(Some(from), Some(join)) if from <= join => Some((from, "FROM")),
4339+
(Some(_), Some(join)) => Some((join, "JOIN")),
4340+
(Some(from), None) => Some((from, "FROM")),
4341+
(None, Some(join)) => Some((join, "JOIN")),
4342+
(None, None) => None,
4343+
}
4344+
}
4345+
42864346
/// Information about a resolved measure
42874347
#[derive(Debug, Clone)]
42884348
pub struct ResolvedMeasure {
@@ -6501,7 +6561,7 @@ pub fn expand_aggregate_with_at(sql: &str) -> AggregateExpandResult {
65016561
// Prefer parser-FFI FROM extraction (supports JOIN aliases when SQL parses there),
65026562
// then fall back to string extraction for AGGREGATE/AT syntax that parser-FFI cannot parse.
65036563
let context_sql = top_level_parenthesized_query_body(&sql).unwrap_or(sql.as_str());
6504-
let mut from_info = extract_from_clause_info_ffi(context_sql);
6564+
let mut from_info = extract_from_clause_info(context_sql);
65056565
let (primary_table_name, existing_alias) = if let Some(pt) = from_info.primary_table.clone() {
65066566
let alias = if pt.has_alias {
65076567
Some(pt.effective_name.clone())
@@ -9075,6 +9135,17 @@ GROUP BY s.year";
90759135
assert_eq!(find_alias_for_view(&info, "nonexistent"), None);
90769136
}
90779137

9138+
#[test]
9139+
fn test_extract_from_clause_info_string_fallback_join_aliases() {
9140+
let sql = "SELECT r.region, AGGREGATE(refunds) AT (ALL region) \
9141+
FROM fact_orders_v o JOIN fact_returns_v r ON o.id = r.order_id";
9142+
let info = extract_from_clause_info(sql);
9143+
9144+
assert_eq!(info.primary_table.as_ref().unwrap().name, "fact_orders_v");
9145+
assert_eq!(find_alias_for_view(&info, "fact_orders_v"), Some("o"));
9146+
assert_eq!(find_alias_for_view(&info, "fact_returns_v"), Some("r"));
9147+
}
9148+
90789149
#[test]
90799150
#[serial]
90809151
fn test_expand_aggregate_with_join_measure_from_first_table() {
@@ -9128,6 +9199,33 @@ GROUP BY s.year";
91289199
assert!(result.expanded_sql.contains("FROM orders_v"));
91299200
}
91309201

9202+
#[test]
9203+
fn test_at_all_warns_for_joined_measure_alias_where_filter() {
9204+
let sql = "SELECT r.region, AGGREGATE(refunds) AT (ALL region) AS all_region_refunds \
9205+
FROM fact_orders_v o JOIN fact_returns_v r ON o.id = r.order_id";
9206+
let from_info = extract_from_clause_info(sql);
9207+
let mut source_qualifiers = HashSet::from([normalize_identifier_name("fact_returns_v")]);
9208+
if let Some(alias) = find_alias_for_view(&from_info, "fact_returns_v") {
9209+
source_qualifiers.insert(normalize_identifier_name(alias));
9210+
}
9211+
let source_dims = HashSet::from([
9212+
normalize_identifier_name("year"),
9213+
normalize_identifier_name("region"),
9214+
]);
9215+
9216+
let warning = warning_for_at_all_ungrouped_where_with_qualifiers(
9217+
"refunds",
9218+
&[ContextModifier::All("region".to_string())],
9219+
Some("r.year = 2023"),
9220+
&["r.region".to_string()],
9221+
&source_dims,
9222+
&source_qualifiers,
9223+
)
9224+
.unwrap();
9225+
9226+
assert!(warning.contains("year"));
9227+
}
9228+
91319229
#[test]
91329230
fn test_parse_at_modifier_ad_hoc_dimension() {
91339231
// Ad hoc dimension with function expression

0 commit comments

Comments
 (0)