GH-34852: [C++][Go][Java][FlightRPC] Add support for ordered data #35178

kou · 2023-04-17T09:28:15Z

Rationale for this change

No ordering is unnecessarily limiting. Systems can and do implement distributed sorts, but they can’t reflect this in Flight RPC.

What changes are included in this PR?

These changes add FlightInfo.ordered.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

This PR includes breaking changes to public APIs.

github-actions · 2023-04-17T09:28:44Z

Closes: [C++][Java][FlightRPC] Add support for ordered data #34852

github-actions · 2023-04-17T09:28:47Z

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

kou · 2023-04-19T02:27:29Z

An integration test is added.
We can review this but we can NOT merge this yet. We need one more implementation and a formal vote before we merge this.

lidavidm

LGTM! Just a few minor comments.

lidavidm · 2023-04-19T07:50:40Z

docs/source/format/Flight.rst

Maybe to be clear, leave in an "Otherwise, there is no ordering defined on endpoints or the data within"?

lidavidm · 2023-04-19T07:51:00Z

format/Flight.proto

Compared to the wording in Flight.rst, it is not as clear what this is trying to say

I've copied some missing descriptions from Flight.rst.

lidavidm · 2023-04-19T07:51:45Z

dev/archery/archery/integration/runner.py

I can put together Java support too.

Wow! Thanks!
Could you push it to this branch when you work on it? Or you can create another branch for it.

Thanks!
Merged.

lidavidm · 2023-04-19T07:54:02Z

cpp/src/arrow/flight/integration_tests/test_integration.cc

Shouldn't we be asserting that this is true here?

Ah, it may be a more straightforward approach.
I'll use the approach.

lidavidm · 2023-04-19T07:55:18Z

cpp/src/arrow/flight/sql/example/sqlite_server.cc

SQLite doesn't tell us whether the result set is sorted or not, so I suppose technically this should always be 'true'? (That said, this server also only ever returns one endpoint so it's moot either way.)

We can use true here because this flight server always returns one endpoint.
But I think that false is better here because false means "unknown order".

A real Flight SQL server will set true here only when it analyzes the given query and detects ORDER BY in a main SELECT.
How about adding a comment something like "TODO: Set ordered to true only when the query has ORDER BY" here?

wgtmac · 2023-04-20T01:51:44Z

cpp/src/arrow/flight/types.h

I am not familiar with flight so maybe a dumb question: does the user know which order it is? Like ascending/descending and what are the sorted columns?

Good question, but no, this is just a semantic flag to indicate that (unlike the current status) there is some ordering on the dataset. It's still up to the application to define what that means.

lidavidm

LGTM! Thanks!

docs/source/format/Flight.rst

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

kou · 2023-04-29T11:46:38Z

@github-actions crossbow submit preview-docs

github-actions · 2023-04-29T11:49:02Z

Revision: 4c2b78c

Submitted crossbow builds: ursacomputing/crossbow @ actions-0d6f29303c

Task	Status
preview-docs

kou · 2023-05-02T03:20:44Z

docs/source/format/Flight.rst

-   consumption; this is up to the application to implement.
+   consumption; this is up to the application to implement. The client
+   can also use ``FlightInfo.ordered``. See the previous item for
+   details of ``FlightInfo.ordered``.


I forgot to update this sentence...
If anyone has a suggestion for wording, please share it.

kou · 2023-05-02T03:21:00Z

@github-actions crossbow submit preview-docs

github-actions · 2023-05-02T03:23:30Z

Revision: 2cc8304

Submitted crossbow builds: ursacomputing/crossbow @ actions-4c9d228232

Task	Status
preview-docs

kou · 2023-05-09T01:02:08Z

The vote thread: https://lists.apache.org/thread/gnxv7cm5l01zgfos7mw52gr2337qofln

The vote result (passed): https://lists.apache.org/thread/v05hxvffc118qtd4z4yqclryb7qslgfv

I'll merge this.

alamb · 2023-05-09T09:37:12Z

Thank you @kou

ursabot · 2023-05-09T18:00:11Z

Benchmark runs are scheduled for baseline = aa8ffbe and contender = b73ddc3. b73ddc3 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️2.82% ⬆️0.23%] test-mac-arm
[Finished ⬇️0.26% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️4.25% ⬆️3.76%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] b73ddc33 ec2-t3-xlarge-us-east-2
[Finished] b73ddc33 test-mac-arm
[Finished] b73ddc33 ursa-i9-9960x
[Finished] b73ddc33 ursa-thinkcentre-m75q
[Finished] aa8ffbe1 ec2-t3-xlarge-us-east-2
[Finished] aa8ffbe1 test-mac-arm
[Finished] aa8ffbe1 ursa-i9-9960x
[Finished] aa8ffbe1 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

ursabot · 2023-05-09T18:02:07Z

['Python', 'R'] benchmarks have high level of regressions.
test-mac-arm

…ta (apache#35178) ### Rationale for this change No ordering is unnecessarily limiting. Systems can and do implement distributed sorts, but they can’t reflect this in Flight RPC. ### What changes are included in this PR? These changes add `FlightInfo.ordered`. ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes. **This PR includes breaking changes to public APIs.** * Closes: apache#34852 * Closes: apache#35085 Lead-authored-by: Sutou Kouhei <kou@clear-code.com> Co-authored-by: David Li <li.davidm96@gmail.com> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

github-actions bot added Component: C++ Component: Documentation Component: FlightRPC awaiting committer review Awaiting committer review labels Apr 17, 2023

kou force-pushed the cpp-flight-ordered branch from 89f28a0 to 45bd367 Compare April 19, 2023 02:24

kou marked this pull request as ready for review April 19, 2023 02:26

kou requested review from assignUser, lidavidm and raulcd as code owners April 19, 2023 02:26

lidavidm approved these changes Apr 19, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Apr 19, 2023

wgtmac reviewed Apr 20, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Apr 20, 2023

kou added 6 commits April 20, 2023 14:34

apacheGH-34852: [C++][FlightRPC] Add support for ordered data

93cd973

Add an integration test

26d7e59

Restore "There is not ordering ..."

4e6b2b9

Add more descriptions

3cb3890

Client requires FlightInfo.ordered=true

11643fd

Add a TODO comment

ead7260

kou force-pushed the cpp-flight-ordered branch from 231a17d to ead7260 Compare April 20, 2023 05:34

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Apr 20, 2023

lidavidm approved these changes Apr 20, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Apr 26, 2023

alamb reviewed Apr 28, 2023

View reviewed changes

docs/source/format/Flight.rst Outdated Show resolved Hide resolved

Improve wording

fa66812

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Apr 29, 2023

Adjust style

4c2b78c

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Apr 29, 2023

Add one more note about FlightInfo.ordered

2cc8304

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 2, 2023

kou commented May 2, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels May 2, 2023

kou merged commit b73ddc3 into apache:main May 9, 2023

kou deleted the cpp-flight-ordered branch May 9, 2023 01:03

lidavidm mentioned this pull request Dec 13, 2023

GH-34865: [C++][Java][Flight RPC] Add Session management messages #34817

Merged

GH-34852: [C++][Go][Java][FlightRPC] Add support for ordered data #35178

GH-34852: [C++][Go][Java][FlightRPC] Add support for ordered data #35178

Uh oh!

Conversation

kou commented Apr 17, 2023 • edited 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 Apr 17, 2023

Uh oh!

github-actions bot commented Apr 17, 2023

Uh oh!

kou commented Apr 19, 2023

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kou commented Apr 29, 2023

Uh oh!

github-actions bot commented Apr 29, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kou commented May 2, 2023

Uh oh!

github-actions bot commented May 2, 2023

Uh oh!

kou commented May 9, 2023

Uh oh!

alamb commented May 9, 2023

Uh oh!

ursabot commented May 9, 2023

Uh oh!

ursabot commented May 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kou commented Apr 17, 2023 •

edited

Loading