Temporary tables for volume and freshness tests are not properly cleaned on Athena about elementary HOT 8 CLOSED

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.

from elementary.

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)

from elementary.

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!

from elementary.

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.

from elementary.

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

from elementary.

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

from elementary.

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?

from elementary.

PR was merged so closing the issue
Thanks @bebbo203 !

from elementary.