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

Pass Constraint with functional predicate to ConnectorSplitManager #7608

Closed
findepi opened this issue Apr 16, 2021 · 7 comments · Fixed by #9830
Closed

Pass Constraint with functional predicate to ConnectorSplitManager #7608

findepi opened this issue Apr 16, 2021 · 7 comments · Fixed by #9830
Assignees

Comments

@findepi
Copy link
Member

findepi commented Apr 16, 2021

Constraint#predicate represents a Filter that cannot be converted to a TupleDomain.
Today the Constraint#predicate is passed to io.trino.spi.connector.ConnectorMetadata#applyFilter (but only sometimes, i.e. within AddExchanges)
Connector may be unable to derived all the possible information out of that at this stage.
We should pass the same information second time, to ConnectorSplitManager.

@sopel39
Copy link
Member

sopel39 commented Apr 16, 2021

Should we fix planner and connectors instead so that pushdown of predicate via applyFilter is more reliable?

@findepi
Copy link
Member Author

findepi commented Apr 16, 2021

Constraint#predicate is not always present for performance reasons.

@martint
Copy link
Member

martint commented Apr 16, 2021

Are the performance reasons that it can be expensive to evaluate? Then we should improve that, instead. There’s no reason why it should be as slow as it is today — Trino used to evaluate expressions exclusively by interpreting them on the early days, and it was not horrible.

@findepi
Copy link
Member Author

findepi commented Apr 16, 2021

Are the performance reasons that it can be expensive to evaluate? Then we should improve that

Let's track this as a separate issue

instead.

Improving performance wouldn't alleviate the underlying problem that split manager doesn't have access to full information.
For example, HiveTableHandle keeps list of partitions to scan. (It uses memory, so we have a safety limit for 100K partitions).
Now imagine you want to change hive connector not to track partition list explicitly, or you simply delay this computing this
information until the time of split generation. Today it's not possible, as ConnectorSplitManager gets TupleDomain only,
and not Constraint.

@sopel39
Copy link
Member

sopel39 commented Apr 16, 2021

or you simply delay this computing this
information until the time of split generation

We use selected partitions list to generate table predicates.

We could maybe keep Optional<Predicate<Map<ColumnHandle, NullableValue>>> predicate within HiveTableHandle (since it's used only on coordinator)?

@findepi
Copy link
Member Author

findepi commented Apr 16, 2021

We could maybe keep Optional<Predicate<Map<ColumnHandle, NullableValue>>> predicate within HiveTableHandle (since it's used only on coordinator)?

I wanted to start a separate thread about ... #7620

@losipiuk
Copy link
Member

cc: @losipiuk

@homar homar self-assigned this Nov 3, 2021
homar added a commit to homar/trino that referenced this issue Nov 18, 2021
…le domain

fixes trinodb#9309
fixes trinodb#7608
The idea is to push constraint down to split source and not generate splits
that would be filtered out by this constraint's predicate.
findepi pushed a commit that referenced this issue Nov 22, 2021
…le domain

fixes #9309
fixes #7608
The idea is to push constraint down to split source and not generate splits
that would be filtered out by this constraint's predicate.
@findepi findepi mentioned this issue Nov 22, 2021
12 tasks
sumannewton pushed a commit to sumannewton/trino that referenced this issue Jan 17, 2022
…le domain

fixes trinodb#9309
fixes trinodb#7608
The idea is to push constraint down to split source and not generate splits
that would be filtered out by this constraint's predicate.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

5 participants