Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

When creating the "missing column" error message, use the same (case insensitive) column name comparer used in field value retrieval #33749

Merged
merged 1 commit into from
Jun 12, 2024
Merged

When creating the "missing column" error message, use the same (case insensitive) column name comparer used in field value retrieval #33749

merged 1 commit into from
Jun 12, 2024

Conversation

simonmckenzie
Copy link
Contributor

@simonmckenzie simonmckenzie commented May 18, 2024

Issue #33748

This addresses an issue where a column with different casing to the entity's field would be selected as the "first missing column" when assembling the error message. The field name lookup's key comparer is case insensitive, and using it here ensures that all column name comparisons will use the same comparer.

  • I've read the guidelines for contributing and seen the walkthrough
  • I've posted a comment on an issue with a detailed description of how I am planning to contribute and got approval from a member of the team
  • The code builds and tests pass locally (also verified by our automated build checks)
  • Commit messages follow this format:
        Summary of the changes
        - Detail 1
        - Detail 2

        Fixes #bugnumber
  • Tests for the changes have been added (for bug fixes / features)
  • Code follows the same patterns and style as existing code in this repo

…insensitive) column name comparer used in field value retrieval

- Update the existing code that finds the "first missing column" to use the `_fieldNameLookup`, as its key comparer is case insensitive.

Fixes #33748
@simonmckenzie simonmckenzie changed the title Use the field name lookup when matching column names When creating the "missing column" error message, use the same (case insensitive) column name comparer used in field value retrieval May 18, 2024
@simonmckenzie
Copy link
Contributor Author

simonmckenzie commented May 18, 2024 via email

@cincuranet cincuranet requested review from ajcvickers and roji May 24, 2024 09:15
@roji
Copy link
Member

roji commented May 27, 2024

Note: this PR is waiting on a design discussion on #33748

@simonmckenzie
Copy link
Contributor Author

Hi @roji,

Since the parent issue (#33748) has been put into the backlog pending work on provider-level case-sensitivity settings, this PR is now in an uncertain state.

I would argue that since it addresses a known bug (inconsistent case comparison within the BufferedDataReader), it makes sense to still proceed with this fix now. Additionally, its centralisation of the column comparison logic (plus extra tests) should help with the future case-sensitivity work.

If this isn't desired, I guess I should close this PR.

Please tell me what you think.

@roji
Copy link
Member

roji commented Jun 5, 2024

Sorry, I forgot a PR was out - should have commented here. Given that we don't intend to break e.g. the SQL Server provider by changing to be case-sensitive, I think fixing the bug definitely still makes sense - one of us will review it soon.

@cincuranet cincuranet merged commit 22fa9d5 into dotnet:main Jun 12, 2024
7 checks passed
@cincuranet
Copy link
Contributor

Thanks.

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