feat(kernel): pass ColumnMappingMode to physical_name

[aleksandarskrbic]

Fixes #1120

[codecov]

Codecov Report

❌ Patch coverage is 94.73684% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.92%. Comparing base (e1faff7) to head (1ffe747).
⚠️ Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
kernel/src/scan/log_replay.rs	90.00%	1 Missing ⚠️
kernel/src/table_changes/log_replay/tests.rs	75.00%	1 Missing ⚠️
kernel/src/table_changes/physical_to_logical.rs	90.00%	1 Missing ⚠️
kernel/src/table_changes/scan.rs	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1329      +/-   ##
==========================================
+ Coverage   84.90%   84.92%   +0.02%     
==========================================
  Files         113      113              
  Lines       28923    28973      +50     
  Branches    28923    28973      +50     
==========================================
+ Hits        24556    24606      +50     
- Misses       3197     3200       +3     
+ Partials     1170     1167       -3     

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

[nicklan]

Thanks for the PR!

This is actually a little tricky because I don't think we want to be passing the column_mapping_mode everywhere like this, mainly because it's a very specific piece of information that we're plumbing through and we should actually plumb through the whole TableConfiguration so we don't need to make these deep changes again when we need some other bit of config deep in transform generation.

I realize that we have an Arc<StateInfo> in our ScanLogReplayProcessor. I think we should be able to just pass that into AddRemoveDedupVisitor::new, which actually provides the logical_schema, physical_schema, and transform_spec too, so we can greatly reduce the number of arguments to that new method which is good. Then you can just pull the column-mapping-mode out of the StateInfo.

[aleksandarskrbic]

Thanks for the PR!

This is actually a little tricky because I don't think we want to be passing the column_mapping_mode everywhere like this, mainly because it's a very specific piece of information that we're plumbing through and we should actually plumb through the whole TableConfiguration so we don't need to make these deep changes again when we need some other bit of config deep in transform generation.

I realize that we have an Arc<StateInfo> in our ScanLogReplayProcessor. I think we should be able to just pass that into AddRemoveDedupVisitor::new, which actually provides the logical_schema, physical_schema, and transform_spec too, so we can greatly reduce the number of arguments to that new method which is good. Then you can just pull the column-mapping-mode out of the StateInfo.

@nicklan Hi, thanks for feedback, I just opened new PR for this issue => #1403