Skip to content

Fix filter pushdown for DELETE queries #13705

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.

Sign up for GitHub

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

Merged
mbasmanova merged 1 commit into from
Nov 16, 2019
Merged

Fix filter pushdown for DELETE queries #13705

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 35 additions & 2 deletions presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1793,9 +1793,42 @@ static Predicate<Map<ColumnHandle, NullableValue>> convertToPredicate(TupleDomai
}

@Override
public boolean supportsMetadataDelete(ConnectorSession session, ConnectorTableHandle tableHandle)
public boolean supportsMetadataDelete(ConnectorSession session, ConnectorTableHandle tableHandle, Optional<ConnectorTableLayoutHandle> tableLayoutHandle)
{
return true;
Copy link

@highker highker Nov 15, 2019

Choose a reason for hiding this comment

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

Can we have an enumeration of comments to explain the cases when metadata delete can happen and when it can't? By looking at the code, it seems that it's not an all or nothing call given the filter could be "simple" or very "complicated" but turning to be simple if we do constant folding or even evaluation. Of course, there is a limit we can push. We can only inference the delete if the filter is not "that complicated" .

Copy link
Contributor Author

@mbasmanova mbasmanova Nov 16, 2019

Choose a reason for hiding this comment

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

@highker James, thank you review.

Strictly speaking we should be able to support metadata delete for any filter that doesn't touch non-partition columns. Today we only support it for range filters on partition columns though (regardless of whether filter pushdown is on or off). I'll add a comment and a TODO.

// Allow metadata delete for range filters on partition columns.
// TODO Add support for metadata delete for any filter on partition columns.

if (!tableLayoutHandle.isPresent()) {
Copy link
Contributor

@ajaygeorge ajaygeorge Nov 15, 2019

Choose a reason for hiding this comment

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

do we need a requireNonNull check for the arguments here or are they handled in the caller?

Copy link
Contributor Author

@mbasmanova mbasmanova Nov 15, 2019

Choose a reason for hiding this comment

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

From what I've seen in Presto codebase, requireNonNull is used only in constructors.

Copy link
Contributor

@ajaygeorge ajaygeorge Nov 15, 2019

Choose a reason for hiding this comment

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

sure, the question was more geared towards do we need to null check using either requireNonNull or explicit arg==null .

return true;
}

// Allow metadata delete for range filters on partition columns.
// TODO Add support for metadata delete for any filter on partition columns.

HiveTableLayoutHandle layoutHandle = (HiveTableLayoutHandle) tableLayoutHandle.get();
if (!layoutHandle.isPushdownFilterEnabled()) {
return true;
}

if (layoutHandle.getPredicateColumns().isEmpty()) {
return true;
}

if (!TRUE_CONSTANT.equals(layoutHandle.getRemainingPredicate())) {
return false;
}

TupleDomain<Subfield> domainPredicate = layoutHandle.getDomainPredicate();
if (domainPredicate.isAll()) {
return true;
}

Set<String> predicateColumnNames = domainPredicate.getDomains().get().keySet().stream()
.map(Subfield::getRootName)
.collect(toImmutableSet());

Set<String> partitionColumnNames = layoutHandle.getPartitionColumns().stream()
.map(HiveColumnHandle::getName)
.collect(toImmutableSet());

return partitionColumnNames.containsAll(predicateColumnNames);
}

@Override
Expand Down
9 changes: 9 additions & 0 deletions presto-hive/src/test/java/com/facebook/presto/hive/TestHiveIntegrationSmokeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1677,6 +1677,15 @@ public void testMetadataDelete()

assertQuery("SELECT * from test_metadata_delete", "SELECT orderkey, linenumber, linestatus FROM lineitem WHERE linestatus<>'F' or linenumber<>3");

// TODO This use case can be supported
Copy link
Contributor

@ajaygeorge ajaygeorge Nov 15, 2019

Choose a reason for hiding this comment

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

can you please explain what this //TODO means.
Is this a failing test that needs to be fixed later on or some functionality that needs to be added.

Copy link
Contributor Author

@mbasmanova mbasmanova Nov 15, 2019

Choose a reason for hiding this comment

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

@ajaygeorge This is missing functionality. This query should work fine, but it doesn't. Since this is existing behavior, I'm keeping it like that for pushdown-query code path.

try {
getQueryRunner().execute("DELETE FROM test_metadata_delete WHERE lower(LINE_STATUS)='f' and LINE_NUMBER=CAST(4 AS INTEGER)");
fail("expected exception");
}
catch (RuntimeException e) {
assertEquals(e.getMessage(), "This connector only supports delete where one or more partitions are deleted entirely");
}

assertUpdate("DELETE FROM test_metadata_delete WHERE LINE_STATUS='O'");

assertQuery("SELECT * from test_metadata_delete", "SELECT orderkey, linenumber, linestatus FROM lineitem WHERE linestatus<>'O' and linenumber<>3");
Expand Down
6 changes: 0 additions & 6 deletions presto-hive/src/test/java/com/facebook/presto/hive/TestHivePushdownIntegrationSmokeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,4 @@ public TestHivePushdownIntegrationSmokeTest()
public void testMaterializedPartitioning()
{
}

// TODO Enable this test after we fix the query plan to retain information to indicate that a column filter is pushed down and the "DELETE" operation is not on entire partition.
@Test
public void testMetadataDelete()
{
}
}
2 changes: 1 addition & 1 deletion presto-kudu/src/main/java/com/facebook/presto/kudu/KuduMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ public void finishDelete(ConnectorSession session, ConnectorTableHandle tableHan
}

@Override
public boolean supportsMetadataDelete(ConnectorSession session, ConnectorTableHandle tableHandle)
public boolean supportsMetadataDelete(ConnectorSession session, ConnectorTableHandle tableHandle, Optional<ConnectorTableLayoutHandle> tableLayoutHandle)
{
return false;
}
Expand Down
3 changes: 2 additions & 1 deletion presto-main/src/main/java/com/facebook/presto/metadata/MetadataManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -871,7 +871,8 @@ public boolean supportsMetadataDelete(Session session, TableHandle tableHandle)
ConnectorMetadata metadata = getMetadata(session, connectorId);
return metadata.supportsMetadataDelete(
session.toConnectorSession(connectorId),
tableHandle.getConnectorHandle());
tableHandle.getConnectorHandle(),
tableHandle.getLayout());
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion presto-raptor/src/main/java/com/facebook/presto/raptor/RaptorMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -834,7 +834,7 @@ public void finishDelete(ConnectorSession session, ConnectorTableHandle tableHan
}

@Override
public boolean supportsMetadataDelete(ConnectorSession session, ConnectorTableHandle tableHandle)
public boolean supportsMetadataDelete(ConnectorSession session, ConnectorTableHandle tableHandle, Optional<ConnectorTableLayoutHandle> tableLayoutHandle)
{
return false;
}
Expand Down
2 changes: 1 addition & 1 deletion presto-spi/src/main/java/com/facebook/presto/spi/connector/ConnectorMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -538,7 +538,7 @@ default Map<SchemaTableName, ConnectorViewDefinition> getViews(ConnectorSession
/**
* @return whether delete without table scan is supported
*/
default boolean supportsMetadataDelete(ConnectorSession session, ConnectorTableHandle tableHandle)
default boolean supportsMetadataDelete(ConnectorSession session, ConnectorTableHandle tableHandle, Optional<ConnectorTableLayoutHandle> tableLayoutHandle)
{
throw new PrestoException(NOT_SUPPORTED, "This connector does not support deletes");
}
Expand Down
4 changes: 2 additions & 2 deletions .../java/com/facebook/presto/spi/connector/classloader/ClassLoaderSafeConnectorMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,10 +482,10 @@ public void finishDelete(ConnectorSession session, ConnectorTableHandle tableHan
}

@Override
public boolean supportsMetadataDelete(ConnectorSession session, ConnectorTableHandle tableHandle)
public boolean supportsMetadataDelete(ConnectorSession session, ConnectorTableHandle tableHandle, Optional<ConnectorTableLayoutHandle> tableLayoutHandle)
{
try (ThreadContextClassLoader ignored = new ThreadContextClassLoader(classLoader)) {
return delegate.supportsMetadataDelete(session, tableHandle);
return delegate.supportsMetadataDelete(session, tableHandle, tableLayoutHandle);
}
}

Expand Down