Temporary tables for volume and freshness tests are not properly cleaned on Athena

[bebbo203]

Describe the bug
After a succesful run of volume and source freshness tests, tables in the form:

data_monitoring_metrics_tmp_<timestamp>

are left in Athena.
Moreover, while not present in Athena, is still possible to find files about other temporary tables in the form:

test_<hex_code>_elementary_volume_anomalies_<model_name>_<other_descs>

in the s3 bucket

To Reproduce
Steps to reproduce the behavior:

1. Add an Elementary's volume or freshness test to any model or source in your dbt project while using dbt-athena adapter
2. Run dbt run --select elementary
3. Run dbt test
4. Check the tables in athena and in the bucket s3_data_dir

Expected behavior
Temporary tables must be completely dropped after a run (metadata + parquet files in S3).

Environment (please complete the following information):

• edr Version: 0.14.1
• dbt package Version: 0.14.0
• dbt version: 1.7.13

[haritamar]

Hi @bebbo203 !
Thanks for surfacing. I think you may be right and it's a bug we have for the Athena adapter.
Would you be willing to contribute a fix? I'll be happy to provide guidance.

[bebbo203]

Hi, happy to contribute as soon as I can.
I think the problem is in the dbt-data-reliability error, can we use the issue in this repo or should I open another one in the right repo? (I think I opened the same exact issue a week ago on the other repo)

[bebbo203]

Hello @haritamar,

I've found a possible solution to the problem, but I have some questions about the approach.

1. In the handle_tests_results macro, the temporary table used to update the data_monitoring_metrics table is cleaned up only if elementary.has_temp_table_support is true. However, the has_temp_table_support macro lacks a redefinition for Athena, which does not support temporary tables. Adding the following snippet to the has_temp_table_support.sql file will resolve this issue:

   {% macro athena__has_temp_table_support() %}
       {% do return(false) %}
   {% endmacro %}

2. In handle_tests_results, it's necessary to add an instruction to drop tables from the source (using only adapter.drop_relation() will drop metadata from the Glue Catalog). This can be added as follows in the handle_tests_results.sql file:

   {% if not elementary.has_temp_table_support() %}
       {% do adapter.drop_relation(temp_relation) %}
       {% do adapter.clean_up_table(temp_relation) %}
   {% endif %}

   This solution works well for Athena, but we need to consider that other adapters might not have the clean_up_table method. Do you have any suggestions on how to handle this?

3. Temporary tables for freshness and volume tests are dropped with a query generated in the clean_elementary_test_tables.sql file. However, Athena cannot drop files using a query. A (very inelegant) solution would be:

   {% macro athena__get_clean_elementary_test_tables_queries(test_table_relations) %}
       {% set queries = [] %}
       {% for test_relation in test_table_relations %}
           {% do queries.append("DROP TABLE IF EXISTS {}".format(test_relation.render_pure())) %}
           {% do adapter.clean_up_table(test_relation) %}
       {% endfor %}
       {% do return(queries) %}
   {% endmacro %}

   Adding {% do adapter.clean_up_table(test_relation) %} is not elegant, but it provides a simple fix.

What do you think about this proposed approach? I've tested it, and it seems to work.

Additionally, there's another issue: if a run-time error occurs, the run stops, leaving temporary tables in Athena. Subsequent runs won't see these tables, leaving them in the database and on S3 indefinitely. This could be addressed by adding a script to the on-run-end hook, but this solution is specific to Athena and won't work with other adapters. Should we proceed with the solutions proposed in points 1, 2, and 3 and leave this issue as is?

If you want to take a look at the code, I'm currentyl working on this repo that already contains all the fix that I described above.
I don't want to already submit a PR since I'm not sure about the fix.

I'm sorry if this is not the correct way to discuss the changes that need to be made. This is my first time collaborating on bug resolution. If you could point me in the right direction, I would be more than happy to follow your guidance!

[haritamar]

Hi @bebbo203 !
Thanks a lot for this detailed description of the steps and the research effort. I think actually that discussing in a Github issue is a great way to move forward.
I think btw you can go ahead and submit a PR as a Draft - and we can keep discussing it. Though overall it seems to me like a good approach but I wrote a few comments below regarding the points you mentioned.

Regarding (2) - I believe clean_up_table indeed doesn't exist for other adapters. But what do you think about adding a fully_drop_relation macro (or something like that) that works as follows:

{% macro default__fully_drop_relation(relation) %}
    {% do adapter.drop_relation(relation) %}
{% endmacro %}

{% macro athena__fully_drop_relation(temp_relation) %}
    {% do adapter.drop_relation(temp_relation) %}
    {% do adapter.clean_up_table(temp_relation) %}
{% endmacro %}

Regarding (3) - I think it's better that get_clean_elementary_test_tables_queries won't actually perform operations on the adapter since it's supposed to only get queries.
I realize it's also not the most elegant, but maybe it's better to add a for loop to the end of clean_elementary_test_tables that only runs for Athena, e.g:

{% if target.name == 'athena' %}
    {% for test_relation in test_table_relations %}
        {% do adapter.clean_up_table(test_relation) %}
    {% endfor %}
{% endif %}

(or possibly change clean_elementary_test_tables to have adapter-specific implementations. I'm actually fine with either option)

Regarding the run-time error issue - since users tend to be sensitive about runtimes of the on-run-end hook, maybe I'd actually create a separate macro that users can schedule with run-operation that cleanups leftover tables.
But in any case I think I'd handle that separately and not as a part of the same PR.

[bebbo203]

Hi @haritamar,

I've submitted a draft PR.
Just to keep the thread updated, I've managed the problems in the following way:

1. As you said, I created the fully_drop_relation macro that has an adapter specific implementation and that is called in the function in handle_tests_results.sql

2. I added, before dropping the relation in clean_elementary_test_tables macro, a call to a new adapter-dependant macro called clean_up_tables. The default case is empty while the Athena one has the adapter.clean_up_table() call

[haritamar]

Hi @bebbo203 !
I added one comment in your PR.
Once this is fixed I think we can merge these changes.

[bebbo203]

Yes! Thank you for the hint! I've pushed the change to the branch but it's not appearing in the PR. Should I do anything else?

[haritamar]

PR was merged so closing the issue
Thanks @bebbo203 !