GH-37553: [Java] Allow FlightInfo#Schema to be nullable for long-running queries by tdcmeehan · Pull Request #37528 · apache/arrow

tdcmeehan · 2023-09-01T20:25:23Z

With #36155, implementations of Flight RPC may not return quickly via a newly added pollFlightInfo function. Sometimes, the system implementing this function may not know the output schema for some time--for example, after a lengthy queue time as elapsed, or after planning.

In proto3, fields may not be present, and it's a coding convention to require them 1. To support upcoming client integration work for pollFlightInfo, the schema field can be made optional so that it's not a requirement to populate the FlightInfo's schema on the first pollFlightInfo request.

We can modify our client code to allow this field to be optional. This is already the case for the Go code.

This changes the Java client code to allow the Schema to be null. A new getSchemaOptional method returns Optional<Schema>, which is a backwards compatible change. The existing method is deprecated, but will still return an empty schema if the schema is not present on wire (as it used to before).

Rationale for this change

With #36155, implementations of Flight RPC may not return quickly via a newly added pollFlightInfo function. Sometimes, the system implementing this function may not know the output schema for some time--for example, after a lengthy queue time as elapsed, or after planning.

In proto3, fields may not be present, and it's a coding convention to require them 1. To support upcoming client integration work for pollFlightInfo, the schema field can be made optional so that it's not a requirement to populate the FlightInfo's schema on the first pollFlightInfo request.

CC: @lidavidm

What changes are included in this PR?

This changes the Java client code to allow the Schema to be null. getSchema is now deprecated and a new getSchemaOptional returns Optional<Schema>, which is a backwards compatible change.

Are these changes tested?

Existing tests ensure serialization and deserialization continue to work.

Are there any user-facing changes?

The getSchema methods are now deprecated in favor of getSchemaOptional.

github-actions · 2023-09-01T20:25:57Z

⚠️ GitHub issue apache/arrow-java#167 has been automatically assigned in GitHub to PR creator.

kou · 2023-09-01T21:21:12Z

It seems that the current implementation already accepts the empty schema message. And an empty schema is used for the case.

arrow/java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightInfo.java

Lines 115 to 118 in a432068

    
           schema = pbFlightInfo.getSchema().size() > 0 ? 
        
               MessageSerializer.deserializeSchema( 
        
                   new ReadChannel(Channels.newChannel(new ByteBufferBackedInputStream(schemaBuf)))) 
        
               : new Schema(ImmutableList.of());

How about passing an empty instead of accepting null schema when we can't create a schema for long-running queries?
What is the difference of the above explicit empty schema approach and your suggested implicit empty schema approach? Easy to write?

lidavidm · 2023-09-01T21:53:18Z

I think there are two reasons:

Go already allows this, and we should make sure it's exposed and handled consistently across implementations
No schema is more informative than an empty schema (which I doubt would ever be returned by any database, but is still technically a valid schema)

Protobuf doesn't differentiate a null and an absent schema string, but an empty schema should serialize to a non-null string, so we can still tell the difference, too.

tdcmeehan · 2023-09-02T01:04:24Z

My 0.02 cents: as a user of Arrow, it's not obvious that I should represent a missing schema as a new Schema with a 0-length list of fields.

I tried to browse the codebase for conventions I could learn from, and saw PollInfo as a source of inspiration: nullable fields are simply passed in null references, and they're exposed externally as Optional absent. Please let me know if I misjudged this project's conventions!

kou · 2023-09-03T07:34:49Z

Thanks. I understand.

Could you create a new sub-issue of apache/arrow-java#167 (like #36620 and #36643) only for this (update the Java implementation)?
I think that we also need to update our documents to clarify FlightInfo::schema may be null. We can defer the task by creating a sub-issue for this.
(Do we need to update the C++ implementation too?)

java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightInfo.java

lidavidm · 2023-09-04T14:42:45Z

(Do we need to update the C++ implementation too?)

Yes, the C++ implementation has the same (mistaken) assumption.

tdcmeehan · 2023-09-04T22:45:36Z

Thanks. I understand.

Could you create a new sub-issue of GH-37527 (like GH-36620 and GH-36643) only for this (update the Java implementation)? I think that we also need to update our documents to clarify FlightInfo::schema may be null. We can defer the task by creating a sub-issue for this. (Do we need to update the C++ implementation too?)

I created #37554 for the docs update, and #37553 specifically for the Java update, and they are part of a task list in apache/arrow-java#167. I would like some help on the C++ issue. :)

github-actions · 2023-09-04T22:48:09Z

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

lidavidm · 2023-09-05T01:19:22Z

I can likely help for C++, will just take me a while to be able to get around to it

lidavidm

FlightInfo#equals still needs updating

java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightInfo.java

java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightProducer.java

java/vector/src/main/java/org/apache/arrow/vector/ipc/message/MessageSerializer.java

lidavidm

Thanks! Just a couple final nits.

java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightInfo.java

java/vector/src/main/java/org/apache/arrow/vector/ipc/message/MessageSerializer.java

…g-running queries With apache#36155, implementations of Flight RPC may not return quickly via a newly added pollFlightInfo function. Sometimes, the system implementing this function may not know the output schema for some time--for example, after a lengthy queue time has elapsed, or after planning. In proto3, fields may not be present, and it's a coding convention to require them. To support upcoming client integration work for pollFlightInfo, the schema field can be made optional so that it's not a requirement to populate the FlightInfo's schema on the first pollFlightInfo request. We can modify our client code to allow this field to be optional. This is already the case for the Go code. This changes the Java client code to allow the Schema to be null. `getSchema` methods now return `Optional<Schema>`, which is a backwards incompatible change.

lidavidm

Thank you!

lidavidm · 2023-09-07T13:50:19Z

(I tweaked the PR description to reflect the changes made.)

conbench-apache-arrow · 2023-09-08T14:25:13Z

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

There were no benchmark performance regressions. 🎉

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

tdcmeehan · 2023-09-08T14:27:08Z

Thank you for merging and advice @lidavidm and @kou

lidavidm · 2023-09-08T14:35:18Z

No problem. Feel free to reach out with questions about Flight, since I see there's a discussion going on for Presto

…g-running queries (apache#37528) With apache#36155, implementations of Flight RPC may not return quickly via a newly added pollFlightInfo function. Sometimes, the system implementing this function may not know the output schema for some time--for example, after a lengthy queue time as elapsed, or after planning. In proto3, fields may not be present, and it's a coding convention to require them 1. To support upcoming client integration work for pollFlightInfo, the schema field can be made optional so that it's not a requirement to populate the FlightInfo's schema on the first pollFlightInfo request. We can modify our client code to allow this field to be optional. This is already the case for the Go code. This changes the Java client code to allow the Schema to be null. A new `getSchemaOptional` method returns `Optional<Schema>`, which is a backwards compatible change. The existing method is deprecated, but will still return an empty schema if the schema is not present on wire (as it used to before). ### Rationale for this change With apache#36155, implementations of Flight RPC may not return quickly via a newly added pollFlightInfo function. Sometimes, the system implementing this function may not know the output schema for some time--for example, after a lengthy queue time as elapsed, or after planning. In proto3, fields may not be present, and it's a coding convention to require them 1. To support upcoming client integration work for pollFlightInfo, the schema field can be made optional so that it's not a requirement to populate the FlightInfo's schema on the first pollFlightInfo request. CC: `@ lidavidm` ### What changes are included in this PR? This changes the Java client code to allow the Schema to be null. `getSchema` is now deprecated and a new `getSchemaOptional` returns `Optional<Schema>`, which is a backwards compatible change. ### Are these changes tested? Existing tests ensure serialization and deserialization continue to work. ### Are there any user-facing changes? The `getSchema` methods are now deprecated in favor of `getSchemaOptional`. * Closes: apache#37553 Authored-by: Tim Meehan <tim@timdmeehan.com> Signed-off-by: David Li <li.davidm96@gmail.com>

tdcmeehan requested a review from lidavidm as a code owner September 1, 2023 20:25

github-actions bot added Component: Java awaiting review Awaiting review labels Sep 1, 2023

kou changed the title ~~GH-37527: [Java] Allow FlightInfo#Schema to be nullable for long-runn…~~ GH-37527: [Java] Allow FlightInfo#Schema to be nullable for long-running queries Sep 1, 2023

lidavidm reviewed Sep 4, 2023

View reviewed changes

java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightInfo.java Outdated Show resolved Hide resolved

java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightInfo.java Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Sep 4, 2023

tdcmeehan changed the title ~~GH-37527: [Java] Allow FlightInfo#Schema to be nullable for long-running queries~~ GH-37553: [Java] Allow FlightInfo#Schema to be nullable for long-running queries Sep 4, 2023

tdcmeehan force-pushed the schema branch from 941db16 to 3d2dc93 Compare September 5, 2023 00:54

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 5, 2023

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Sep 5, 2023

tdcmeehan force-pushed the schema branch from 3d2dc93 to 3657c92 Compare September 6, 2023 15:31

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 6, 2023

lidavidm requested changes Sep 6, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Sep 6, 2023

tdcmeehan force-pushed the schema branch from 3657c92 to c899b51 Compare September 6, 2023 21:03

github-actions bot removed the awaiting changes Awaiting changes label Sep 6, 2023

github-actions bot added the awaiting change review Awaiting change review label Sep 6, 2023

tdcmeehan force-pushed the schema branch 4 times, most recently from 2cfa4d2 to 567ee6c Compare September 6, 2023 23:51

lidavidm reviewed Sep 7, 2023

View reviewed changes

java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightInfo.java Outdated Show resolved Hide resolved

java/vector/src/main/java/org/apache/arrow/vector/ipc/message/MessageSerializer.java Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Sep 7, 2023

tdcmeehan force-pushed the schema branch from 567ee6c to 818b983 Compare September 7, 2023 13:40

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 7, 2023

lidavidm approved these changes Sep 7, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Sep 7, 2023

lidavidm merged commit 48cc2ab into apache:main Sep 7, 2023

lidavidm removed the awaiting merge Awaiting merge label Sep 7, 2023

tdcmeehan deleted the schema branch September 7, 2023 14:55

Conversation

tdcmeehan commented Sep 1, 2023 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented Sep 1, 2023

Uh oh!

kou commented Sep 1, 2023

Uh oh!

lidavidm commented Sep 1, 2023

Uh oh!

tdcmeehan commented Sep 2, 2023

Uh oh!

kou commented Sep 3, 2023

Uh oh!

Uh oh!

Uh oh!

lidavidm commented Sep 4, 2023

Uh oh!

tdcmeehan commented Sep 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Sep 4, 2023

Uh oh!

lidavidm commented Sep 5, 2023

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

lidavidm commented Sep 7, 2023

Uh oh!

conbench-apache-arrow bot commented Sep 8, 2023

Uh oh!

tdcmeehan commented Sep 8, 2023

Uh oh!

lidavidm commented Sep 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tdcmeehan commented Sep 1, 2023 •

edited by github-actions bot

Loading

tdcmeehan commented Sep 4, 2023 •

edited

Loading