Deprecate table layouts #420
Conversation
| @@ -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"); | |||
There was a problem hiding this comment.
I'd put this first in the method. If that isn't possible, add a comment on why.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
remove use of "layout" from the descriptions
| import javax.inject.Inject; | ||
|
|
||
| import java.util.Map; | ||
|
|
There was a problem hiding this comment.
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); | |||
There was a problem hiding this comment.
I think you can inject TypeAnalyer to constructor
There was a problem hiding this comment.
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()); | |||
There was a problem hiding this comment.
should it based to SqlQueryExecution constructor?
There was a problem hiding this comment.
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 | |||
There was a problem hiding this comment.
nit: put deprecated in io.prestosql.metadata.Metadata only?
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.
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, ...)
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.