-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Improve documentation for PostgreSQL predicate pushdown support #14036
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good to go as is and we should merge. If we get more technical details we should update.
@@ -459,6 +459,9 @@ The connector supports pushdown for a number of operations: | |||
Predicate pushdown support | |||
^^^^^^^^^^^^^^^^^^^^^^^^^^ | |||
|
|||
Predicates are pushed down for most types, including ``UUID`` and temporal | |||
types, such as ``DATE`` and ``TIME``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reflects what @findepi mentioned in the slack communication and is good imho. Ideally we have a real list of supported data type or a statement like "for all types, besides xyz."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIME pushdown is not supported.
@sheajamba @mosabua
you should be able to validate soundness of docs with EXPLAIN over example tables.
At this point, the docs should already cover all the necessary steps to do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explicitly documenting which types are and are not supported is error prone and complex since pushdown depends on a lot of other factors as well.
We should instead teach users how to identify whether something was or wasn't pushed down by reading the EXPLAIN plan. It scales much better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure .. we have that to some degree in https://trino.io/docs/current/optimizer/pushdown.html and we should add more in https://trino.io/docs/current/optimizer/pushdown.html#predicate-pushdown
But .. we also need to add info that helps once a user finds that something is not pushed down .. then they have to be able to understand why .. is there some config problem, do have to do that.. or is it just not supported..
Can we get this merged @hashhar ? |
8c407a2
to
acac3b8
Compare
Description
Non-technical explanation
Clarifies which data types are supported for Postgresql predicate pushdown.
Release notes
(x) This is not user-visible and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text: