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

Deprecate table layouts #420

Merged
merged 3 commits into from Mar 11, 2019

Conversation

3 participants
@martint
Copy link
Member

martint commented Mar 9, 2019

Connectors that participate in complex operation pushdown will not be able to support table layouts. This change introduces a set of alternate connector APIs that don't rely on layouts for connectors to implement.

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

@martint martint referenced this pull request Mar 9, 2019

Open

Allow connectors to participate in query optimization #18

4 of 15 tasks complete
@dain

dain approved these changes Mar 9, 2019

Copy link
Member

dain left a comment

Looks good

@@ -391,6 +394,9 @@ public boolean catalogExists(Session session, String catalogName)

CatalogMetadata catalogMetadata = getCatalogMetadata(session, connectorId);
ConnectorMetadata metadata = catalogMetadata.getMetadataFor(connectorId);

checkState(metadata.usesLegacyTableLayouts(), "getLayout() was called even though connector doesn't support legacy Table Layout");

This comment has been minimized.

@dain

dain Mar 9, 2019

Member

I'd put this first in the method. If that isn't possible, add a comment on why.

This comment has been minimized.

@martint

martint Mar 9, 2019

Author Member

Well, that checkState depends on "metadata", which needs to be obtained from catalogMetadata, which needs the connectorId, so it's kind of obvious why it can't go at the beginning.

throw new IllegalStateException("getTableProperties() must be implemented if usesLegacyTableLayouts is false");
}

// TODO: ensure single layout

This comment has been minimized.

@dain

dain Mar 9, 2019

Member

maybe do this?

This comment has been minimized.

@martint

martint Mar 9, 2019

Author Member

Ah, yes, forgot to fix that before I submitted the PR

}

/**
* The columns from the original table provided by this layout. A layout may provide only a subset of columns.

This comment has been minimized.

@dain

dain Mar 9, 2019

Member

remove use of "layout" from the descriptions

@martint martint force-pushed the martint:tl-deprecate branch from 01d37a6 to f8f5f05 Mar 9, 2019

@martint martint force-pushed the martint:tl-deprecate branch from f8f5f05 to ab9a4ae Mar 10, 2019

import javax.inject.Inject;

import java.util.Map;

This comment has been minimized.

@sopel39

sopel39 Mar 11, 2019

Member

nit: change comments from // to /**/

@@ -175,7 +176,7 @@ public Plan getLogicalPlan(Session session, Statement statement, List<Expression
PlanNodeIdAllocator idAllocator = new PlanNodeIdAllocator();

// plan statement
LogicalPlanner logicalPlanner = new LogicalPlanner(session, planOptimizers, idAllocator, metadata, sqlParser, statsCalculator, costCalculator, warningCollector);
LogicalPlanner logicalPlanner = new LogicalPlanner(session, planOptimizers, idAllocator, metadata, new TypeAnalyzer(sqlParser, metadata), statsCalculator, costCalculator, warningCollector);

This comment has been minimized.

@sopel39

sopel39 Mar 11, 2019

Member

I think you can inject TypeAnalyer to constructor

This comment has been minimized.

@martint

martint Mar 11, 2019

Author Member

Similar to SqlQueryExecution, this class has all it needs and requires a SqlParser anyway to create an Analyzer, so no need to further pollute the constructor. Also, it'd be confusing to have it take a type analyzer but not an analyzer.

@@ -414,7 +415,7 @@ private PlanRoot doAnalyzeQuery()

// plan query
PlanNodeIdAllocator idAllocator = new PlanNodeIdAllocator();
LogicalPlanner logicalPlanner = new LogicalPlanner(stateMachine.getSession(), planOptimizers, idAllocator, metadata, sqlParser, statsCalculator, costCalculator, stateMachine.getWarningCollector());
LogicalPlanner logicalPlanner = new LogicalPlanner(stateMachine.getSession(), planOptimizers, idAllocator, metadata, new TypeAnalyzer(sqlParser, metadata), statsCalculator, costCalculator, stateMachine.getWarningCollector());

This comment has been minimized.

@sopel39

sopel39 Mar 11, 2019

Member

should it based to SqlQueryExecution constructor?

This comment has been minimized.

@martint

martint Mar 11, 2019

Author Member

I doesn't seem necessary in this case. SqlQueryExecution has all it needs to create it and also needs the parser, so no need to further pollute the constructor.

@@ -380,6 +382,7 @@ public boolean catalogExists(Session session, String catalogName)
}

@Override
@Deprecated

This comment has been minimized.

@sopel39

sopel39 Mar 11, 2019

Member

nit: put deprecated in io.prestosql.metadata.Metadata only?

martint added some commits Mar 6, 2019

Encapsulate expression type analysis in planner
The new class is to facilitate obtaining the type of an expression and its subexpressions
during planning (i.e., when interacting with IR expression) and to remove spurious
dependencies on the SQL parser. It will eventually get removed when we split the AST
from the IR and we encode the type directly into IR expressions.
Deprecate table layouts
Introduce a usesLegacyMetadata() method to ConnectorMetadata for
connectors to indicate if they use the legacy Table Layouts feature.
The method returns true by default during the transition period.

The following methods are deprecated:

    ConnectorMetadata.getTableLayouts()
    ConnectorMetadata.getTableLayout()
    ConnectorMetadata.getInfo(ConnectorTableLayoutHandle)
    ConnectorHandleResolver.getTableLayoutHandleClass()
    ConnectorSplitManager.getSplits(..., ConnectorTableLayoutHandle, ...)

Connectors that do not support Table Layouts need to implement
new methods instead:

    ConnectorMetadata.getTableProperties()
    ConnectorMetadata.getInto(ConnectorTableHandle)
    ConnectorSplitManager.getSplits(..., ConnectorTableHandle, ...)

@martint martint force-pushed the martint:tl-deprecate branch from ab9a4ae to f4ce2a2 Mar 11, 2019

@martint martint closed this Mar 11, 2019

@martint martint deleted the martint:tl-deprecate branch Mar 11, 2019

@martint martint merged commit f4ce2a2 into prestosql:master Mar 11, 2019

1 check passed

verification/cla-signed
Details

@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.