Skip to content

Conversation

@lidavidm
Copy link
Member

This improves the protocol docs and adds C++ API documentation. Java already has Javadocs so this PR doesn't touch anything there.

@github-actions
Copy link

@lidavidm
Copy link
Member Author

Rendered (new protocol docs section)

image

The API docs are a little too large to render.

@lidavidm
Copy link
Member Author

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

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.

This is really nice, thank you!

Copy link
Member

Choose a reason for hiding this comment

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

Spell out "POD" in full (or use an equivalent term)?

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps you want to define a doxygen group encompassing these typedefs and enums?

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, how are the corresponding SVGs generated? (do you want to add a comment as to that?)

Copy link
Member

Choose a reason for hiding this comment

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

Is this the case? I though all returned tickets were equivalent and only one of them needed to be fetched, am I misremembering?

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 yes, this should be "for each FlightEndpoint in FlightInfo"

Copy link
Member

Choose a reason for hiding this comment

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

*enum int32 ordinal

Copy link
Member

Choose a reason for hiding this comment

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

Valid union operators

Copy link
Member

Choose a reason for hiding this comment

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

Nit: newline at end of file

@rafael-telles
Copy link
Contributor

Hey @lidavidm !
Commenting here as this may affect this docs: we just noticed that on FlightSql.proto there is some wrong documentation around enum SqlInfo values, saying they should retrieve uint32 values where it should be int64. Like here:

  // Retrieves a uint32 value representing the maximum number of hex characters allowed in an inline binary literal.
  SQL_MAX_BINARY_LITERAL_LENGTH = 541;

uint32 values are not returned by CommandGetSqlInfo, just int64 ones.

Is it better to address this in another PR or in this one?

Thanks!

@lidavidm
Copy link
Member Author

I think if it's just clarifications/fixes, we can address them here.

@lidavidm
Copy link
Member Author

Thank you all for the suggestions, I've cleaned up the reST markup a bit, tweaked the charts, and fixed up the types in the Protobuf and C++ header

@lidavidm
Copy link
Member Author

lidavidm commented Apr 5, 2022

Any final comments? We can open another issue to address anything else we see

@lidavidm lidavidm closed this in dd52b38 Apr 8, 2022
@ursabot
Copy link

ursabot commented Apr 9, 2022

Benchmark runs are scheduled for baseline = b285350 and contender = dd52b38. dd52b38 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.29% ⬆️0.0%] test-mac-arm
[Failed ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.09% ⬆️0.13%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/476| dd52b384 ec2-t3-xlarge-us-east-2>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/461| dd52b384 test-mac-arm>
[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/462| dd52b384 ursa-i9-9960x>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/471| dd52b384 ursa-thinkcentre-m75q>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/475| b2853507 ec2-t3-xlarge-us-east-2>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/460| b2853507 test-mac-arm>
[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/461| b2853507 ursa-i9-9960x>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/470| b2853507 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants