Support metadata DELETE in JDBC connectors #6287

kokosing · 2020-12-10T14:54:06Z

Support metadata DELETE in JDBC connectors

Fixes #5275

kokosing · 2020-12-10T14:54:53Z

Fixes #5275

findepi · 2020-12-11T12:30:35Z

How does this interact with predicate pushdown?

kokosing · 2020-12-11T12:36:50Z

How does this interact with predicate pushdown?

It is based on predicate pushdown. If predicate is fully pushed down, then we can call DELETE. Otherwise it fails to run DELETE. It is called metadata delete.

ssheikin · 2020-12-14T15:16:55Z

presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/QueryBuilder.java

@@ -102,7 +102,7 @@ public PreparedStatement buildSql(

        List<TypeAndValue> accumulator = new ArrayList<>();

-        List<String> clauses = toConjuncts(client, session, connection, tupleDomain, accumulator);
+        List<String> clauses = toConjuncts(session, connection, tupleDomain, accumulator);
        if (additionalPredicate.isPresent()) {


What is additionalPredicate? Is it somehow connected with limit? If not, why don't you need it in delete?

It is not. Connector when creates a splits then it can use additionalPredicate so it can run JDBC queries in parallel. One split where key_column % 2 = 0 and other when key_column % 2 = 1 where you have 2 splits.

In this case you cannot delete something which does not exist (or can because it could somehow exist?), and it safe not to provide additionalPredicate for delete. However consider situation if you can not match indexes in underlying systems which had been created with such additionalPredicate conditions.

This code is not executed for delete. buildSql is called only for SELECT. DELETE is using buildDeleteSql there is no additionalPredicate

Yeah, I meant exactly that place (I started this discussion here because there is no additionalPredicate in code you added for DELETE).

Let me please clarify my comment.
Imagine situation when you have some data (key_column, year, name) in table A.
And there are 2 splits: key_column % 2 = 0 and other when key_column % 2 = 1.
Usually user executes selects on A: select name from A where year = <year> which translates to two with splits:

select name from A where key_column % 2 = 0 and year = <year> select name from A where key_column % 2 = 1 and year = <year>

To optimize these queries downstream system has CREATE INDEX a_idx ON a (key_column, year)
If you don't propagate additionalPredicate to DELETE this could end up with a_idx is not used for DELETE.
Could you please check if such scenario is covered?

If you don't propagate additionalPredicate to DELETE this could end up with a_idx is not used for DELETE.

I don't care about it. It does not have to be well performant.

Notice that for this metadata delete there are no splits generated, hence there is no additional predicate nor even actual query execution in Presto. It all happens during the "planning".

additionalPredicate is not about performance, but about correctness.
anyway, this is split's attribute, and no splits are involved at this point yet, afaict

presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/JdbcMetadata.java

ssheikin · 2020-12-14T15:30:26Z

presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/JdbcMetadata.java

+    @Override
+    public Optional<ConnectorTableHandle> applyDelete(ConnectorSession session, ConnectorTableHandle handle)
+    {
+        return Optional.of(handle);


Can it be not null when beginDelete throws an exception?

beginDelete is not called if this can be done by metadata delete (applyDelete)

ssheikin · 2020-12-14T15:37:51Z

...ostgresql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlDistributedQueries.java

@@ -85,5 +82,15 @@ protected TestTable createTableWithDefaultColumns()
                        "col_required2 BIGINT NOT NULL)");
    }

+    @Test
+    @Override
+    public void testDelete()


if supportsDelete=true why this test never assertQuerySucceeds("DELETE FROM "?
Could you please add such scenario? The same is for other connectors.

Added a comment below, which should answer your question

// PostgreSql connector currently does not support row-by-row delete

Thanks for comment.
So if supportsDelete=true and connector currently does not support row-by-row delete I'd expect that this connector supports some other type of delete. I see you added testDeleteWithSimplePredicate and testDeleteEntireTable. Maybe it worth to rename this method to something like testDeleteRowByRow?

ssheikin · 2020-12-15T13:48:47Z

...o-sqlserver/src/test/java/io/prestosql/plugin/sqlserver/TestSqlServerDistributedQueries.java

+    {
+        String tableName = "test_delete_" + randomTableSuffix();
+        assertUpdate("CREATE TABLE " + tableName + " AS SELECT * FROM orders WITH NO DATA", 0);
+        assertQueryFails("DELETE FROM " + tableName + " WHERE orderkey % 2 = 0", "Not supported delete");


Suggested change

assertQueryFails("DELETE FROM " + tableName + " WHERE orderkey % 2 = 0", "Not supported delete");

// SqlServer connector currently does not support row-by-row delete

assertQueryFails("DELETE FROM " + tableName + " WHERE orderkey % 2 = 0", "Not supported delete");

presto-oracle/src/test/java/io/prestosql/plugin/oracle/BaseTestOracleDistributedQueries.java

ssheikin · 2020-12-15T14:03:45Z

...ostgresql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlDistributedQueries.java

@@ -85,5 +82,15 @@ protected TestTable createTableWithDefaultColumns()
                        "col_required2 BIGINT NOT NULL)");
    }

+    @Test
+    @Override
+    public void testDelete()


// PostgreSql connector currently does not support row-by-row delete

Thanks for comment.
So if supportsDelete=true and connector currently does not support row-by-row delete I'd expect that this connector supports some other type of delete. I see you added testDeleteWithSimplePredicate and testDeleteEntireTable. Maybe it worth to rename this method to something like testDeleteRowByRow?

ssheikin · 2020-12-15T14:47:07Z

presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/QueryBuilder.java

@@ -102,7 +102,7 @@ public PreparedStatement buildSql(

        List<TypeAndValue> accumulator = new ArrayList<>();

-        List<String> clauses = toConjuncts(client, session, connection, tupleDomain, accumulator);
+        List<String> clauses = toConjuncts(session, connection, tupleDomain, accumulator);
        if (additionalPredicate.isPresent()) {


Yeah, I meant exactly that place (I started this discussion here because there is no additionalPredicate in code you added for DELETE).

Let me please clarify my comment.
Imagine situation when you have some data (key_column, year, name) in table A.
And there are 2 splits: key_column % 2 = 0 and other when key_column % 2 = 1.
Usually user executes selects on A: select name from A where year = <year> which translates to two with splits:

select name from A where key_column % 2 = 0 and year = <year> select name from A where key_column % 2 = 1 and year = <year>

To optimize these queries downstream system has CREATE INDEX a_idx ON a (key_column, year)
If you don't propagate additionalPredicate to DELETE this could end up with a_idx is not used for DELETE.
Could you please check if such scenario is covered?

findepi

Looks nice, but there are some parts I do not understand.

presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/BaseJdbcClient.java

findepi · 2020-12-19T21:12:50Z

presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/QueryBuilder.java

+        List<TypeAndValue> accumulator = new ArrayList<>();
+
+        List<String> clauses = toConjuncts(session, connection, tupleDomain, accumulator);
+        if (!clauses.isEmpty()) {
+            sql += " WHERE " + Joiner.on(" AND ").join(clauses);
+        }
+
+        log.debug("Preparing query: %s", sql);
+        PreparedStatement statement = client.getPreparedStatement(connection, sql);
+        applyParameters(session, connection, accumulator, statement);


This looks like common code which could be unified. Thoughts?

I thought that I have already unified this by extracting applyParameters method. Do you have any idea what else could be done here?

presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveDistributedQueries.java

presto-iceberg/src/test/java/io/prestosql/plugin/iceberg/TestIcebergDistributed.java

findepi · 2020-12-19T21:16:17Z

presto-mysql/src/main/java/io/prestosql/plugin/mysql/MySqlClient.java

@@ -457,6 +460,20 @@ public boolean isLimitGuaranteed(ConnectorSession session)
        return true;
    }

+    @Override
+    public OptionalLong delete(ConnectorSession session, ConnectorTableHandle handle)


The only difference is the commit is missing. Why?

presto-testing/src/main/java/io/prestosql/testing/AbstractTestDistributedQueries.java

presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/BaseJdbcClient.java

kokosing

@findepi comments addressed

presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/JdbcMetadata.java

kokosing · 2021-01-18T12:22:05Z

presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/QueryBuilder.java

+        List<TypeAndValue> accumulator = new ArrayList<>();
+
+        List<String> clauses = toConjuncts(session, connection, tupleDomain, accumulator);
+        if (!clauses.isEmpty()) {
+            sql += " WHERE " + Joiner.on(" AND ").join(clauses);
+        }
+
+        log.debug("Preparing query: %s", sql);
+        PreparedStatement statement = client.getPreparedStatement(connection, sql);
+        applyParameters(session, connection, accumulator, statement);


I thought that I have already unified this by extracting applyParameters method. Do you have any idea what else could be done here?

presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveDistributedQueries.java

kokosing · 2021-01-18T12:35:34Z

presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/JdbcMetadata.java

+    @Override
+    public Optional<ConnectorTableHandle> applyDelete(ConnectorSession session, ConnectorTableHandle handle)
+    {
+        return Optional.of(handle);


beginDelete is not called if this can be done by metadata delete (applyDelete)

kokosing · 2021-01-18T12:44:32Z

presto-mysql/src/test/java/io/prestosql/plugin/mysql/TestMySqlDistributedQueries.java

+    @Override
+    public void testDelete()
+    {
+        // Mysql connector currently does not support row-by-row delete


I added io.trino.testing.AbstractTestDistributedQueries#testDeleteWithSimplePredicate that is executed for mysql and covers very simple predicate pushdown.

Notice that I added here predicate with random. I don't want to hardcode what is supported and what extend. Delete is supported if predicate pushdown is supported. Predicate pushdown support is changing over time, so such test would need to be updated frequently.

kokosing · 2021-01-18T12:52:54Z

presto-oracle/src/test/java/io/prestosql/plugin/oracle/BaseTestOracleDistributedQueries.java

@@ -328,5 +322,15 @@ public void testInsertWithCoercion()
        return Optional.empty();
    }

+    @Test
+    @Override
+    public void testDelete()


It will have to be overriden for all JDBC connectors, test testDelete requires delete row by row which JDBC connector won't have for a while.

Maybe I could add supportsRowByRowDelete()? (I don't like it)

kokosing · 2021-01-18T12:55:12Z

presto-oracle/src/main/java/io/prestosql/plugin/oracle/OracleClient.java

@@ -237,6 +240,20 @@ public void createSchema(ConnectorSession session, String schemaName)
        throw new PrestoException(NOT_SUPPORTED, "This connector does not support creating schemas");
    }

+    @Override
+    public OptionalLong delete(ConnectorSession session, ConnectorTableHandle handle)


It was falling that one cannot call commit when autocommit=true. Now I set autocommit explicitly so I can remove explicit commit.

kokosing · 2021-01-18T13:10:45Z

...ostgresql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlDistributedQueries.java

+    @Override
+    public void testDelete()
+    {
+        // PostgreSql connector currently does not support row-by-row delete


tuple domain is not used to delete row-by-row, by rather to remove by predicate. By row-by-row I mean to delete query execution in Trino where we collect ids of rows to be deleted and then we remove them (beginDelete and finishDelete methods).

findepi · 2021-01-20T13:09:46Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

+        checkArgument(tableHandle.getGroupingSets().isEmpty() && tableHandle.getLimit().isEmpty(), "Unable to delete from synthetic table: %s", tableHandle);
+        try (Connection connection = connectionFactory.openConnection(session);
+                PreparedStatement preparedStatement = queryBuilder.buildDeleteSql(session, connection, tableHandle.getRemoteTableName(), tableHandle.getConstraint())) {
+            connection.setAutoCommit(true);


isn't it default for JDBC?

It looks it is not.

I think we have to separate openConnection to two methods: openReadOnlyConnection(readonly=true, autocommit=false) and openConnectionForWrite(readonly=false, autocommit=true). But not in this PR.

isn't it default for JDBC?

It looks it is not.

it actually is, but Phoenix and ClickHouse have it wrong. Addressed in #7875

plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveDistributedQueries.java

findepi · 2021-01-20T13:12:03Z

plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveDistributedQueries.java

+    @Override
+    public void testDeleteWithBigintEquality()
+    {
+        // Hive connector supports row-by-row delete only for ACID tables or when entire partition is deleted


Can this test set up an ACID table?
i guess file metastore is not supported for ACID, right? cc @djsstarburst

Sadly, that is correct @findepi

...no-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlDistributedQueries.java

...trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerDistributedQueries.java

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestMySqlDistributedQueries.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcMetadata.java

djsstarburst · 2021-01-20T14:38:39Z

There are two APIs for deletion in ConnectorMetadata:

applyDelete() and executeDelete(). In connectors like the Hive and Iceberg, this pair is used for "metadata delete", which has been interpreted as deletion of entire partitions. The dogma is that applyDelete() should only return a non-empty result if the WHERE clause matches entire partitions.
beginDelete() and finishDelete(). This is the API intended for row-by-row delete. As of today, it's only supported by the Hive and Raptor connectors.

Don't blame me for the goofiness of the two APIs - - both pairs existed before I touched the code :)

AFAICT what you've implemented is row-by-row deletion. So perhaps you should use the beginDelete()/finishDelete() APIs.

findepi · 2021-01-20T16:19:32Z

AFAICT what you've implemented is row-by-row deletion. So perhaps you should use the beginDelete()/finishDelete() APIs.

in hive the distinction is needed because dropping partitions is very different operation than "deleting rows from files".

the distinction is not as sound in JDBC worlds. Conceptually, you could consider every row to be its own partition, since there is "an API call" which you can remove the data, without reading it.
(the API call would be issuing DELETE to the remote system)

kokosing · 2021-01-21T19:36:53Z

@djsstarburst Thanks for taking a look!

applyDelete() and executeDelete(). In connectors like the Hive and Iceberg, this pair is used for "metadata delete", which has been interpreted as deletion of entire partitions.

I implemented here "metadata delete".

The dogma is that applyDelete() should only return a non-empty result if the WHERE clause matches entire partitions.

That is my case here. Notice that there is no concept of partitions in JDBC connectors. Here applyDelete() should only return a non-empty result if the entire predicate is pushed down. PushDeleteIntoConnector requires that there is nothing between table scan and delete.

djsstarburst · 2021-01-21T19:57:18Z

in hive the distinction is needed because dropping partitions is very different operation than "deleting rows from files".

That is my case here. Notice that there is no concept of partitions in JDBC connectors.

I see your point, @findepi and @kokosing. Objection withdrawn.

kokosing · 2021-01-26T12:01:56Z

Automation hit #6723

kokosing · 2021-05-10T10:09:11Z

@ksobolew can you please take a look?

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java

plugin/trino-druid/pom.xml

plugin/trino-druid/src/test/java/io/trino/plugin/druid/BaseDruidConnectorTest.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

plugin/trino-druid/src/main/java/io/trino/plugin/druid/DruidJdbcClient.java

testing/trino-testing/src/main/java/io/trino/testing/TestingConnectorBehavior.java

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

findepi · 2021-05-10T11:23:11Z

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorSmokeTest.java

    {
-        if (!hasBehavior(SUPPORTS_DELETE)) {
-            assertQueryFails("DELETE FROM region", "This connector does not support deletes");


Please bring this back. This check was verifying whether ! hasBehavior(SUPPORTS_DELETE) is declared correctly.

findepi · 2021-05-10T11:24:43Z

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorSmokeTest.java

@@ -165,26 +164,24 @@ public void testInsert()
    }

    @Test
-    public void testDelete()
+    public void testRowLevelDelete()


i'd suggest reverting this change.
hasBehavior should allow us to drive the reusable test without need to split it up.
the need to split up the test indicates that testing behaviors are not defined/plugged ideally yet

ksobolew

I don't think I have much to add to what others said. Looks good in general

kokosing

@findepi AC

I removed the TestingConnectorBehavior.SUPPORTS_METADATA_DELETE. It is not possible currently to test if delete was pushed down or not.

kokosing · 2021-05-31T10:52:05Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java

@@ -497,6 +497,7 @@ CacheStats getStatisticsCacheStats()
        cache.invalidateAll(cacheKeys);
    }

+    @SuppressWarnings("unused")


I cannot, as the other method is using SchemaTableName and this is using JdbcTableHandle.

Using always version with SchemaTableName is suboptimal when data change event does not affect entire table.

I think that onDataChanged is too ambitious. It does not fit well with parameter JdbcTableHandle as it does not invalidate data for entire table like one could expect when data is changed. Unless "data" term also refers to "metadata"? I think the easiest is to rename it is to invalidateTableStatistics and do not pretend it is anything more than that.

WDYT?

kokosing · 2021-05-31T10:55:40Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

+        checkArgument(handle.isNamedRelation(), "Unable to delete from synthetic table: %s", handle);
+        QueryBuilder queryBuilder = new QueryBuilder(this);
+        try (Connection connection = connectionFactory.openConnection(session)) {
+            connection.setAutoCommit(false);


This was written in a way you want. And it was failing for certain connectors. Now I don't remember the exact problem. Will do it suggested way and try to capture differences in connectors.

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorSmokeTest.java

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorSmokeTest.java

core/trino-main/src/test/java/io/trino/sql/query/QueryAssertions.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java

plugin/trino-druid/src/main/java/io/trino/plugin/druid/DruidJdbcClient.java

findepi · 2021-06-24T11:19:48Z

plugin/trino-postgresql/src/main/java/io/trino/plugin/postgresql/PostgreSqlClient.java

+            PreparedQuery preparedQuery = queryBuilder.prepareDelete(session, connection, handle.getRequiredNamedRelation(), handle.getConstraint());
+            try (PreparedStatement preparedStatement = queryBuilder.prepareStatement(session, connection, preparedQuery)) {
+                int affectedRowsCount = preparedStatement.executeUpdate();
+                connection.commit();


redundant, because we did not disable autocommit

since this is the only different between this & super, remove override

I will add a comment, it is required. Otherwise PSQL does not commit the delete.

findepi · 2021-06-24T11:20:37Z

testing/trino-testing/src/main/java/io/trino/testing/AbstractTestDistributedQueries.java

-
-        // delete half the table, then delete the rest
-        assertUpdate("DELETE FROM " + tableName + " WHERE orderkey % 2 = 0", "SELECT count(*) FROM orders WHERE orderkey % 2 = 0");
-        assertQuery("SELECT * FROM " + tableName, "SELECT * FROM orders WHERE orderkey % 2 <> 0");


is it only reorg of test cases?

move to separate commit

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

...in/trino-iceberg/src/test/java/io/trino/plugin/iceberg/AbstractTestIcebergConnectorTest.java

kokosing

@ssheikin @ksobolew @Praveen2112 Can you please take a look again, a lot changed since your last review.

kokosing · 2021-06-25T10:37:38Z

plugin/trino-postgresql/src/main/java/io/trino/plugin/postgresql/PostgreSqlClient.java

+            PreparedQuery preparedQuery = queryBuilder.prepareDelete(session, connection, handle.getRequiredNamedRelation(), handle.getConstraint());
+            try (PreparedStatement preparedStatement = queryBuilder.prepareStatement(session, connection, preparedQuery)) {
+                int affectedRowsCount = preparedStatement.executeUpdate();
+                connection.commit();


I will add a comment, it is required. Otherwise PSQL does not commit the delete.

ssheikin · 2021-06-28T13:56:59Z

...in/trino-iceberg/src/test/java/io/trino/plugin/iceberg/AbstractTestIcebergConnectorTest.java

+    @Override
+    public void testDeleteWithComplexPredicate()
+    {
+        // Deletes are covered with testMetadata*Delete test methods


Outdated comments?
maybe just remove them?

No. It is copied from other not supported test for deletes. Delete in Iceberg is special.

I see only testMetadataDelete (without star) in codebase and in this pr.

plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveConnectorTest.java

ssheikin · 2021-06-28T14:23:31Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java

+                return;
+            }
+
+            assertUpdate("DELETE FROM " + table.getName() + " WHERE col < 'A'", 0);


I know we already discussed it before, but I'm still not convinced to a check where the result is compared with 0.
Up to you.

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java

Some connector might have partial support for DELETE statement.

Praveen2112 · 2021-06-29T08:11:50Z

testing/trino-testing/src/main/java/io/trino/testing/AbstractTestDistributedQueries.java

+        // delete using a scalar and EXISTS subquery
+        assertUpdate("CREATE TABLE " + tableName + " AS SELECT * FROM orders", "SELECT count(*) FROM orders");
+        assertUpdate("DELETE FROM " + tableName + " WHERE orderkey = (SELECT orderkey FROM orders ORDER BY orderkey LIMIT 1)", 1);
+        assertUpdate("DELETE FROM " + tableName + " WHERE orderkey = (SELECT orderkey FROM orders WHERE false)", 0);


Can we add support for correlated sub query ... So that we could ensure it works for corelated joins too.

It is out of the scope of this PR. Surely it won't be supported for jdbc connectors.

ssheikin · 2021-06-29T09:11:21Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java

+    @Override
+    public void testDeleteWithVarcharPredicate()
+    {
+        throw new SkipException("This is implemented by testMetadataDeleteWithVarcharEquality");


Where does testMetadataDeleteWithVarcharEquality live?

It should be testDeleteWithVarcharEqualityPredicate

ssheikin · 2021-06-29T09:15:46Z

...in/trino-iceberg/src/test/java/io/trino/plugin/iceberg/AbstractTestIcebergConnectorTest.java

+    @Override
+    public void testDeleteWithComplexPredicate()
+    {
+        // Deletes are covered with testMetadata*Delete test methods


I see only testMetadataDelete (without star) in codebase and in this pr.

kokosing · 2021-06-29T09:40:11Z

I see only testMetadataDelete (without star) in codebase and in this pr.

Yes, and there is testMetadataDeleteSimple. The star is in wrong place.

lhofhansl · 2021-06-30T18:09:36Z

Tried with Phoenix in real setup. Works perfectly.

kokosing · 2021-07-02T07:28:40Z

CI hit: #8457

hashhar · 2021-07-05T11:33:55Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java

+    public void testDeleteWithBigintEqualityPredicate()
+    {
+        if (!hasBehavior(SUPPORTS_DELETE)) {
+            assertQueryFails("DELETE FROM region WHERE regionkey = 1", "This connector does not support deletes");


@kokosing In case of connectors where the test infra is shared this can lead to test data being deleted.

Or is the assumption that if the hasBehavior is implemented correctly it shouldn't matter since the actual test performs deletions against copies of tables?

I'm submitting a PR to change this to follow the approach in testDeleteWithVarcharEqualityPredicate where an explicit TestTable is created for the negative test case.

@hashhar if you're concerned about that, there are other cases like that in tests, where negative test cases are "dangerous".

(i was not concerned as much as you are ...)

eg

trino/testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

Line 700 in 96be8f2

assertQueryFails("ALTER TABLE nation RENAME TO other_schema.yyyy", "This connector does not support renaming tables across schemas");

cla-bot bot added the cla-signed label Dec 10, 2020

kokosing added the WIP label Dec 10, 2020

kokosing marked this pull request as draft December 10, 2020 14:54

kokosing force-pushed the origin/master/269_jdbc_delete branch from 86c4765 to 44da5b6 Compare December 11, 2020 14:05

kokosing requested review from findepi, ssheikin and Praveen2112 December 11, 2020 14:05

kokosing marked this pull request as ready for review December 11, 2020 14:05

kokosing removed the WIP label Dec 12, 2020

kokosing force-pushed the origin/master/269_jdbc_delete branch from 44da5b6 to e5fa7d1 Compare December 13, 2020 09:30

ssheikin reviewed Dec 14, 2020

View reviewed changes

kokosing force-pushed the origin/master/269_jdbc_delete branch from e5fa7d1 to c63ddb6 Compare December 15, 2020 11:27

ssheikin reviewed Dec 15, 2020

View reviewed changes

kokosing force-pushed the origin/master/269_jdbc_delete branch from c63ddb6 to 8542efc Compare December 19, 2020 09:02

findepi reviewed Dec 19, 2020

View reviewed changes

findepi added the enhancement New feature or request label Dec 19, 2020

findepi reviewed Dec 19, 2020

View reviewed changes

presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/BaseJdbcClient.java Outdated Show resolved Hide resolved

kokosing force-pushed the origin/master/269_jdbc_delete branch from 8542efc to 6a8e391 Compare January 18, 2021 13:25

kokosing commented Jan 18, 2021

View reviewed changes

findepi reviewed Jan 20, 2021

View reviewed changes

djsstarburst reviewed Jan 20, 2021

View reviewed changes

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcMetadata.java Outdated Show resolved Hide resolved

findepi mentioned this pull request Jan 22, 2021

Separate query rendering from statement creation in QueryBuilder #6688

Merged

kokosing requested a review from ksobolew May 10, 2021 10:09

ssheikin approved these changes May 10, 2021

View reviewed changes

findepi requested changes May 10, 2021

View reviewed changes

ksobolew reviewed May 10, 2021

View reviewed changes

kokosing force-pushed the origin/master/269_jdbc_delete branch from 0ffb179 to e4d26b4 Compare May 31, 2021 13:14

kokosing commented May 31, 2021

View reviewed changes

kokosing mentioned this pull request Jun 23, 2021

Minor JDBC Connector tests fixes #8361

Merged

kokosing force-pushed the origin/master/269_jdbc_delete branch 2 times, most recently from 2758d03 to 3409fa0 Compare June 24, 2021 10:58

findepi approved these changes Jun 24, 2021

View reviewed changes

kokosing force-pushed the origin/master/269_jdbc_delete branch from 3409fa0 to 2cd8429 Compare June 25, 2021 11:17

kokosing commented Jun 25, 2021

View reviewed changes

kokosing force-pushed the origin/master/269_jdbc_delete branch 3 times, most recently from a8e8533 to 3583b10 Compare June 27, 2021 17:34

ssheikin approved these changes Jun 28, 2021

View reviewed changes

Extract complex delete tests to separate test methods

4851774

Some connector might have partial support for DELETE statement.

Praveen2112 approved these changes Jun 29, 2021

View reviewed changes

kokosing force-pushed the origin/master/269_jdbc_delete branch from 3583b10 to 4f22747 Compare June 29, 2021 08:46

ssheikin reviewed Jun 29, 2021

View reviewed changes

Support metadata DELETE in JDBC connectors

2d9a113

kokosing force-pushed the origin/master/269_jdbc_delete branch from 4f22747 to 2d9a113 Compare June 29, 2021 09:44

kokosing merged commit 825e6d8 into trinodb:master Jul 2, 2021

kokosing deleted the origin/master/269_jdbc_delete branch July 2, 2021 07:28

kokosing mentioned this pull request Jul 2, 2021

Release notes for 360 #8455

Closed

11 tasks

hashhar reviewed Jul 5, 2021

View reviewed changes

	assertQueryFails("DELETE FROM " + tableName + " WHERE orderkey % 2 = 0", "Not supported delete");
	// SqlServer connector currently does not support row-by-row delete
	assertQueryFails("DELETE FROM " + tableName + " WHERE orderkey % 2 = 0", "Not supported delete");

Support metadata DELETE in JDBC connectors #6287

Support metadata DELETE in JDBC connectors #6287

Conversation

kokosing commented Dec 10, 2020 • edited by findepi Loading

kokosing commented Dec 10, 2020

findepi commented Dec 11, 2020

kokosing commented Dec 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssheikin Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssheikin Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

findepi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokosing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djsstarburst commented Jan 20, 2021

findepi commented Jan 20, 2021

kokosing commented Jan 21, 2021

djsstarburst commented Jan 21, 2021

kokosing commented Jan 26, 2021

kokosing commented May 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksobolew left a comment

Choose a reason for hiding this comment

kokosing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokosing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokosing commented Jun 29, 2021

lhofhansl commented Jun 30, 2021

kokosing commented Jul 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokosing commented Dec 10, 2020 •

edited by findepi

Loading

ssheikin Dec 15, 2020 •

edited

Loading

ssheikin Dec 15, 2020 •

edited

Loading