CurtHagenlocher commented on code in PR #44633:
URL: https://github.com/apache/arrow/pull/44633#discussion_r1847329703
##########
csharp/src/Apache.Arrow/Schema.cs:
##########
@@ -31,6 +31,7 @@ public partial class Schema : IRecordType
private readonly List<Field> _fieldsList;
public ILookup<string, Field> FieldsLookup { get; }
+ private readonly ILookup<string, int> _fieldsIndexLookup;
Review Comment:
Can we also expose this in a way that lets users get all the indexes for a
particular field name?
##########
csharp/src/Apache.Arrow/Schema.cs:
##########
@@ -80,15 +79,20 @@ public int GetFieldIndex(string name, StringComparer
comparer)
public int GetFieldIndex(string name, IEqualityComparer<string>
comparer = default)
{
- comparer ??= StringComparer.CurrentCulture;
+ if (comparer == null ||
comparer.Equals(StringComparer.CurrentCulture))
+ {
+ return _fieldsIndexLookup[name].First();
+ }
- for (int i = 0; i < _fieldsList.Count; i++)
+ for (var i = 0; i < _fieldsList.Count; ++i)
{
if (comparer.Equals(_fieldsList[i].Name, name))
+ {
return i;
+ }
}
- return -1;
+ throw new InvalidOperationException();
Review Comment:
This is a breaking change. Is there a specific reason for it?
##########
csharp/src/Apache.Arrow/Schema.cs:
##########
@@ -66,6 +61,10 @@ internal Schema(List<Field> fieldsList,
IReadOnlyDictionary<string, string> meta
_fieldsDictionary = FieldsLookup.ToDictionary(g => g.Key, g =>
g.First());
Metadata = metadata;
+
+ _fieldsIndexLookup = _fieldsList
+ .Select((x, idx) => (Name: x.Name, Index: idx))
+ .ToLookup(x => x.Name, x => x.Index,
StringComparer.CurrentCulture);
Review Comment:
I think for new code, we should use `Ordinal`. I don't think
`CurrentCulture` really makes sense here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
