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

Add support for limit pushdown into connectors #421

Merged
merged 5 commits into from Mar 12, 2019

Conversation

3 participants
@martint
Copy link
Member

martint commented Mar 10, 2019

Add support for limit pushdown in the spirit of https://github.com/prestosql/presto/wiki/Pushdown-of-complex-operations. Also, removed support for legacy table layouts from memory connector and implemented limit pushdown in memory connector.

Depends on #420

@cla-bot cla-bot bot added the cla-signed label Mar 10, 2019

@martint martint referenced this pull request Mar 10, 2019

Open

Allow connectors to participate in query optimization #18

4 of 15 tasks complete
Show resolved Hide resolved presto-memory/src/main/java/io/prestosql/plugin/memory/MemoryMetadata.java
}

return metadata.applyLimit(table.getConnectorHandle(), limit)
.map(newTable -> new TableHandle(connectorId, newTable, table.getTransaction(), table.getLayout()));

This comment has been minimized.

@dain

dain Mar 10, 2019

Member

If table.getLayout() is always null here, maybe just pass null, to keep the call count down.

MemoryTableHandle table = (MemoryTableHandle) handle;

if (table.getLimit().isPresent() && table.getLimit().getAsLong() < limit) {
// can't apply the limit if we already have a lower limit

This comment has been minimized.

@dain

dain Mar 10, 2019

Member

I don't think that is right. If we are only going to produce 100 rows, and there is a limit of 200, we are still good.


Optional<TableHandle> newTable = metadata.applyLimit(context.getSession(), tableScan.getTable(), limit.getCount());

return newTable.map(tableHandle -> Result.ofPlanNode(

This comment has been minimized.

@dain

dain Mar 10, 2019

Member

What if I can't completely satisfy the the limit, but I want to update my table handle to reduce overscan?


public class TableId
{
private final String connectorId;

This comment has been minimized.

@dain

dain Mar 10, 2019

Member

Connectors don't really need connectorId anymore. I'd remove this if it isn't too much work.

@prestosql prestosql deleted a comment from martint Mar 10, 2019

@martint martint force-pushed the martint:limit-pushdown branch from cbb1f31 to ccd6466 Mar 10, 2019

@dain

dain approved these changes Mar 10, 2019

Copy link
Member

dain left a comment

Couple of questions and suggestions. If no major changes are needed, looks good to me.


public LimitApplicationResult(T handle, boolean limitGuaranteed)
{
this.handle = handle;

This comment has been minimized.

@dain

dain Mar 10, 2019

Member

add null check

* <p>
* If the connector could benefit from the information but can't guarantee that it will be able to produce
* fewer rows than the provided limit, it should return a non-empty result containing a new handle for the
* derived table and the "limit guaranteed" flag set to false.

This comment has been minimized.

@dain

dain Mar 10, 2019

Member

Do we need to mention that it is important for the connector to only return a new handle instance if something changed?

This comment has been minimized.

@martint

martint Mar 12, 2019

Author Member

It's there, but probably not clear enough. I'll make more emphasis on that.


return metadata.applyLimit(context.getSession(), tableScan.getTable(), limit.getCount())
.map(result -> {
PlanNode node = new TableScanNode(

This comment has been minimized.

@dain

dain Mar 10, 2019

Member

do we need to check if the handle changed with "equals"?

This comment has been minimized.

@martint

martint Mar 12, 2019

Author Member

That's not necessary and probably not desirable. A connector may use handles as pure identifiers and record the limit in some sidecar data structure, so it'd need to return the same handle.

{
MemoryTableHandle table = (MemoryTableHandle) handle;

if (!table.getLimit().isPresent() || limit < table.getLimit().getAsLong()) {

This comment has been minimized.

@dain

dain Mar 10, 2019

Member

you could use table.getLimit().orElse(Long.MAX_VALUE)


import static java.util.Objects.requireNonNull;

public final class MemoryConnectorId
public class ColumnInfo

This comment has been minimized.

@dain

dain Mar 10, 2019

Member

Maybe add toString

@martint martint force-pushed the martint:limit-pushdown branch from ccd6466 to 3671949 Mar 12, 2019

martint added some commits Mar 9, 2019

Clean up memory connector data structures
Introduce TableId to represent the identity of a table
and separate it from the associated metadata, which is
now stored in TableInfo/ColumnInfo.

MemoryTableHandle is now use only in the interface between
engine and connector and is meant to represent the reference
to the entity over which the query will execute. Once we
add support for various types of pushdown, that information
will be encoded in the handle.
Add support for limit pushdown
Adds a new API to ConnectorMetadata for connectors to implement
if they can handle pushing down limit into a table scan:

  applyLimit(ConnectorTableHandle handle, long limit);

@martint martint force-pushed the martint:limit-pushdown branch from 3671949 to e63051b Mar 12, 2019

@martint martint merged commit ebfe216 into prestosql:master Mar 12, 2019

1 check passed

verification/cla-signed
Details
@findepi

This comment has been minimized.

Copy link
Member

findepi commented Mar 13, 2019

🚀

@martint martint referenced this pull request Mar 14, 2019

Closed

Release notes for 306 #410

5 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.