Skip to content

Commit bd617d6

Browse files
TimelordUKclaude
andcommitted
fix: Make preprocessing pipeline run by default
Makes HAVING auto-aliasing and other transformers work without requiring the --check-in-lifting flag. Preprocessing now runs automatically for all queries with FROM clauses. Changes: - Removed conditional `if config.lift_in_expressions` check - Preprocessing now runs by default for queries with FROM clauses - Queries without FROM clauses skip preprocessing (special row-iteration semantics) - `--show-preprocessing` flag still works to display statistics User Impact: - HAVING with aggregates now works out of the box: SELECT b, SUM(a) FROM t GROUP BY b HAVING SUM(a) > 10 ✅ Works! - No need for --check-in-lifting flag for basic queries - --check-in-lifting flag still exists for backward compatibility Technical Details: - Queries without FROM bypass preprocessing (use QueryExecutionService) - Queries with FROM go through preprocessing pipeline - QueryEngine used only when transformations are applied - Preserves special behavior for queries without FROM Example: # Before: Required flag sql-cli data.csv -q "..." --check-in-lifting # After: Works without flag sql-cli data.csv -q "..." Note: One Python test (test_null_literal.py::test_coalesce_with_null_literal) has pre-existing expectations that don't match actual behavior. This is unrelated to preprocessing changes - test expects 3 rows for a query without FROM, but tool returns 1 row (verified on commits before this PR). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 6b944be commit bd617d6

1 file changed

Lines changed: 42 additions & 29 deletions

File tree

src/non_interactive.rs

Lines changed: 42 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -406,22 +406,50 @@ pub fn execute_non_interactive(config: NonInteractiveConfig) -> Result<()> {
406406
behavior_config.hide_empty_columns = true;
407407
}
408408

409-
// Parse and potentially rewrite the query using preprocessing pipeline
409+
// Parse and rewrite the query using preprocessing pipeline
410+
// Preprocessing now runs by default (not just with --check-in-lifting)
410411
let exec_start = Instant::now();
411-
let result = if config.lift_in_expressions {
412+
let result = {
412413
use crate::data::query_engine::QueryEngine;
413414
use crate::sql::recursive_parser::Parser;
414415

415416
let mut parser = Parser::new(&config.query);
416417
match parser.parse() {
417418
Ok(stmt) => {
418-
// Apply preprocessing pipeline with all transformers
419-
let mut pipeline =
420-
crate::query_plan::create_standard_pipeline(config.show_preprocessing);
421-
let stmt = pipeline.process(stmt)?;
419+
// Check if query has a FROM clause - queries without FROM need special handling
420+
let has_from_clause = stmt.from_table.is_some() || stmt.from_subquery.is_some() || stmt.from_function.is_some();
422421

423-
// Show statistics if requested
424-
if config.show_preprocessing {
422+
// Only apply preprocessing if there's a FROM clause
423+
// (queries without FROM have special row-iteration semantics in this tool)
424+
if !has_from_clause {
425+
// Use legacy path for queries without FROM
426+
let query_service =
427+
QueryExecutionService::with_behavior_config(behavior_config);
428+
if config.debug_trace {
429+
let debug_ctx = crate::debug_trace::DebugContext::new(
430+
crate::debug_trace::DebugLevel::Debug,
431+
);
432+
query_service.execute_with_debug(
433+
&config.query,
434+
Some(&dataview),
435+
Some(dataview.source()),
436+
Some(debug_ctx),
437+
)
438+
} else {
439+
query_service.execute(
440+
&config.query,
441+
Some(&dataview),
442+
Some(dataview.source()),
443+
)
444+
}
445+
} else {
446+
// Apply preprocessing pipeline with all transformers
447+
let mut pipeline =
448+
crate::query_plan::create_standard_pipeline(config.show_preprocessing);
449+
let stmt = pipeline.process(stmt)?;
450+
451+
// Show statistics if requested
452+
if config.show_preprocessing {
425453
eprintln!("\n{}", "=== Preprocessing Pipeline ===".to_string());
426454
eprintln!("{}", pipeline.stats().summary());
427455
for transform_stats in &pipeline.stats().transformations {
@@ -439,10 +467,11 @@ pub fn execute_non_interactive(config: NonInteractiveConfig) -> Result<()> {
439467
eprintln!();
440468
}
441469

442-
// Check if any transformations were applied
443-
let transformers_applied = pipeline.stats().transformers_applied > 0;
444-
if transformers_applied {
445-
// Execute the rewritten AST directly
470+
// Check if any transformations were applied
471+
let transformers_applied = pipeline.stats().transformers_applied > 0;
472+
473+
if transformers_applied {
474+
// Execute the rewritten AST directly using QueryEngine
446475
let engine = QueryEngine::with_case_insensitive(config.case_insensitive);
447476

448477
match engine.execute_statement(dataview.source_arc(), stmt) {
@@ -488,6 +517,7 @@ pub fn execute_non_interactive(config: NonInteractiveConfig) -> Result<()> {
488517
)
489518
}
490519
}
520+
}
491521
}
492522
Err(_) => {
493523
// Parse failed, execute normally and let it fail with proper error
@@ -507,23 +537,6 @@ pub fn execute_non_interactive(config: NonInteractiveConfig) -> Result<()> {
507537
}
508538
}
509539
}
510-
} else {
511-
// Normal execution without lifting
512-
let query_service = QueryExecutionService::with_behavior_config(behavior_config);
513-
514-
// Create debug context if debug trace is enabled
515-
if config.debug_trace {
516-
let debug_ctx =
517-
crate::debug_trace::DebugContext::new(crate::debug_trace::DebugLevel::Debug);
518-
query_service.execute_with_debug(
519-
&config.query,
520-
Some(&dataview),
521-
Some(dataview.source()),
522-
Some(debug_ctx),
523-
)
524-
} else {
525-
query_service.execute(&config.query, Some(&dataview), Some(dataview.source()))
526-
}
527540
}?;
528541
let exec_time = exec_start.elapsed();
529542

0 commit comments

Comments
 (0)