Skip to content

Conversation

@stuarttimwhite
Copy link
Contributor

Please be sure to look over the pull request guidelines here: https://github.com/spaceandtimelabs/sxt-proof-of-sql/blob/main/CONTRIBUTING.md#submit-pr.

Please go through the following checklist

Rationale for this change

What changes are included in this PR?

Are these changes tested?

@stuarttimwhite stuarttimwhite force-pushed the fix/planner-incorrectly-filtermaps-on-join-exprs branch from 4ce2128 to 7a54647 Compare December 5, 2025 17:04
@codecov
Copy link

codecov bot commented Dec 5, 2025

Codecov Report

❌ Patch coverage is 80.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.77%. Comparing base (3c43ab1) to head (7a54647).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
crates/proof-of-sql-planner/src/plan.rs 80.00% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1104   +/-   ##
=======================================
  Coverage   96.77%   96.77%           
=======================================
  Files         302      302           
  Lines       59220    59223    +3     
=======================================
+ Hits        57310    57313    +3     
  Misses       1910     1910           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@stuarttimwhite stuarttimwhite marked this pull request as ready for review December 5, 2025 18:53
@iajoiner iajoiner requested a review from Copilot December 5, 2025 19:01
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a bug in the join planner where missing join columns were silently ignored instead of raising an error. Previously, the code used filter_map which would skip over columns that couldn't be found in the table schemas, potentially leading to incorrect join operations. The fix changes to map and explicitly converts None values from get_index_of() to PlannerError::ColumnNotFound.

Key Changes

  • Changed from filter_map to map in join column processing to ensure all columns are validated
  • Added explicit error handling using ok_or(PlannerError::ColumnNotFound)? when join columns are not found in table schemas
  • This ensures the planner fails fast with a clear error message instead of silently continuing with incomplete join information

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +327 to 345
.map(|(left_expr, right_expr)| match (left_expr, right_expr) {
(Expr::Column(col_a), Expr::Column(col_b)) if col_a.name == col_b.name => {
let column_id = Ident::new(col_a.name.clone());
Ok((
(
left_column_result_fields
.get_index_of(&column_id)
.ok_or(PlannerError::ColumnNotFound)?,
right_column_result_fields
.get_index_of(&column_id)
.ok_or(PlannerError::ColumnNotFound)?,
),
column_id,
))
}
_ => Err(PlannerError::UnsupportedLogicalPlan {
plan: Box::new(plan.clone()),
}),
})
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new error handling for missing join columns (ColumnNotFound errors on lines 334 and 337) lacks test coverage. Consider adding a test case that verifies a ColumnNotFound error is raised when join columns are not present in the left or right table schemas.

Example test structure:

#[test]
fn we_cannot_convert_join_with_missing_column() {
    // Create tables where join column doesn't exist in one or both tables
    // Attempt to join on the missing column
    // Assert that result matches Err(PlannerError::ColumnNotFound)
}

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants