Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[docs] Restructure and expand presto_cpp docs #22717

Merged
merged 1 commit into from
May 16, 2024

Conversation

steveburnett
Copy link
Contributor

@steveburnett steveburnett commented May 10, 2024

Description

  • Moved Prestissimo Developer Guide documentation files to "presto_cpp" directory, including updating index files
  • Revised text references from "Prestissimo" to "Presto C++" or "C++ based Presto"
  • Added new content for Motivation, Vision, Supported Use Cases, and significant expansion of existing Limitations content (draft content provided by @tdcmeehan, thanks!)

Motivation and Context

Updates and expands C++ based Presto documentation.

Impact

Documentation.

Test Plan

Test in local build before opening PR, then CI.

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* ... Improve C++ based Presto documentation :pr:`22717`

presto-docs/src/main/sphinx/presto-cpp.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/presto-cpp.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/presto-cpp.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/presto-cpp.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/presto-cpp.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/presto_cpp/features.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/presto_cpp/features.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/presto_cpp/limitations.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/presto_cpp/limitations.rst Outdated Show resolved Hide resolved
@steveburnett
Copy link
Contributor Author

All comments addressed, commits squashed, ready!

elharo
elharo previously approved these changes May 14, 2024
Copy link
Collaborator

@majetideepak majetideepak left a comment

Choose a reason for hiding this comment

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

@steveburnett looks great. One comment.

presto-docs/src/main/sphinx/presto-cpp.rst Show resolved Hide resolved
presto-docs/src/main/sphinx/presto_cpp/limitations.rst Outdated Show resolved Hide resolved
@steveburnett
Copy link
Contributor Author

Thanks! Review comments addressed, commits squashed, CI running.

@steveburnett steveburnett merged commit fccc180 into prestodb:master May 16, 2024
57 checks passed
@steveburnett steveburnett deleted the steveburnett-presto_cpp branch May 16, 2024 16:10

* Not all built-in functions are implemented in C++. Attempting to use unimplemented functions results in a query failure. For supported functions, see `Function Coverage <https://facebookincubator.github.io/velox/functions/presto/coverage.html>`_.

* Not all built-in types are implemented in C++. Attempting to use unimplemented types will result in a query failure.
Copy link
Contributor

Choose a reason for hiding this comment

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

@steveburnett : Apologize for the delay. I feel we should add more detail here about the supported and unsupported types here.

The specifics are :
All basic/structured types (from https://prestodb.io/docs/0.286/language/types.html) but [CHAR, TIME, TIME WITH TIMEZONE] are supported. These are subsumed by VARCHAR, TIMESTAMP and TIMESTAMP WITH TIMEZONE.

Caveat : Prestissimo only supports unlimited length Varchar. It does not honor the lengths in varchar[n].

Types IPADDRESS, IPPREFIX, UUID, kHYPERLOGLOG, P4HYPERLOGLOG, QDIGEST, TDIGEST are not supported

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened #22772 to add this content to the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants