Code review comment for lp:~therve/storm/delete-with-using

Revision history for this message
James Henstridge (jamesh) wrote :

I notice that you don't do a state.push/pop on auto_tables like compile_select() does. I guess it isn't as important since DELETE statements can't be nested (can they?), but it wouldn't hurt.

Also, the use of build_tables() and manually adding tables from state.auto_tables looks fishy. For compile_select() the automatically generated table list comes from build_tables, so I think there is a problem with how it is being used here.

I think the way it should work is:

 1. just run compile() on Delete.table to generate the "FROM xxx" clause.
 2. use build_tables(compile, Undef, Undef, state) to generate the "USING xxx" clause,
    plus some way to ensure Delete.table does not appear in this list.

Now on to the test changes: While we've used the "if cls_name.startswith(...):" pattern to disable tests for some backends, perhaps it would be cleaner to add a "supports_delete_using()" method that the backend test classes can override to say whether the test should run.

Lastly, it would be good to include a test for the case where the backend does not support "DELETE ... USING" to ensure we give a sensible error rather than generating nonsense SQL like we currently do.

review: Needs Fixing

« Back to merge proposal