Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix log_query to format SQL statement correctly in AthenaOperator #36962

Merged
merged 3 commits into from
Jan 31, 2024

Conversation

ferruzzi
Copy link
Contributor

Just some whitespace changes to make the Athena query log messages look more readable in the logs.

Before:
image

After:
image

closes: #34189

@boring-cyborg boring-cyborg bot added area:providers area:Scheduler including HA (high availability) scheduler provider:amazon-aws AWS/Amazon - related issues labels Jan 23, 2024
Copy link
Contributor

@syedahsn syedahsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@dirrao dirrao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work. It would be great if you can add the test cases for this change.

@eladkal eladkal changed the title Ferruzzi/athena logs Fix log_query to format SQL statement correctly in AthenaOperator Jan 24, 2024
@Taragolis Taragolis removed the area:Scheduler including HA (high availability) scheduler label Jan 24, 2024
@ferruzzi
Copy link
Contributor Author

@dirrao I didn't add any test cases because it's just reformatting a string. Could you suggest a test case you'd like to see? I thought of two: one with a single-line query and one with a multi-line query which each assert the expected number of newlines in the log message, but that seems overly complicated for what it is... I think it would be something like "capture all logs, for each captured log message, if it contains 'Running Query with params' then assert it has the expected number of line breaks"... It seems maybe excessive but I can add it if you think it's important.

@eladkal
Copy link
Contributor

eladkal commented Jan 28, 2024

Could you suggest a test case you'd like to see?

I think 1 unit test for query_params_to_string that gets a mock of params and verify the expected result of the function is enough.

@ferruzzi
Copy link
Contributor Author

@eladkal and @dirrao - Added two unit tests. Let me know what you think.

Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@eladkal eladkal merged commit fa29ddb into apache:main Jan 31, 2024
55 checks passed
abhishekbhakat pushed a commit to abhishekbhakat/my_airflow that referenced this pull request Mar 5, 2024
…apache#36962)

* Pretty-print Athena query parameter logs

* fix type hint

* unit tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:amazon-aws AWS/Amazon - related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

log_query of AthenaOperator dumps un readable query
7 participants