Skip to content

Conversation

@Wan95u
Copy link

@Wan95u Wan95u commented Jan 19, 2026

Purpose of this pull request

Fixed the issue where Excel output file column order was incorrect. Previously, columns were output in original index order instead of the configured sink_columns order.

Does this PR introduce any user-facing change?

Yes. This fix changes Excel output column order from incorrect (original index order) to correct (sink_columns order). Users who relied on the previous incorrect order will see different output after upgrade.

How was this patch tested?

Added a new test case with reordered columns [3, 1, 2, 0] to verify the fix. All existing tests also pass.

Check list

@DanielCarter-stack
Copy link

Thanks! Two MINOR suggestions:

  • ExcelGeneratorTest.java:78 - Test uses sinkColumnsIndexInRow = Arrays.asList(0, 1, 2, 3) with consecutive indices. With consecutive indices, the old code createCell(i) and new code createCell(index++) produce identical results (since i == index). The bug only manifests with non-consecutive indices (e.g., [3, 1, 2, 0]), so the current test cannot verify the fix. Consider adding a test with reordered indices to prevent regression.

  • PR Description - The "Does this PR introduce any user-facing change?" section is empty. Since this fix changes Excel output column order from incorrect (original index order) to correct (sink_columns order), users who relied on the previous incorrect order will see different output after upgrade. Documenting this behavior change would help with release notes and user migration.

@corgy-w
Copy link
Contributor

corgy-w commented Jan 19, 2026

Thank you for your contribution. You should add the corresponding test cases

@Wan95u
Copy link
Author

Wan95u commented Jan 20, 2026

Okay, I will proceed to add test cases in the coming days.

Copy link
Collaborator

@chl-wxp chl-wxp left a comment

Choose a reason for hiding this comment

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

LGTM

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants