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

[C#] Consider replacing LINQ expression when accessing columns by name #44575

Closed
georgevanburgh opened this issue Oct 30, 2024 · 3 comments · Fixed by #44576
Closed

[C#] Consider replacing LINQ expression when accessing columns by name #44575

georgevanburgh opened this issue Oct 30, 2024 · 3 comments · Fixed by #44576

Comments

@georgevanburgh
Copy link
Contributor

Describe the enhancement requested

We've recently been doing some profiling of code that uses the C# Arrow library, and which frequently accesses columns by name in a tight loop - and found the following LINQ expression makes a significant contribution to execution time.

public int GetFieldIndex(string name, IEqualityComparer<string> comparer = default)
{
comparer ??= StringComparer.CurrentCulture;
return _fieldsList.IndexOf(_fieldsList.First(x => comparer.Equals(x.Name, name)));
}

Obviously there are workarounds for this, such as creating our own cached mapping of column names to ordinals - but it would be nice if the built-in method was a little more efficient. In addition to the use of linq, we're also iterating through _fieldsList twice (once to find a match, and then another to find the index of the match).

We've also noticed that usage of StringComparer.CurrentCulture causes an allocation every time this method is called (assuming a value for comparer is not provided). I'm assuming the choice to use a culture-aware comparison by default is intentional?

Component(s)

C#

@CurtHagenlocher
Copy link
Contributor

The use of StringComparer.CurrentCulture is definitely a head-scratcher. It apparently dates back to the very first checkin of the file. It's especially surprising because the very same checkin used StringComparer.OrdinalIgnoreCase to build the dictionary. While I ordinarily argue against making breaking changes, I think it might be justified this time.

I would expect the allocation to be a result of the closure that needs to be created to pass to First.

@CurtHagenlocher
Copy link
Contributor

Issue resolved by pull request 44576
#44576

@CurtHagenlocher CurtHagenlocher added this to the 19.0.0 milestone Nov 2, 2024
@vthemelis
Copy link

vthemelis commented Nov 5, 2024

The use of StringComparer.CurrentCulture is definitely a head-scratcher. It apparently dates back to the very first checkin of the file. It's especially surprising because the very same checkin used StringComparer.OrdinalIgnoreCase to build the dictionary. While I ordinarily argue against making breaking changes, I think it might be justified this time.

I would expect the allocation to be a result of the closure that needs to be created to pass to First.

Added #44650 for this as it's fairly confusing.

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

Successfully merging a pull request may close this issue.

3 participants