Skip to content

Conversation

@aleksandarskrbic
Copy link
Contributor

Fixes #1120

What changes are proposed in this pull request?

This PR makes StructField::physical_name() aware of ColumnMappingMode by adding it as a required parameter.

Previously, physical_name() would always return the physical name from metadata if present, regardless of the column mapping mode. This was incorrect behavior - when column mapping mode is
None, the logical name should be used even if physical name metadata exists in the field.

The key changes are:

  1. Updated StructField::physical_name() to take column_mapping_mode: ColumnMappingMode parameter
  2. When mode is None, always return the logical name
  3. When mode is Id or Name, return the physical name from metadata (or logical name as fallback)
  4. Added column_mapping_mode field to StateInfo struct to avoid threading it through many function calls
  5. Updated all call sites throughout the codebase to pass the column mapping mode
  6. Refactored AddRemoveDedupVisitor to accept Arc instead of 8 individual parameters, following feedback to avoid excessive parameter passing

@codecov
Copy link

codecov bot commented Oct 17, 2025

Codecov Report

❌ Patch coverage is 96.96970% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.66%. Comparing base (2ec1462) to head (9987545).

Files with missing lines Patch % Lines
kernel/src/scan/log_replay.rs 94.44% 1 Missing ⚠️
kernel/src/table_changes/log_replay/tests.rs 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1403      +/-   ##
==========================================
+ Coverage   84.65%   84.66%   +0.01%     
==========================================
  Files         115      115              
  Lines       29557    29589      +32     
  Branches    29557    29589      +32     
==========================================
+ Hits        25021    25053      +32     
- Misses       3329     3330       +1     
+ Partials     1207     1206       -1     

☔ 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.

@github-actions github-actions bot added the breaking-change Change that require a major version bump label Oct 17, 2025
@nicklan nicklan requested review from nicklan and zachschuermann and removed request for nicklan October 23, 2025 18:31
Copy link
Collaborator

@nicklan nicklan left a comment

Choose a reason for hiding this comment

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

Hey, this looks great and pretty much in the direction we want to go. Unfortunately another PR got merged which reorganized things a bit. I do apologize for that, if you'd like I can merge main for you and clean up the conflicts, or if you have time you can as well.

But I think once we get main merged in this should be pretty much good to go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking-change Change that require a major version bump

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make StructField.physical_name take ColumnMappingMode

2 participants