You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "DanTm99 (via GitHub)" <gi...@apache.org> on 2023/02/10 18:02:18 UTC
[GitHub] [arrow] DanTm99 opened a new pull request, #34125: GH-34076: [C#] Allow schema fields with duplicate names
DanTm99 opened a new pull request, #34125:
URL: https://github.com/apache/arrow/pull/34125
Skip fields with duplicate names when building the `_fieldsDictionary` in the Schema constructor.
This allows schemas to have multiple fields with the same name, as the field name is used as the key for this dictionary.
Deprecate the existing `IReadOnlyDictionary<string, Field> Fields` and expose a new `IReadOnlyList<Field> FieldList`. Replace usages of `Fields` with `FieldList`
This allows for access to fields that were omitted from the `_fieldsDictionary`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a diff in pull request #34125: GH-34076: [C#] Allow schema fields with duplicate names
Posted by "eerhardt (via GitHub)" <gi...@apache.org>.
eerhardt commented on code in PR #34125:
URL: https://github.com/apache/arrow/pull/34125#discussion_r1103317618
##########
csharp/src/Apache.Arrow/Schema.cs:
##########
@@ -22,32 +22,31 @@ namespace Apache.Arrow
{
public partial class Schema
{
- public IReadOnlyDictionary<string, Field> Fields
- {
- get => _fieldsDictionary;
- }
+ [Obsolete("Use `FieldList` instead")]
+ public IReadOnlyDictionary<string, Field> Fields => _fieldsDictionary;
Review Comment:
We could still support this API, couldn't we? We could use a custom `IReadOnlyDictionary` that stored multiple `Field` instances per string. And when someone tried accessing a field by name, if there was more than 1 field, we could define behavior of either:
* throw an exception
* return the last field with that name
Of course in a "normal" case where the field names didn't conflict, it would still work as it does today.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] DanTm99 commented on a diff in pull request #34125: GH-34076: [C#] Allow schema fields with duplicate names
Posted by "DanTm99 (via GitHub)" <gi...@apache.org>.
DanTm99 commented on code in PR #34125:
URL: https://github.com/apache/arrow/pull/34125#discussion_r1103597114
##########
csharp/src/Apache.Arrow/Schema.cs:
##########
@@ -22,32 +22,31 @@ namespace Apache.Arrow
{
public partial class Schema
{
- public IReadOnlyDictionary<string, Field> Fields
- {
- get => _fieldsDictionary;
- }
+ [Obsolete("Use `FieldList` instead")]
+ public IReadOnlyDictionary<string, Field> Fields => _fieldsDictionary;
Review Comment:
When enumerating the key value pairs, either:
- multiple key value pairs would have the same key
- some fields would be missing
In the remarks for the `IReadOnlyDictionary` docs, it is stated that:
> Each pair must have a unique key.
This means the former would violate this and could lead to behaviour that the user would not expect.
If both
- a single field is returned (instead of throwing an exception) when someone tries accessing a field by name for which there are multiple fields
- duplicate fields are skipped when enumerating
then this is equivalent to just skipping duplicate keys when building the dictionary.
This API can still be supported by skipping duplicate keys when building the dictionary, but that would lead to the dictionary being an inaccurate representation of the state of the schema when there are multiple fields with the same name. Because of this, deprecating it could make it clear that using it is to be avoided.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #34125: GH-34076: [C#] Allow schema fields with duplicate names
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #34125:
URL: https://github.com/apache/arrow/pull/34125#issuecomment-1454851153
Benchmark runs are scheduled for baseline = f7506497c94b2429fcfb557d03b4b453bf564c7b and contender = 4b31aa4f4d76b629e62f2223cd13906f6c911ccb. 4b31aa4f4d76b629e62f2223cd13906f6c911ccb is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/411e9409c51d4eb9a37e339c181493b2...876cfe5fa2e84e5b80b563674dc9dca2/)
[Failed :arrow_down:0.25% :arrow_up:0.0%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/796d61aeff9040a1acc6f16784c08622...7de908e882894ff8bfe36ce5d3e6f853/)
[Finished :arrow_down:0.77% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/687eb7b56bcd4d83bab9bbb939c931d1...11cc99f4fcfa44fc88dcc00c791cf80e/)
[Finished :arrow_down:0.25% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/73c2553ead82444a832a82e313b1ac74...3afd98affba24a09b157529c53ad4566/)
Buildkite builds:
[Finished] [`4b31aa4f` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2473)
[Failed] [`4b31aa4f` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2503)
[Finished] [`4b31aa4f` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2471)
[Finished] [`4b31aa4f` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2494)
[Finished] [`f7506497` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2472)
[Failed] [`f7506497` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2502)
[Finished] [`f7506497` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2470)
[Finished] [`f7506497` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2493)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] DanTm99 commented on a diff in pull request #34125: GH-34076: [C#] Allow schema fields with duplicate names
Posted by "DanTm99 (via GitHub)" <gi...@apache.org>.
DanTm99 commented on code in PR #34125:
URL: https://github.com/apache/arrow/pull/34125#discussion_r1103597114
##########
csharp/src/Apache.Arrow/Schema.cs:
##########
@@ -22,32 +22,31 @@ namespace Apache.Arrow
{
public partial class Schema
{
- public IReadOnlyDictionary<string, Field> Fields
- {
- get => _fieldsDictionary;
- }
+ [Obsolete("Use `FieldList` instead")]
+ public IReadOnlyDictionary<string, Field> Fields => _fieldsDictionary;
Review Comment:
IF we were to use a custom `IReadOnlyDictionary`. when enumerating the key value pairs either:
- multiple key value pairs would have the same key
- fields with duplicate keys would be missing
In the remarks for the `IReadOnlyDictionary` Microsoft docs, it is stated that:
> Each pair must have a unique key.
This means the former would violate this and could lead to behaviour that the user would not expect.
If both
- a single field is returned (instead of an exception being thrown) when someone tries accessing a field by name for which there are multiple fields
- duplicate fields are skipped when enumerating
then this is equivalent to just skipping duplicate keys when building the dictionary.
This API can still be supported by skipping duplicate keys when building the dictionary, but that would lead to the dictionary being an inaccurate representation of the state of the schema when there are multiple fields with the same name. Because of this, deprecating it could make it clear that using it is to be avoided.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a diff in pull request #34125: GH-34076: [C#] Allow schema fields with duplicate names
Posted by "eerhardt (via GitHub)" <gi...@apache.org>.
eerhardt commented on code in PR #34125:
URL: https://github.com/apache/arrow/pull/34125#discussion_r1117341439
##########
csharp/src/Apache.Arrow/Schema.cs:
##########
@@ -44,80 +43,74 @@ public partial class Schema
IEnumerable<Field> fields,
IEnumerable<KeyValuePair<string, string>> metadata)
{
- if (fields == null)
+ if (fields is null)
{
throw new ArgumentNullException(nameof(fields));
}
- _fields = fields.ToList();
-
- _fieldsDictionary = fields.ToDictionary(field => field.Name, field => field);
+ _fieldsList = fields.ToList();
+ FieldsLookup = _fieldsList.ToLookup(f => f.Name);
+ _fieldsDictionary = FieldsLookup.ToDictionary(g => g.Key, g => g.First());
Metadata = metadata?.ToDictionary(kv => kv.Key, kv => kv.Value);
}
- internal Schema(List<Field> fields, IReadOnlyDictionary<string, string> metadata, bool copyCollections)
+ internal Schema(List<Field> fieldsList, IReadOnlyDictionary<string, string> metadata, bool copyCollections)
{
- Debug.Assert(fields != null);
+ Debug.Assert(fieldsList != null);
Debug.Assert(copyCollections == false, "This internal constructor is to not copy the collections.");
- _fields = fields;
-
- _fieldsDictionary = fields.ToDictionary(field => field.Name, field => field);
+ _fieldsList = fieldsList;
+ FieldsLookup = _fieldsList.ToLookup(f => f.Name);
+ _fieldsDictionary = FieldsLookup.ToDictionary(g => g.Key, g => g.First());
Metadata = metadata;
}
- public Field GetFieldByIndex(int i)
- {
- return _fields[i];
- }
+ public Field GetFieldByIndex(int i) => _fieldsList[i];
- public Field GetFieldByName(string name) =>
- Fields.TryGetValue(name, out Field field) ? field : null;
+ public Field GetFieldByName(string name) => FieldsLookup[name].First();
Review Comment:
```suggestion
public Field GetFieldByName(string name) => FieldsLookup[name].FirstOrDefault();
```
##########
csharp/src/Apache.Arrow/Ipc/ArrowStreamWriter.cs:
##########
@@ -522,13 +522,12 @@ private protected async Task WriteDictionaryAsync(Field field, CancellationToken
long id = DictionaryMemo.GetId(field);
IArrowArray dictionary = DictionaryMemo.GetDictionary(id);
- var fieldsDictionary = new Dictionary<string, Field> {
- { dictionaryField.Name, dictionaryField } };
+ var fieldsList = new List<Field> { dictionaryField };
Review Comment:
```suggestion
var fields = new Field[] { dictionaryField };
```
Array implements `IReadOnlyList`, so we can just create an array here instead of a List and save on an allocation.
##########
csharp/src/Apache.Arrow/Schema.cs:
##########
@@ -44,80 +43,74 @@ public partial class Schema
IEnumerable<Field> fields,
IEnumerable<KeyValuePair<string, string>> metadata)
{
- if (fields == null)
+ if (fields is null)
{
throw new ArgumentNullException(nameof(fields));
}
- _fields = fields.ToList();
-
- _fieldsDictionary = fields.ToDictionary(field => field.Name, field => field);
+ _fieldsList = fields.ToList();
+ FieldsLookup = _fieldsList.ToLookup(f => f.Name);
+ _fieldsDictionary = FieldsLookup.ToDictionary(g => g.Key, g => g.First());
Metadata = metadata?.ToDictionary(kv => kv.Key, kv => kv.Value);
}
- internal Schema(List<Field> fields, IReadOnlyDictionary<string, string> metadata, bool copyCollections)
+ internal Schema(List<Field> fieldsList, IReadOnlyDictionary<string, string> metadata, bool copyCollections)
{
- Debug.Assert(fields != null);
+ Debug.Assert(fieldsList != null);
Debug.Assert(copyCollections == false, "This internal constructor is to not copy the collections.");
- _fields = fields;
-
- _fieldsDictionary = fields.ToDictionary(field => field.Name, field => field);
+ _fieldsList = fieldsList;
+ FieldsLookup = _fieldsList.ToLookup(f => f.Name);
+ _fieldsDictionary = FieldsLookup.ToDictionary(g => g.Key, g => g.First());
Metadata = metadata;
}
- public Field GetFieldByIndex(int i)
- {
- return _fields[i];
- }
+ public Field GetFieldByIndex(int i) => _fieldsList[i];
- public Field GetFieldByName(string name) =>
- Fields.TryGetValue(name, out Field field) ? field : null;
+ public Field GetFieldByName(string name) => FieldsLookup[name].First();
Review Comment:
Can you add a test here to ensure `null` is returned when a field doesn't exist?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #34125: GH-34076: [C#] Allow schema fields with duplicate names
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34125:
URL: https://github.com/apache/arrow/pull/34125#issuecomment-1426155958
* Closes: #34076
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace merged pull request #34125: GH-34076: [C#] Allow schema fields with duplicate names
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace merged PR #34125:
URL: https://github.com/apache/arrow/pull/34125
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #34125: GH-34076: [C#] Allow schema fields with duplicate names
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34125:
URL: https://github.com/apache/arrow/pull/34125#issuecomment-1426156025
:warning: GitHub issue #34076 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a diff in pull request #34125: GH-34076: [C#] Allow schema fields with duplicate names
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #34125:
URL: https://github.com/apache/arrow/pull/34125#discussion_r1109120934
##########
csharp/src/Apache.Arrow/Schema.cs:
##########
@@ -22,98 +22,91 @@ namespace Apache.Arrow
{
public partial class Schema
{
- public IReadOnlyDictionary<string, Field> Fields
- {
- get => _fieldsDictionary;
- }
-
+ [Obsolete("Use `FieldsList` instead")]
Review Comment:
```suggestion
[Obsolete("Use `FieldsLookup` instead")]
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] DanTm99 commented on a diff in pull request #34125: GH-34076: [C#] Allow schema fields with duplicate names
Posted by "DanTm99 (via GitHub)" <gi...@apache.org>.
DanTm99 commented on code in PR #34125:
URL: https://github.com/apache/arrow/pull/34125#discussion_r1109427363
##########
csharp/src/Apache.Arrow/Schema.cs:
##########
@@ -22,98 +22,91 @@ namespace Apache.Arrow
{
public partial class Schema
{
- public IReadOnlyDictionary<string, Field> Fields
- {
- get => _fieldsDictionary;
- }
-
+ [Obsolete("Use `FieldsList` instead")]
Review Comment:
Since `FieldsList` wasn't previously exposed, `Fields` was the only way of getting a list of all the fields (via `Fields.Values`).
So for some use cases `FieldsList` would be preferred over `Fields`, but for others `FieldsLookup` would be preferred over `Fields`. I've updated the message to mention both, so the user can decide which would be best to use for their use case.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] DanTm99 commented on a diff in pull request #34125: GH-34076: [C#] Allow schema fields with duplicate names
Posted by "DanTm99 (via GitHub)" <gi...@apache.org>.
DanTm99 commented on code in PR #34125:
URL: https://github.com/apache/arrow/pull/34125#discussion_r1108372002
##########
csharp/src/Apache.Arrow/Schema.cs:
##########
@@ -22,32 +22,31 @@ namespace Apache.Arrow
{
public partial class Schema
{
- public IReadOnlyDictionary<string, Field> Fields
- {
- get => _fieldsDictionary;
- }
+ [Obsolete("Use `FieldList` instead")]
+ public IReadOnlyDictionary<string, Field> Fields => _fieldsDictionary;
Review Comment:
I've added the `ILookup<string, Field>`
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #34125: GH-34076: [C#] Allow schema fields with duplicate names
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #34125:
URL: https://github.com/apache/arrow/pull/34125#issuecomment-1434935831
@eerhardt did you want to take another look? Otherwise I'll merge this later next week.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] DanTm99 commented on a diff in pull request #34125: GH-34076: [C#] Allow schema fields with duplicate names
Posted by "DanTm99 (via GitHub)" <gi...@apache.org>.
DanTm99 commented on code in PR #34125:
URL: https://github.com/apache/arrow/pull/34125#discussion_r1108236064
##########
csharp/src/Apache.Arrow/Schema.cs:
##########
@@ -22,32 +22,31 @@ namespace Apache.Arrow
{
public partial class Schema
{
- public IReadOnlyDictionary<string, Field> Fields
- {
- get => _fieldsDictionary;
- }
+ [Obsolete("Use `FieldList` instead")]
+ public IReadOnlyDictionary<string, Field> Fields => _fieldsDictionary;
Review Comment:
@westonpace Exposing an `ILookup<string, Field>` alongside an `IReadOnlyList<Field>` would provide more flexibility to users. I think doing so could be a good idea
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] DanTm99 commented on a diff in pull request #34125: GH-34076: [C#] Allow schema fields with duplicate names
Posted by "DanTm99 (via GitHub)" <gi...@apache.org>.
DanTm99 commented on code in PR #34125:
URL: https://github.com/apache/arrow/pull/34125#discussion_r1103597114
##########
csharp/src/Apache.Arrow/Schema.cs:
##########
@@ -22,32 +22,31 @@ namespace Apache.Arrow
{
public partial class Schema
{
- public IReadOnlyDictionary<string, Field> Fields
- {
- get => _fieldsDictionary;
- }
+ [Obsolete("Use `FieldList` instead")]
+ public IReadOnlyDictionary<string, Field> Fields => _fieldsDictionary;
Review Comment:
If we were to use a custom `IReadOnlyDictionary`. when enumerating the key value pairs either:
- multiple key value pairs would have the same key
- fields with duplicate keys would be missing
In the remarks for the `IReadOnlyDictionary` Microsoft docs, it is stated that:
> Each pair must have a unique key.
This means the former would violate this and could lead to behaviour that the user would not expect.
If both
- a single field is returned (instead of an exception being thrown) when someone tries accessing a field by name for which there are multiple fields
- duplicate fields are skipped when enumerating
then this is equivalent to just skipping duplicate keys when building the dictionary.
This API can still be supported by skipping duplicate keys when building the dictionary, but that would lead to the dictionary being an inaccurate representation of the state of the schema when there are multiple fields with the same name. Because of this, deprecating it could make it clear that using it is to be avoided.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a diff in pull request #34125: GH-34076: [C#] Allow schema fields with duplicate names
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #34125:
URL: https://github.com/apache/arrow/pull/34125#discussion_r1107797906
##########
csharp/src/Apache.Arrow/Schema.cs:
##########
@@ -22,32 +22,31 @@ namespace Apache.Arrow
{
public partial class Schema
{
- public IReadOnlyDictionary<string, Field> Fields
- {
- get => _fieldsDictionary;
- }
+ [Obsolete("Use `FieldList` instead")]
+ public IReadOnlyDictionary<string, Field> Fields => _fieldsDictionary;
Review Comment:
Do we want to add an `ILookup<string, Field>` which might be handy on wide schemas? It's still an API break but might be useful.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] DanTm99 commented on a diff in pull request #34125: GH-34076: [C#] Allow schema fields with duplicate names
Posted by "DanTm99 (via GitHub)" <gi...@apache.org>.
DanTm99 commented on code in PR #34125:
URL: https://github.com/apache/arrow/pull/34125#discussion_r1117415086
##########
csharp/src/Apache.Arrow/Ipc/ArrowStreamWriter.cs:
##########
@@ -522,13 +522,12 @@ private protected async Task WriteDictionaryAsync(Field field, CancellationToken
long id = DictionaryMemo.GetId(field);
IArrowArray dictionary = DictionaryMemo.GetDictionary(id);
- var fieldsDictionary = new Dictionary<string, Field> {
- { dictionaryField.Name, dictionaryField } };
+ var fieldsList = new List<Field> { dictionaryField };
Review Comment:
Done
##########
csharp/src/Apache.Arrow/Schema.cs:
##########
@@ -44,80 +43,74 @@ public partial class Schema
IEnumerable<Field> fields,
IEnumerable<KeyValuePair<string, string>> metadata)
{
- if (fields == null)
+ if (fields is null)
{
throw new ArgumentNullException(nameof(fields));
}
- _fields = fields.ToList();
-
- _fieldsDictionary = fields.ToDictionary(field => field.Name, field => field);
+ _fieldsList = fields.ToList();
+ FieldsLookup = _fieldsList.ToLookup(f => f.Name);
+ _fieldsDictionary = FieldsLookup.ToDictionary(g => g.Key, g => g.First());
Metadata = metadata?.ToDictionary(kv => kv.Key, kv => kv.Value);
}
- internal Schema(List<Field> fields, IReadOnlyDictionary<string, string> metadata, bool copyCollections)
+ internal Schema(List<Field> fieldsList, IReadOnlyDictionary<string, string> metadata, bool copyCollections)
{
- Debug.Assert(fields != null);
+ Debug.Assert(fieldsList != null);
Debug.Assert(copyCollections == false, "This internal constructor is to not copy the collections.");
- _fields = fields;
-
- _fieldsDictionary = fields.ToDictionary(field => field.Name, field => field);
+ _fieldsList = fieldsList;
+ FieldsLookup = _fieldsList.ToLookup(f => f.Name);
+ _fieldsDictionary = FieldsLookup.ToDictionary(g => g.Key, g => g.First());
Metadata = metadata;
}
- public Field GetFieldByIndex(int i)
- {
- return _fields[i];
- }
+ public Field GetFieldByIndex(int i) => _fieldsList[i];
- public Field GetFieldByName(string name) =>
- Fields.TryGetValue(name, out Field field) ? field : null;
+ public Field GetFieldByName(string name) => FieldsLookup[name].First();
Review Comment:
DOne
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] DanTm99 commented on a diff in pull request #34125: GH-34076: [C#] Allow schema fields with duplicate names
Posted by "DanTm99 (via GitHub)" <gi...@apache.org>.
DanTm99 commented on code in PR #34125:
URL: https://github.com/apache/arrow/pull/34125#discussion_r1117415381
##########
csharp/src/Apache.Arrow/Schema.cs:
##########
@@ -44,80 +43,74 @@ public partial class Schema
IEnumerable<Field> fields,
IEnumerable<KeyValuePair<string, string>> metadata)
{
- if (fields == null)
+ if (fields is null)
{
throw new ArgumentNullException(nameof(fields));
}
- _fields = fields.ToList();
-
- _fieldsDictionary = fields.ToDictionary(field => field.Name, field => field);
+ _fieldsList = fields.ToList();
+ FieldsLookup = _fieldsList.ToLookup(f => f.Name);
+ _fieldsDictionary = FieldsLookup.ToDictionary(g => g.Key, g => g.First());
Metadata = metadata?.ToDictionary(kv => kv.Key, kv => kv.Value);
}
- internal Schema(List<Field> fields, IReadOnlyDictionary<string, string> metadata, bool copyCollections)
+ internal Schema(List<Field> fieldsList, IReadOnlyDictionary<string, string> metadata, bool copyCollections)
{
- Debug.Assert(fields != null);
+ Debug.Assert(fieldsList != null);
Debug.Assert(copyCollections == false, "This internal constructor is to not copy the collections.");
- _fields = fields;
-
- _fieldsDictionary = fields.ToDictionary(field => field.Name, field => field);
+ _fieldsList = fieldsList;
+ FieldsLookup = _fieldsList.ToLookup(f => f.Name);
+ _fieldsDictionary = FieldsLookup.ToDictionary(g => g.Key, g => g.First());
Metadata = metadata;
}
- public Field GetFieldByIndex(int i)
- {
- return _fields[i];
- }
+ public Field GetFieldByIndex(int i) => _fieldsList[i];
- public Field GetFieldByName(string name) =>
- Fields.TryGetValue(name, out Field field) ? field : null;
+ public Field GetFieldByName(string name) => FieldsLookup[name].First();
Review Comment:
Done
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a diff in pull request #34125: GH-34076: [C#] Allow schema fields with duplicate names
Posted by "eerhardt (via GitHub)" <gi...@apache.org>.
eerhardt commented on code in PR #34125:
URL: https://github.com/apache/arrow/pull/34125#discussion_r1103317618
##########
csharp/src/Apache.Arrow/Schema.cs:
##########
@@ -22,32 +22,31 @@ namespace Apache.Arrow
{
public partial class Schema
{
- public IReadOnlyDictionary<string, Field> Fields
- {
- get => _fieldsDictionary;
- }
+ [Obsolete("Use `FieldList` instead")]
+ public IReadOnlyDictionary<string, Field> Fields => _fieldsDictionary;
Review Comment:
We could still support this API, couldn't we? We could use a custom `IReadOnlyDictionary` that stored multiple `Field` instances per string. And when someone tried accessing a field by name, if there was more than 1 field, we could define behavior of either:
* throw an exception
* return the last field with that name
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org