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-43672: [C#] Schema should be optional on FlightInfo #43673

Merged
merged 2 commits into from
Sep 4, 2024

Conversation

ndglover
Copy link
Contributor

@ndglover ndglover commented Aug 13, 2024

Rationale for this change

Schema is not required on a FlightInfo message and sometimes needs to be lazily evaluated on the server. This PR allows schema to be null on the FlightInfo since it will be picked up later when requests with those tickets are made.

What changes are included in this PR?

Are these changes tested?

Yes, added a test to confirm this behaviour

Are there any user-facing changes?

Copy link

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

@ndglover ndglover changed the title Arrow-43672 Schema should be optional on FlightInfo GH-43672:[C#] Schema should be optional on FlightInfo Aug 13, 2024
Copy link

⚠️ GitHub issue #43672 has been automatically assigned in GitHub to PR creator.

@kou kou changed the title GH-43672:[C#] Schema should be optional on FlightInfo GH-43672: [C#] Schema should be optional on FlightInfo Aug 13, 2024
@ndglover
Copy link
Contributor Author

ndglover commented Aug 15, 2024

@CurtHagenlocher - any chance you could give this a quick review please?

@CurtHagenlocher - gentle nudge for a review please

@ndglover
Copy link
Contributor Author

ndglover commented Sep 4, 2024

Hi all, sorry to @ so many people but I've had this PR outstanding for review for some time. Would anyone be able to give it a look over and merge if good. I'm keen to get a new release with this added and don't want to fork if possible,
@adamreeve
@assignUser
@jonkeane
@kou
@raulcd

Copy link
Member

@raulcd raulcd left a comment

Choose a reason for hiding this comment

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

The change looks good to me but I don't have enough knowledge around the C# code base to know whether other changes would be required. I've triggered the CI jobs.

@raulcd raulcd requested a review from eerhardt September 4, 2024 09:07
@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Sep 4, 2024
@CurtHagenlocher
Copy link
Contributor

I'm sorry; I forgot about this PR :(.

@CurtHagenlocher
Copy link
Contributor

Test failure looks unrelated

@CurtHagenlocher CurtHagenlocher merged commit 6382c0a into apache:main Sep 4, 2024
11 of 12 checks passed
@CurtHagenlocher CurtHagenlocher removed the awaiting changes Awaiting changes label Sep 4, 2024
@github-actions github-actions bot added the awaiting committer review Awaiting committer review label Sep 4, 2024
@ndglover
Copy link
Contributor Author

ndglover commented Sep 4, 2024

Thanks @CurtHagenlocher @raulcd - much appreciated.

What's the process for making this into a release?

@raulcd
Copy link
Member

raulcd commented Sep 4, 2024

This will be part of the 18.0.0. We do quarterly releases and the 18.0.0 release is schedule for next month.

Copy link

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

There were no benchmark performance regressions. 🎉

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

zanmato1984 pushed a commit to zanmato1984/arrow that referenced this pull request Sep 6, 2024
…43673)

### Rationale for this change

Schema is not required on a FlightInfo message and sometimes needs to be lazily evaluated on the server. This PR allows schema to be null on the FlightInfo since it will be picked up later when requests with those tickets are made.

### What changes are included in this PR?

### Are these changes tested?
Yes, added a test to confirm this behaviour

### Are there any user-facing changes?

* GitHub Issue: apache#43672

Authored-by: neilglover <neilglover@gmail.com>
Signed-off-by: Curt Hagenlocher <curt@hagenlocher.org>
khwilson pushed a commit to khwilson/arrow that referenced this pull request Sep 14, 2024
…43673)

### Rationale for this change

Schema is not required on a FlightInfo message and sometimes needs to be lazily evaluated on the server. This PR allows schema to be null on the FlightInfo since it will be picked up later when requests with those tickets are made.

### What changes are included in this PR?

### Are these changes tested?
Yes, added a test to confirm this behaviour

### Are there any user-facing changes?

* GitHub Issue: apache#43672

Authored-by: neilglover <neilglover@gmail.com>
Signed-off-by: Curt Hagenlocher <curt@hagenlocher.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants