You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "Monkwire3 (via GitHub)" <gi...@apache.org> on 2024/03/01 05:07:28 UTC
[PR] Change RecordBatch::schema return type from SchemaRef to &SchemaRef [arrow-rs]
Monkwire3 opened a new pull request, #5448:
URL: https://github.com/apache/arrow-rs/pull/5448
# Which issue does this PR close?
This addresses the first part of #5342.
# Rationale for this change
Prior to this change, `RecordBatch::schema` returned a `SchemaRef`, which in most cases can only be used as an owned value. By instead returning `&SchemaRef`, users will have more flexibility in the way they use `RecordBatch::schema`; it's return value can be used as a borrowed value or cloned into an owned value.
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
`RecordBatch::schema` returns `&SchemaRef` instead of `SchemaRef`. I made minor changes to parts of the code where `RecordBatch::schema` is called.
# Are there any user-facing changes?
Yes, this will affect the way that users use `RecordBatch::schema`.
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
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
Re: [PR] Change RecordBatch::schema return type from SchemaRef to &SchemaRef [arrow-rs]
Posted by "Monkwire3 (via GitHub)" <gi...@apache.org>.
Monkwire3 commented on PR #5448:
URL: https://github.com/apache/arrow-rs/pull/5448#issuecomment-1972521184
I believe this is a breaking change, but I'm not sure how to add the appropriate label.
--
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
Re: [PR] Change RecordBatch::schema return type from SchemaRef to &SchemaRef [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold closed pull request #5448: Change RecordBatch::schema return type from SchemaRef to &SchemaRef
URL: https://github.com/apache/arrow-rs/pull/5448
--
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
Re: [PR] Change RecordBatch::schema return type from SchemaRef to &SchemaRef [arrow-rs]
Posted by "monkwire (via GitHub)" <gi...@apache.org>.
monkwire commented on code in PR #5448:
URL: https://github.com/apache/arrow-rs/pull/5448#discussion_r1510143044
##########
arrow-flight/tests/flight_sql_client_cli.rs:
##########
@@ -245,7 +245,7 @@ impl FlightSqlService for FlightSqlServiceImpl {
"part_2" => batch.slice(2, 1),
ticket => panic!("Invalid ticket: {ticket:?}"),
};
- let schema = batch.schema();
+ let schema = batch.schema().clone();
let batches = vec![batch];
let flight_data = batches_to_flight_data(schema.as_ref(), batches)
Review Comment:
I tried this change on my machine, but I got a borrow checker error. How does this look instead?
```suggestion
let schema = batch.schema();
let batches = vec![batch.clone()];
let flight_data = batches_to_flight_data(schema, batches)
```
--
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
Re: [PR] Change RecordBatch::schema return type from SchemaRef to &SchemaRef [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5448:
URL: https://github.com/apache/arrow-rs/pull/5448#issuecomment-1975847659
https://github.com/apache/arrow-rs/issues/5463 for the larger discussion
--
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
Re: [PR] Change RecordBatch::schema return type from SchemaRef to &SchemaRef [arrow-rs]
Posted by "monkwire (via GitHub)" <gi...@apache.org>.
monkwire commented on code in PR #5448:
URL: https://github.com/apache/arrow-rs/pull/5448#discussion_r1510154828
##########
arrow-flight/examples/flight_sql_server.rs:
##########
@@ -643,7 +643,8 @@ impl FlightSqlService for FlightSqlServiceImpl {
self.check_token(&request)?;
let schema = Self::fake_result()
.map_err(|e| status!("Error getting result schema", e))?
- .schema();
+ .schema()
+ .clone();
let message = SchemaAsIpc::new(&schema, &IpcWriteOptions::default())
Review Comment:
Thanks, I was able to remove this `.clone()` by saving the `RecordBatch` to a non-temporary value.
--
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
Re: [PR] Change RecordBatch::schema return type from SchemaRef to &SchemaRef [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5448:
URL: https://github.com/apache/arrow-rs/pull/5448#issuecomment-1975837413
> Perhaps we could add a `schema_ref` method in that case, and keep the `schema` method unchanged?
This would make sense to me.
--
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
Re: [PR] Change RecordBatch::schema return type from SchemaRef to &SchemaRef [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5448:
URL: https://github.com/apache/arrow-rs/pull/5448#issuecomment-1975832072
Perhaps we could add a `schema_ref` method in that case, and keep the `schema` method unchanged?
--
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
Re: [PR] Change RecordBatch::schema return type from SchemaRef to &SchemaRef [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5448:
URL: https://github.com/apache/arrow-rs/pull/5448#discussion_r1510080526
##########
arrow-flight/examples/flight_sql_server.rs:
##########
@@ -643,7 +643,8 @@ impl FlightSqlService for FlightSqlServiceImpl {
self.check_token(&request)?;
let schema = Self::fake_result()
.map_err(|e| status!("Error getting result schema", e))?
- .schema();
+ .schema()
+ .clone();
let message = SchemaAsIpc::new(&schema, &IpcWriteOptions::default())
Review Comment:
I think we could avoid this clone
##########
arrow-flight/tests/flight_sql_client_cli.rs:
##########
@@ -245,7 +245,7 @@ impl FlightSqlService for FlightSqlServiceImpl {
"part_2" => batch.slice(2, 1),
ticket => panic!("Invalid ticket: {ticket:?}"),
};
- let schema = batch.schema();
+ let schema = batch.schema().clone();
let batches = vec![batch];
let flight_data = batches_to_flight_data(schema.as_ref(), batches)
Review Comment:
```suggestion
let schema = batch.schema();
let batches = vec![batch];
let flight_data = batches_to_flight_data(schema.as_ref(), batches)
```
--
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
Re: [PR] Change RecordBatch::schema return type from SchemaRef to &SchemaRef [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5448:
URL: https://github.com/apache/arrow-rs/pull/5448#issuecomment-1980003153
Closing in favour of #5474
--
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