Skip to content

Conversation

@lidavidm
Copy link
Member

@lidavidm lidavidm commented Mar 5, 2022

Adds a Flight SQL documentation page that we can link to from the website.

@github-actions
Copy link

github-actions bot commented Mar 5, 2022

@lidavidm
Copy link
Member Author

lidavidm commented Mar 5, 2022

I would like to add sequence diagrams, how do we feel about something like sphinxcontrib-mermaid?

@lidavidm
Copy link
Member Author

lidavidm commented Mar 5, 2022

CC @jduo if you'd like to take a look

Generally, a database will implement the RPC methods according to the
specification, but does not need to implement a client-side driver. A
database client can use the provided Flight SQL client to interact
with any database that supports the necessary endpoints.
Copy link
Member

Choose a reason for hiding this comment

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

Let's mention that Flight SQL clients decorate existing Flight client classes to add methods corresponding to the new RPC calls.

lidavidm and others added 2 commits March 7, 2022 08:04
@lidavidm
Copy link
Member Author

lidavidm commented Mar 7, 2022

Thanks for the suggestions!

@pitrou
Copy link
Member

pitrou commented Mar 7, 2022

I would like to add sequence diagrams, how do we feel about something like sphinxcontrib-mermaid?

Looks interesting. @jorisvandenbossche What do you think?

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

Nice addition, thank you.

``CommandPreparedStatementQuery``
Execute a previously created prepared statement and get the results.

When used with DoPut: binds parameter values to the prepared statement.
Copy link
Member

Choose a reason for hiding this comment

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

So one must call first DoPut then GetFlightInfo to execute a query with parameters? Wouldn't it be nicer to be able to use DoExchange for that?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe this was considered during the initial discussion but was rejected; @jduo do you remember why?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, it was due to output parameters potentially changing the schema mid-stream. #9368 (comment)

Create a new prepared statement for a SQL query.

``CommandPreparedStatementQuery``
Execute a previously created prepared statement and get the results.
Copy link
Member

Choose a reason for hiding this comment

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

Does this also close the prepared statement or can it be reused multiple times?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is reusable (else, you don't save much from having a prepared statement).

@lidavidm lidavidm changed the title ARROW-15721: [Docs][FlightSQL] Add Flight/Flight SQL to subprojects ARROW-15721: [Docs][FlightRPC] Add Flight/Flight SQL to subprojects Mar 8, 2022
@lidavidm
Copy link
Member Author

lidavidm commented Mar 9, 2022

@jduo @pitrou any other comments here? I think we can use ARROW-14698 to expand the documentation once this lands

All of these commands can be used with the GetFlightInfo and GetSchema
RPC methods. The Protobuf request message should be packed into a
google.protobuf.Any message, then serialized and packed as the ``cmd``
field in a CMD-type FlightDescriptor.
Copy link
Member

Choose a reason for hiding this comment

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

For the record, would there be an easy way to cross-reference the table definitions in the Flight protobuf file (such as FlightDescriptor here)? I guess not, but just asking.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, we would probably need to have Sphinx index the Protobuf file somehow and then use Sphinx's cross-reference support. Unfortunately there doesn't seem to be such integration from a quick glance (and if we wanted to do it, we would need to create a custom Protobuf plugin or modify the existing one (which supports neither Doxygen nor Sphinx): https://github.com/pseudomuto/protoc-gen-doc/

Copy link
Member Author

Choose a reason for hiding this comment

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

That may be worthwhile, though, since then we could avoid repeating docs between both files.

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

+1

@lidavidm lidavidm closed this in 76b1403 Mar 9, 2022
@ursabot
Copy link

ursabot commented Mar 9, 2022

Benchmark runs are scheduled for baseline = 875af62 and contender = 76b1403. 76b1403 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 ⬇️0.08% ⬆️0.04%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.09% ⬆️0.0%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. 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

@lidavidm lidavidm deleted the arrow-15721 branch March 11, 2022 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants