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.
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. compile, Undef, Undef, state) to generate the "USING xxx" clause,
2. use build_tables(
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.