diff --git a/src/data/hash_join.rs b/src/data/hash_join.rs index f71f8d6b..65773882 100644 --- a/src/data/hash_join.rs +++ b/src/data/hash_join.rs @@ -7,9 +7,124 @@ use tracing::{debug, info}; use crate::data::arithmetic_evaluator::ArithmeticEvaluator; use crate::data::datatable::{DataColumn, DataRow, DataTable, DataValue}; +use crate::data::value_comparisons::compare_with_op; use crate::sql::parser::ast::{JoinClause, JoinOperator, JoinType}; use crate::sql::recursive_parser::SqlExpression; +/// Normalize a value into a canonical form for join-key matching. +/// +/// Equi-joins index keys in a `HashMap`, which keys on the exact +/// `DataValue` variant. That means `String("220")` and `Integer(220)` never +/// collide, so a join between a string column (e.g. a value pulled out of JSON +/// via `SUBSTR`) and an integer column silently produces no matches — even +/// though `WHERE a = b` would coerce and match them +/// (see `value_comparisons::compare_values`). +/// +/// Two normalizations are applied: +/// - `InternedString` always collapses to a plain `String` — the same +/// logical type, just a different in-memory representation, so they must +/// always compare equal to a matching `String`. +/// - whole floats always collapse to integers (so `220.0` matches `220`, +/// even between two numeric columns). +/// - When `coerce_numeric` is set, numeric-looking strings also become +/// numbers (so `"220"` matches `220`). +/// +/// `coerce_numeric` is decided per join site by [`join_key_coercion`]: it is +/// enabled only when the two columns hold different *kinds* of value (a string +/// column vs a numeric column). When both sides are strings — notably after +/// `TO_STRING(...)` on both — no string→number coercion happens, so `"007"` and +/// `"7"` stay distinct. The hash path must decide this per column (it never +/// sees the opposite key), unlike WHERE's pairwise comparison; the nested-loop +/// path defers to WHERE's comparator directly. +/// +/// Note: like WHERE, parsing is not whitespace-trimmed (`" 220"` stays a +/// string). +fn canonical_join_key(value: &DataValue, coerce_numeric: bool) -> DataValue { + match value { + DataValue::String(s) => normalize_join_text(s, coerce_numeric), + DataValue::InternedString(s) => normalize_join_text(s.as_str(), coerce_numeric), + DataValue::Float(f) => fold_whole_float(*f), + other => other.clone(), + } +} + +/// Canonicalize a textual join key: always returned as a plain `String` unless +/// numeric coercion is enabled and the text parses as a number. +fn normalize_join_text(s: &str, coerce_numeric: bool) -> DataValue { + if coerce_numeric { + if let Ok(i) = s.parse::() { + return DataValue::Integer(i); + } + if let Ok(f) = s.parse::() { + if f.is_finite() { + return fold_whole_float(f); + } + } + } + DataValue::String(s.to_string()) +} + +/// The broad "kind" of a join-key value. String↔number coercion is applied +/// only when the two columns hold different kinds. +#[derive(PartialEq, Eq)] +enum KeyKind { + Stringy, + Numeric, + Other, +} + +fn value_kind(value: &DataValue) -> KeyKind { + match value { + DataValue::String(_) | DataValue::InternedString(_) => KeyKind::Stringy, + DataValue::Integer(_) | DataValue::Float(_) => KeyKind::Numeric, + _ => KeyKind::Other, + } +} + +/// The kind of a column, sampled from its first non-null value. +/// +/// We sample actual values rather than the column's declared `data_type` +/// because a materialized temp table does not reliably carry accurate column +/// types (a column holding integers can still be typed as `String`/`Mixed`). +fn column_key_kind(table: &DataTable, col_idx: usize) -> Option { + table + .rows + .iter() + .filter_map(|r| r.values.get(col_idx)) + .find(|v| !matches!(v, DataValue::Null)) + .map(value_kind) +} + +/// Whether an equi-join between two columns should coerce string keys to +/// numbers. Enabled only when the columns hold different value kinds (e.g. a +/// string column vs a numeric column); two string columns join on exact text. +/// If a column's kind can't be determined (empty/all-null) we default to +/// coercing — the permissive behaviour that fixes the cross-type case. +fn join_key_coercion( + left_table: &DataTable, + left_col_idx: usize, + right_table: &DataTable, + right_col_idx: usize, +) -> bool { + match ( + column_key_kind(left_table, left_col_idx), + column_key_kind(right_table, right_col_idx), + ) { + (Some(l), Some(r)) => l != r, + _ => true, + } +} + +/// Collapse a float with no fractional part to an integer so that `220.0` +/// hashes/compares equal to `220`. +fn fold_whole_float(f: f64) -> DataValue { + if f.is_finite() && f.fract() == 0.0 && f >= i64::MIN as f64 && f <= i64::MAX as f64 { + DataValue::Integer(f as i64) + } else { + DataValue::Float(f) + } +} + /// Hash join executor for efficient JOIN operations pub struct HashJoinExecutor { case_insensitive: bool, @@ -289,6 +404,11 @@ impl HashJoinExecutor { ) -> Result { let start = std::time::Instant::now(); + // Numeric coercion is enabled only when the two join columns have + // different declared types (e.g. string vs integer). Decided per column + // because the hash index canonicalizes each key without seeing its mate. + let coerce = join_key_coercion(&left_table, left_col_idx, &right_table, right_col_idx); + // Determine which table to use for building the hash index (prefer smaller) let (build_table, probe_table, build_col_idx, probe_col_idx, build_is_left) = if left_table.row_count() <= right_table.row_count() { @@ -318,7 +438,7 @@ impl HashJoinExecutor { // Build hash index on the smaller table let mut hash_index: HashMap> = HashMap::new(); for (row_idx, row) in build_table.rows.iter().enumerate() { - let key = row.values[build_col_idx].clone(); + let key = canonical_join_key(&row.values[build_col_idx], coerce); hash_index.entry(key).or_default().push(row_idx); } @@ -397,9 +517,9 @@ impl HashJoinExecutor { // Probe phase: iterate through the larger table let mut match_count = 0; for probe_row in &probe_table.rows { - let probe_key = &probe_row.values[probe_col_idx]; + let probe_key = canonical_join_key(&probe_row.values[probe_col_idx], coerce); - if let Some(matching_indices) = hash_index.get(probe_key) { + if let Some(matching_indices) = hash_index.get(&probe_key) { for &build_idx in matching_indices { let build_row = &build_table.rows[build_idx]; @@ -454,6 +574,9 @@ impl HashJoinExecutor { ) -> Result { let start = std::time::Instant::now(); + // Coerce string keys only when the join columns differ in type. + let coerce = join_key_coercion(&left_table, left_col_idx, &right_table, right_col_idx); + debug!( "Building hash index on right table ({} rows)", right_table.row_count() @@ -462,7 +585,7 @@ impl HashJoinExecutor { // Build hash index on right table let mut hash_index: HashMap> = HashMap::new(); for (row_idx, row) in right_table.rows.iter().enumerate() { - let key = row.values[right_col_idx].clone(); + let key = canonical_join_key(&row.values[right_col_idx], coerce); hash_index.entry(key).or_default().push(row_idx); } @@ -537,9 +660,9 @@ impl HashJoinExecutor { let mut null_count = 0; for left_row in &left_table.rows { - let left_key = &left_row.values[left_col_idx]; + let left_key = canonical_join_key(&left_row.values[left_col_idx], coerce); - if let Some(matching_indices) = hash_index.get(left_key) { + if let Some(matching_indices) = hash_index.get(&left_key) { // Found matches - emit joined rows for &right_idx in matching_indices { let right_row = &right_table.rows[right_idx]; @@ -680,16 +803,23 @@ impl HashJoinExecutor { } } - /// Compare two values based on the join operator + /// Compare two values based on the join operator. + /// + /// The nested-loop path has both values in hand, so it defers to the same + /// pairwise comparator WHERE uses (`value_comparisons::compare_with_op`). + /// That keeps JOIN equality identical to WHERE equality — including its + /// type-aware coercion (`String` vs `Integer` coerces; `String` vs `String` + /// compares as text) — so the nested-loop and hash paths agree. fn compare_values(&self, left: &DataValue, right: &DataValue, op: &JoinOperator) -> bool { - match op { - JoinOperator::Equal => left == right, - JoinOperator::NotEqual => left != right, - JoinOperator::LessThan => left < right, - JoinOperator::GreaterThan => left > right, - JoinOperator::LessThanOrEqual => left <= right, - JoinOperator::GreaterThanOrEqual => left >= right, - } + let op_str = match op { + JoinOperator::Equal => "=", + JoinOperator::NotEqual => "!=", + JoinOperator::LessThan => "<", + JoinOperator::GreaterThan => ">", + JoinOperator::LessThanOrEqual => "<=", + JoinOperator::GreaterThanOrEqual => ">=", + }; + compare_with_op(left, right, op_str, self.case_insensitive) } /// Nested loop join for INNER JOIN with inequality conditions @@ -1191,3 +1321,140 @@ impl HashJoinExecutor { Ok(result) } } + +#[cfg(test)] +mod tests { + use super::*; + use std::sync::Arc; + + #[test] + fn numeric_string_folds_to_integer_when_coercing() { + // A string pulled from JSON/SUBSTR must match an integer join key when + // the columns differ in type (coerce = true). + assert_eq!( + canonical_join_key(&DataValue::String("220".to_string()), true), + DataValue::Integer(220) + ); + assert_eq!( + canonical_join_key(&DataValue::Integer(220), true), + DataValue::Integer(220) + ); + assert_eq!( + canonical_join_key(&DataValue::String("220".to_string()), true), + canonical_join_key(&DataValue::Integer(220), true) + ); + } + + #[test] + fn numeric_strings_stay_distinct_when_not_coercing() { + // Same-typed columns (e.g. String vs String) do not numerically coerce, + // so "007" and "7" remain distinct keys. This is the TO_STRING opt-out. + assert_eq!( + canonical_join_key(&DataValue::String("007".to_string()), false), + DataValue::String("007".to_string()) + ); + assert_ne!( + canonical_join_key(&DataValue::String("007".to_string()), false), + canonical_join_key(&DataValue::String("7".to_string()), false) + ); + // A string is never folded into an integer when not coercing. + assert_ne!( + canonical_join_key(&DataValue::String("7".to_string()), false), + canonical_join_key(&DataValue::Integer(7), false) + ); + } + + #[test] + fn interned_and_plain_strings_collapse_regardless_of_coercion() { + for coerce in [true, false] { + assert_eq!( + canonical_join_key( + &DataValue::InternedString(Arc::new("North".to_string())), + coerce + ), + canonical_join_key(&DataValue::String("North".to_string()), coerce), + "interned/plain strings must collapse (coerce = {coerce})" + ); + } + } + + #[test] + fn whole_float_folds_to_integer_when_coercing() { + assert_eq!( + canonical_join_key(&DataValue::Float(220.0), true), + DataValue::Integer(220) + ); + assert_eq!( + canonical_join_key(&DataValue::String("220.0".to_string()), true), + DataValue::Integer(220) + ); + // Fractional floats stay floats. + assert_eq!( + canonical_join_key(&DataValue::Float(220.5), true), + DataValue::Float(220.5) + ); + // Whole floats fold to integers regardless of string coercion, so a + // numeric (int) column joins a numeric (float) column. + assert_eq!( + canonical_join_key(&DataValue::Float(220.0), false), + DataValue::Integer(220) + ); + } + + #[test] + fn non_numeric_text_is_preserved() { + assert_eq!( + canonical_join_key(&DataValue::String("North".to_string()), true), + DataValue::String("North".to_string()) + ); + // Leading whitespace is not trimmed, matching WHERE parse semantics. + assert_eq!( + canonical_join_key(&DataValue::String(" 220".to_string()), true), + DataValue::String(" 220".to_string()) + ); + } + + #[test] + fn non_finite_strings_stay_strings() { + assert_eq!( + canonical_join_key(&DataValue::String("inf".to_string()), true), + DataValue::String("inf".to_string()) + ); + assert_eq!( + canonical_join_key(&DataValue::String("NaN".to_string()), true), + DataValue::String("NaN".to_string()) + ); + } + + #[test] + fn null_is_unchanged() { + assert_eq!(canonical_join_key(&DataValue::Null, true), DataValue::Null); + } + + #[test] + fn coercion_enabled_only_for_differing_value_kinds() { + // Column kind is sampled from actual values, not declared types. + let stringy = single_col_table(DataValue::String("7".to_string())); + let numeric = single_col_table(DataValue::Integer(7)); + let stringy2 = single_col_table(DataValue::String("8".to_string())); + let empty = DataTable::new("empty"); // no columns/rows + + // Stringy vs numeric -> coerce. + assert!(join_key_coercion(&stringy, 0, &numeric, 0)); + // Stringy vs stringy -> no coercion. + assert!(!join_key_coercion(&stringy, 0, &stringy2, 0)); + // Numeric vs numeric -> no coercion (float-folding still applies). + assert!(!join_key_coercion(&numeric, 0, &numeric, 0)); + // Undeterminable kind -> permissive (coerce). + assert!(join_key_coercion(&stringy, 0, &empty, 0)); + } + + fn single_col_table(value: DataValue) -> DataTable { + let mut t = DataTable::new("t"); + t.add_column(DataColumn::new("k")); + let _ = t.add_row(DataRow { + values: vec![value], + }); + t + } +} diff --git a/src/data/query_engine.rs b/src/data/query_engine.rs index 13b98411..29cdf121 100644 --- a/src/data/query_engine.rs +++ b/src/data/query_engine.rs @@ -2906,9 +2906,21 @@ impl QueryEngine { } => { // Check if this has a table prefix let index = if let Some(table_prefix) = &col_ref.table_prefix { - // For qualified references, ONLY try qualified lookup - no fallback + // Qualified reference (e.g. `f.region`). Prefer a qualified + // match (JOIN/CTE columns carry qualified names), then fall + // back to an unqualified lookup by column name. The fallback + // makes aliased single-table queries (`SELECT f.region FROM + // #tmp f`) behave like WHERE/expression clauses do — base and + // temp-table columns carry no qualified_name, so a qualified- + // only lookup would otherwise fail. See + // `ExecutionContext::resolve_column_index` for the same logic. let qualified_name = format!("{}.{}", table_prefix, col_ref.name); table.find_column_by_qualified_name(&qualified_name) + .or_else(|| { + table_columns + .iter() + .position(|c| c.eq_ignore_ascii_case(&col_ref.name)) + }) .ok_or_else(|| { // Check if any columns have qualified names for better error message let has_qualified = table.columns.iter() diff --git a/tests/main.rs b/tests/main.rs index 0f6ce64a..804950ce 100644 --- a/tests/main.rs +++ b/tests/main.rs @@ -19,6 +19,9 @@ mod datatable_integration_test; #[path = "datetime_completion.rs"] mod datetime_completion; +#[path = "temp_table_qualified_join_tests.rs"] +mod temp_table_qualified_join_tests; + #[path = "history_protection_integration.rs"] mod history_protection_integration; diff --git a/tests/temp_table_qualified_join_tests.rs b/tests/temp_table_qualified_join_tests.rs new file mode 100644 index 00000000..cdc17adb --- /dev/null +++ b/tests/temp_table_qualified_join_tests.rs @@ -0,0 +1,187 @@ +//! Regression tests for two temp-table issues: +//! 1. Qualified column references (`SELECT f.col FROM #tmp f`) in the SELECT +//! projection used to fail against single base/temp tables because the +//! projection resolver did a qualified-only lookup with no unqualified +//! fallback (query_engine::resolve_select_columns). +//! 2. Equi-joins between a string key (e.g. extracted from JSON via SUBSTR) +//! and an integer key silently produced no matches because the hash index +//! keyed on the exact DataValue variant (hash_join::canonical_join_key). +//! +//! Both go through the real SELECT INTO path so the temp tables are +//! materialized from the filtered view, exactly as script execution does. + +use sql_cli::data::datatable::{DataColumn, DataRow, DataTable, DataType, DataValue}; +use sql_cli::execution::{ExecutionContext, StatementExecutor}; +use sql_cli::sql::recursive_parser::Parser; +use std::sync::Arc; + +fn run(executor: &StatementExecutor, context: &mut ExecutionContext, sql: &str) { + let mut parser = Parser::new(sql); + let stmt = parser + .parse() + .unwrap_or_else(|e| panic!("parse failed for `{sql}`: {e}")); + executor + .execute(stmt, context) + .unwrap_or_else(|e| panic!("exec failed for `{sql}`: {e}")); +} + +fn sales_table() -> DataTable { + let mut table = DataTable::new("sales"); + table.add_column(DataColumn::new("id").with_type(DataType::Integer)); + table.add_column(DataColumn::new("product").with_type(DataType::String)); + table.add_column(DataColumn::new("quantity").with_type(DataType::Integer)); + for (id, product, qty) in [ + (1, "Widget", 10), + (2, "Gadget", 5), + (3, "Doohickey", 15), + (4, "Whatsit", 20), + ] { + let _ = table.add_row(DataRow { + values: vec![ + DataValue::Integer(id), + DataValue::String(product.to_string()), + DataValue::Integer(qty), + ], + }); + } + table +} + +#[test] +fn qualified_alias_in_select_resolves_on_temp_and_base_tables() { + let mut context = ExecutionContext::new(Arc::new(sales_table())); + let executor = StatementExecutor::new(); + + // Materialize a filtered temp table via the real SELECT INTO path. + run( + &executor, + &mut context, + "SELECT id, product, quantity INTO #hi FROM sales WHERE quantity > 10", + ); + + // Qualified columns against the aliased temp table (the reported failure). + let mut p = Parser::new("SELECT f.product, f.quantity FROM #hi f"); + let stmt = p.parse().expect("parse"); + let result = executor + .execute(stmt, &mut context) + .expect("qualified alias on temp table should resolve, not error"); + assert_eq!(result.dataview.row_count(), 2); // Doohickey + Whatsit + assert_eq!(result.dataview.column_count(), 2); + + // Same shape against a plain base table with an alias. + let mut p2 = Parser::new("SELECT s.product FROM sales s"); + let stmt2 = p2.parse().expect("parse"); + let result2 = executor + .execute(stmt2, &mut context) + .expect("qualified alias on base table should resolve"); + assert_eq!(result2.dataview.row_count(), 4); + assert_eq!(result2.dataview.column_count(), 1); +} + +#[test] +fn join_coerces_string_key_to_integer_key() { + let mut context = ExecutionContext::new(Arc::new(sales_table())); + let executor = StatementExecutor::new(); + + // #agents: integer agent_id. + let mut agents = DataTable::new("#agents"); + agents.add_column(DataColumn::new("agent_id").with_type(DataType::Integer)); + agents.add_column(DataColumn::new("agent_name").with_type(DataType::String)); + for (id, name) in [(1, "Ann"), (2, "Bob"), (3, "Cat")] { + let _ = agents.add_row(DataRow { + values: vec![DataValue::Integer(id), DataValue::String(name.to_string())], + }); + } + context + .store_temp_table("#agents".to_string(), Arc::new(agents)) + .expect("store #agents"); + + // #builds: agent reference stored as a STRING (as TeamCity/JSON+SUBSTR yields). + let mut builds = DataTable::new("#builds"); + builds.add_column(DataColumn::new("build_id").with_type(DataType::Integer)); + builds.add_column(DataColumn::new("f_agent_id").with_type(DataType::String)); + for (bid, aref) in [(10, "1"), (11, "2"), (12, "3"), (13, "99")] { + let _ = builds.add_row(DataRow { + values: vec![DataValue::Integer(bid), DataValue::String(aref.to_string())], + }); + } + context + .store_temp_table("#builds".to_string(), Arc::new(builds)) + .expect("store #builds"); + + let mut p = Parser::new( + "SELECT build_id, agent_name \ + FROM #builds b \ + LEFT JOIN #agents a ON b.f_agent_id = a.agent_id", + ); + let stmt = p.parse().expect("parse"); + let result = executor.execute(stmt, &mut context).expect("join exec"); + + // 4 builds; "1"/"2"/"3" coerce-match agents, "99" stays unmatched (NULL). + assert_eq!(result.dataview.row_count(), 4); + let src = result.dataview.source(); + let name_idx = src + .get_column_index("agent_name") + .expect("agent_name column present in join result"); + let matched = (0..result.dataview.row_count()) + .filter_map(|r| src.get_value(r, name_idx)) + .filter(|v| !matches!(v, DataValue::Null)) + .count(); + assert_eq!(matched, 3, "string keys should coerce-match integer keys"); +} + +#[test] +fn join_does_not_coerce_when_both_keys_are_strings() { + // When both join columns are strings, numeric coercion is OFF: exact string + // equality applies, so "07" does not match "7". Casting both sides to + // strings is the deliberate opt-out of numeric matching. + let mut context = ExecutionContext::new(Arc::new(sales_table())); + let executor = StatementExecutor::new(); + + let mut agents = DataTable::new("#agents"); + agents.add_column(DataColumn::new("agent_id").with_type(DataType::String)); + agents.add_column(DataColumn::new("agent_name").with_type(DataType::String)); + for (id, name) in [("7", "Ann"), ("8", "Bob")] { + let _ = agents.add_row(DataRow { + values: vec![ + DataValue::String(id.to_string()), + DataValue::String(name.to_string()), + ], + }); + } + context + .store_temp_table("#agents".to_string(), Arc::new(agents)) + .expect("store #agents"); + + let mut builds = DataTable::new("#builds"); + builds.add_column(DataColumn::new("build_id").with_type(DataType::Integer)); + builds.add_column(DataColumn::new("f_agent_id").with_type(DataType::String)); + for (bid, aref) in [(10, "07"), (11, "8")] { + let _ = builds.add_row(DataRow { + values: vec![DataValue::Integer(bid), DataValue::String(aref.to_string())], + }); + } + context + .store_temp_table("#builds".to_string(), Arc::new(builds)) + .expect("store #builds"); + + let mut p = Parser::new( + "SELECT build_id, agent_name \ + FROM #builds b \ + LEFT JOIN #agents a ON b.f_agent_id = a.agent_id", + ); + let stmt = p.parse().expect("parse"); + let result = executor.execute(stmt, &mut context).expect("join exec"); + + assert_eq!(result.dataview.row_count(), 2); + let src = result.dataview.source(); + let name_idx = src + .get_column_index("agent_name") + .expect("agent_name column present"); + let matched = (0..result.dataview.row_count()) + .filter_map(|r| src.get_value(r, name_idx)) + .filter(|v| !matches!(v, DataValue::Null)) + .count(); + // Only "8" == "8" matches; "07" != "7" because strings are not coerced. + assert_eq!(matched, 1, "string vs string must use exact text equality"); +}