Conversation
|
|
|
|
@Platob do you plan to resolve the outstanding conflicts and submit this? I would really appreciate this feature! |
|
@Platob I've taken your branch, merged the latest changes from main and added support for Map in the C API. I'm currently working on adding a few more tests as well as Archery support. Would you like to pull the changes from my branch when I'm done or should I submit a separate PR? |
|
Okay, I'm done with my changes and I pushed them to https://github.com/CurtHagenlocher/arrow/tree/Map. |
|
Damn sorry forgot I still had this pending, thank you @CurtHagenlocher, I did a version where we would need more primary control to scale up on more complex structures, I'm using a version #35299 |
westonpace
left a comment
There was a problem hiding this comment.
I reviewed #37885 which (if I understand correctly) includes these changes and things look pretty good. Sorry that the review was so late. Please re-request review if we want to return to this PR for some reason.
### What changes are included in this PR? This change includes the original work by `@ Platob` from #35263, updated with the latest changes in the main branch. Additionally, it includes support in the C API for the Map type and fills a few gaps in functionality from the original change (for instance, ArrowArrayConcentrator support). Finally, it fixes a bug in the ArrowArrayConcentrator.Concatenate which prevented it from working correctly for structs. (This bug was discovered in the course of creating a test for concatenating maps.) ### Are these changes tested? Yes. ### Are there any user-facing changes? Maps are now supported in the C# implementation. Closes #35243 * Closes: #35243 Lead-authored-by: Platob <nfillot.pro@gmail.com> Co-authored-by: Curt Hagenlocher <curt@hagenlocher.org> Signed-off-by: David Li <li.davidm96@gmail.com>
|
Going to close this since the other PR was merged, but if you want to revisit this please feel free to reopen/file a new issue. |
### What changes are included in this PR? This change includes the original work by `@ Platob` from apache#35263, updated with the latest changes in the main branch. Additionally, it includes support in the C API for the Map type and fills a few gaps in functionality from the original change (for instance, ArrowArrayConcentrator support). Finally, it fixes a bug in the ArrowArrayConcentrator.Concatenate which prevented it from working correctly for structs. (This bug was discovered in the course of creating a test for concatenating maps.) ### Are these changes tested? Yes. ### Are there any user-facing changes? Maps are now supported in the C# implementation. Closes apache#35243 * Closes: apache#35243 Lead-authored-by: Platob <nfillot.pro@gmail.com> Co-authored-by: Curt Hagenlocher <curt@hagenlocher.org> Signed-off-by: David Li <li.davidm96@gmail.com>
### What changes are included in this PR? This change includes the original work by `@ Platob` from apache#35263, updated with the latest changes in the main branch. Additionally, it includes support in the C API for the Map type and fills a few gaps in functionality from the original change (for instance, ArrowArrayConcentrator support). Finally, it fixes a bug in the ArrowArrayConcentrator.Concatenate which prevented it from working correctly for structs. (This bug was discovered in the course of creating a test for concatenating maps.) ### Are these changes tested? Yes. ### Are there any user-facing changes? Maps are now supported in the C# implementation. Closes apache#35243 * Closes: apache#35243 Lead-authored-by: Platob <nfillot.pro@gmail.com> Co-authored-by: Curt Hagenlocher <curt@hagenlocher.org> Signed-off-by: David Li <li.davidm96@gmail.com>
Implement new MapType with array
Tested with IPC tests