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

GH-35243: [C#] Implement MapType #37885

Merged
merged 28 commits into from
Oct 5, 2023
Merged

GH-35243: [C#] Implement MapType #37885

merged 28 commits into from
Oct 5, 2023

Conversation

CurtHagenlocher
Copy link
Contributor

@CurtHagenlocher CurtHagenlocher commented Sep 26, 2023

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

Copy link
Contributor

@davidhcoe davidhcoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few nits on comments but looks good

csharp/src/Apache.Arrow/C/CArrowSchemaImporter.cs Outdated Show resolved Hide resolved
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 2, 2023
@davidhcoe
Copy link
Contributor

@lidavidm - are you able to help at all?

@lidavidm
Copy link
Member

lidavidm commented Oct 4, 2023

Thanks for the ping - I'll look at this tomorrow!

Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

I restarted the integration pipeline so let's make sure things pass there.

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Oct 5, 2023
@lidavidm
Copy link
Member

lidavidm commented Oct 5, 2023

Hmm, there appears to be something wrong with the pipeline

Building wheels for collected packages: pythonnet
  Building wheel for pythonnet (setup.py): started
  Building wheel for pythonnet (setup.py): finished with status 'error'
  error: subprocess-exited-with-error
  
  × python setup.py bdist_wheel did not run successfully.
  │ exit code: 1
  ╰─> [59 lines of output]
      /tmp/pip-install-m_41mbf1/pythonnet_3828907f28a2453cac8f6ff2d84ed995/setup.py:468: SyntaxWarning: invalid escape sequence '\*'
        "MSBuild\**\Bin\MSBuild.exe",
      /tmp/pip-install-m_41mbf1/pythonnet_3828907f28a2453cac8f6ff2d84ed995/setup.py:533: SyntaxWarning: invalid escape sequence '\*'
        "MSBuild\**\Bin\MSBuild.exe",
      running bdist_wheel
      running build
      running build_ext
      /bin/sh: 1: mono: not found

@lidavidm
Copy link
Member

lidavidm commented Oct 5, 2023

Ah, weird, it tried to install 2.5.2 instead of 3.0.2.

@CurtHagenlocher can you rebase/merge one last time? It appears we recently had to pin Python to keep pythonnet working

@CurtHagenlocher
Copy link
Contributor Author

CurtHagenlocher commented Oct 5, 2023

Looks like another infrastructure problem

################# FAILURES #################
FAILED TEST: recursive_nested Rust producing,  Rust consuming
<class 'RuntimeError'>: Command failed: /arrow/rust/target/debug/flight-test-integration-client --host localhost --port=46587 --path /tmp/arrow-integration-o7e_thtg/generated_recursive_nested.json
With output:
--------------
Error: tonic::transport::Error(Transport, hyper::Error(Connect, ConnectError("tcp connect error", Os { code: 99, kind: AddrNotAvailable, message: "Cannot assign requested address" })))

@lidavidm
Copy link
Member

lidavidm commented Oct 5, 2023

Well, the C# tests pass so I think let's merge this.

@lidavidm lidavidm merged commit 4b795ec into apache:main Oct 5, 2023
11 of 12 checks passed
@lidavidm lidavidm removed the awaiting merge Awaiting merge label Oct 5, 2023
@conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 4b795ec.

There was 1 benchmark result indicating a performance regression:

The full Conbench report has more details. It also includes information about 6 possible false positives for unstable benchmarks that are known to sometimes produce them.

@CurtHagenlocher CurtHagenlocher deleted the Map branch October 16, 2023 16:28
JerAguilon pushed a commit to JerAguilon/arrow that referenced this pull request Oct 23, 2023
### 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 <[email protected]>
Co-authored-by: Curt Hagenlocher <[email protected]>
Signed-off-by: David Li <[email protected]>
loicalleyne pushed a commit to loicalleyne/arrow that referenced this pull request Nov 13, 2023
### 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 <[email protected]>
Co-authored-by: Curt Hagenlocher <[email protected]>
Signed-off-by: David Li <[email protected]>
abandy added a commit to abandy/arrow that referenced this pull request Dec 4, 2023
abandy added a commit to abandy/arrow that referenced this pull request Dec 5, 2023
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
### 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 <[email protected]>
Co-authored-by: Curt Hagenlocher <[email protected]>
Signed-off-by: David Li <[email protected]>
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.

[C#] Implement MapType
5 participants