Presto: EXPLAIN CREATE TABLE AS SELECT creates a table, should be side-effect free

[findepi]

presto> use memory.default;
USE
presto:default> explain create table x as select 1 c;
                                                                                                                               Query Plan
-------------------------------------------------------------------------------------------------------------------------------------------------------
 - Output[rows] => [rows:bigint]
     - TableCommit[memory:INSTANCE:MemoryOutputTableHandle{table=MemoryTableHandle{connectorId=memory, schemaName=default, tableName=x, tableId=0, columnHandles=...
         - LocalExchange[SINGLE] () => partialrows:bigint, fragment:varbinary
             - RemoteExchange[GATHER] => partialrows:bigint, fragment:varbinary
                 - TableWriter => [partialrows:bigint, fragment:varbinary]
                         c := expr
                     ..................

[...]

presto:default> explain create table x as select 1 c;
Query 20180205_101139_00002_ijv9f failed: line 1:9: Destination table 'memory.default.x' already exists

elect * from x;
 c
---
(0 rows)

EXPLAIN renders query plan without actually running the query, so EXPLAIN CREATE TABLE ... AS SELECT ... should do just that.
Currently, it creates the table (but does't populate it).

[ebyhr]

It seems the behavior isn't common among all connectors. I think Memory connector specific. At least, blackhole and raptor don't create the table.

Probably, around following lines should be in finishCreateTable (not beginCreateTable)?
https://github.com/prestodb/presto/blob/24082b130eb43f61573cefaf8e7bbadcf6403f83/presto-memory/src/main/java/com/facebook/presto/plugin/memory/MemoryMetadata.java#L199
https://github.com/prestodb/presto/blob/24082b130eb43f61573cefaf8e7bbadcf6403f83/presto-memory/src/main/java/com/facebook/presto/plugin/memory/MemoryMetadata.java#L204

presto:default> use raptor.default;
USE
presto:default> explain create table x as select 1 c;
                                                           Query Plan                                    
---------------------------------------------------------------------------------------------------------
 - Output[rows] => [rows:bigint]                                                                         
 ...
presto:default> select * from x;
Query 20180205_131223_00080_nkub8 failed: line 1:15: Table raptor.default.x does not exist
select * from x

presto:default> use blackhole.default;
USE
presto:default> explain create table x as select 1 c;
                                                           Query Plan                                    
---------------------------------------------------------------------------------------------------------
 - Output[rows] => [rows:bigint]                                                                         
...
presto:default> 
presto:default> select * from x;
Query 20180205_131541_00085_nkub8 failed: line 1:15: Table blackhole.default.x does not exist
select * from x

[findepi]

@ebyhr it's likely you're right! Would you like to fix the memory connector?
Also, please add ATQ test to ensure no other connector falls into this trap in the future.

[ebyhr]

Yes, I'd like that. And understood the test case.

[martint]

The underlying problem is that the planner is not side effect-free. Thos methods should not be called during planning. It works that way for historical reasons.

See if you can add the tests to the connector smoke tests instead. AbstractTestQueries is getting too big and it’s original intent was to test engine behavior, not connectors.

[findepi]

@martint you mean AbstractTestIntegrationSmokeTest, right? Sorry, i forgot about that class.

[findepi]

The underlying problem is that the planner is not side effect-free. Thos methods should not be called during planning. It works that way for historical reasons.

True. I think we should fix memory connector now and only then plan to fix the planner. Especially that I don't know how the planner should actually be fixed.

[ebyhr]

I've investigated but it is difficult to fix some connectors in SPI level.

Easy to fix: memory, mongodb
Difficult to fix: accumulo, cassandra, jdbc, postgres, mysql
No effect: hive, raptor
Not support CTAS: kafka, redis, thrift

eg) Cassandra connector's code has following comment. So, if I move cassandraSession.execute(queryBuilder.toString()); to finishCreateTable, CassandraPageSink .appendPage throws exception.

https://github.com/prestodb/presto/blob/master/presto-cassandra/src/main/java/com/facebook/presto/cassandra/CassandraMetadata.java#L303

[electrum]

Fixing this properly will require SPI changes. The problem is that we start doing work (execution) during planning, but it needs to be two separate calls.