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

Document aggregation pushdown in Oracle connector #11805

Merged
merged 1 commit into from
Apr 7, 2022

Conversation

mosabua
Copy link
Member

@mosabua mosabua commented Apr 5, 2022

Description

Add list of functions that can be pushed down to Oracle with aggregation pushdown.

Is this change a fix, improvement, new feature, refactoring, or other?

Documentation fix.

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Oracle connector.

How would you describe this change to a non-technical end user or system administrator?

Add details to Oracle connector documentation.

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
(x) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.
( ) Release notes entries required with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label Apr 5, 2022
@mosabua mosabua requested review from ebyhr and findepi April 5, 2022 22:50
@mosabua mosabua requested a review from hashhar April 5, 2022 23:18
@github-actions github-actions bot added the docs label Apr 6, 2022
@findepi findepi requested review from wendigo and removed request for findepi April 6, 2022 08:02
@mosabua
Copy link
Member Author

mosabua commented Apr 7, 2022

Ping @wendigo .. the code change is in master .. ideally we get the docs in before the release as well..

Copy link
Member

@hashhar hashhar 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 good for now, but we might want to reconsider how we document pushdown support.

Comment on lines +410 to +411
Pushdown is only supported for ``DOUBLE`` type columns with the
following functions:
Copy link
Member

Choose a reason for hiding this comment

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

Some such type limitations also apply to the functions above as well but they can vary based on type mappings and what the actual data-type is in remote system.

Longer term I would prefer to move docs to a direction where we clearly set the expectation that operations will be pushed down where possible without trying to overspecify those conditions since it's complex, will never get 100% accurate due to the amount of possible combinations and will change without much visibility.

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 sounds good .. the problem is that users test this stuff and then ask specific questions why something is and is not pushed down .. ideally the docs have the answer..

Copy link
Member

@hashhar hashhar Apr 7, 2022

Choose a reason for hiding this comment

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

The answer is if it doesn't work then it's unsupported. We obviously need a better way to communicate that for sure.

For a taste of complexity to know whether sum(x) will be pushed down:

  • data type of x as Trino sees it
  • the underlying data type in remote database
  • any implicit casts that get added by engine (may vary based on query shape)
  • the data type of the grouping columns used
  • whether x is an expression or a plain column reference
  • whether there are other operations that cannot be pushed down (this is unbounded list of things)

As an example of practical real world case where people don't really care about "when" things get pushed down is that sum() since the beginning has only been pushdown-able if the column is a bigint or a decimal but no one ever complained or asked about this. People generally care about performance - not exactly how that performance is achieved.

@hashhar
Copy link
Member

hashhar commented Apr 7, 2022

Merging since it's accurate and the comment needs further discussion and applies to docs as whole instead of just this PR.

@hashhar hashhar merged commit 3692afa into trinodb:master Apr 7, 2022
@mosabua mosabua deleted the oracle-push branch April 7, 2022 18:05
@github-actions github-actions bot added this to the 376 milestone Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants