Fix handling of single quotes in RedshiftToS3Operator #32915

ashwaq06 · 2023-07-28T13:02:15Z

This PR resolves an issue with the RedshiftToS3Operator in Apache Airflow, ensuring correct handling of single quotes in data. It includes refactoring, unit tests, and follows project guidelines. Collaboration with the community has been instrumental in making this contribution.

boring-cyborg · 2023-07-28T13:02:20Z

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

Pay attention to the quality of your code (ruff, mypy and type annotations). Our pre-commits will help you with that.
In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
Be sure to read the Airflow Coding style.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack

hussein-awala · 2023-07-28T18:58:36Z

airflow/providers/amazon/aws/transfers/redshift_to_s3.py

@@ -144,7 +144,7 @@ def _build_unload_query(
        self, credentials_block: str, select_query: str, s3_key: str, unload_options: str
    ) -> str:
        return f"""
-                    UNLOAD ('{select_query}')
+                    UNLOAD ($${select_query}$$)


could you check this comment?

Thank you for raising this concern. @vincbeck You are right that using dollar quoting might not be sufficient for queries that already use double quotes. Currently, the suggested change in the code does not account for queries using double quotes, and it may not work as expected in those cases.

To address this issue, one possible solution is to detect if the query contains double quotes and, if it does, avoid using dollar quoting. Instead, the operator can dynamically determine the appropriate quoting method based on the query content.

Additionally, to ensure the robustness of the operator, it is essential to include unit tests that cover various use cases, including queries with single quotes, double quotes, and combinations of both. By adding comprehensive unit tests, we can verify that the RedshiftToS3 Operator works correctly with different types of queries.

By implementing these changes and enhancing the operator with proper handling of both single and double quotes, we can provide a more reliable and versatile solution for users dealing with complex queries.

I am not sure this is something worth going to deep on to work around a very limited amount of edge cases. The only use of double quotes would be around column names that contain illegal character sequences. single quotes are for literals which is really going to cover 99% of usage in my opinion. @vincbeck did you have other edge cases in mind from that comment? One other way to approach this would be to simply regex replace any single quote within the string with double single quotes as given in the Redshift Unload docs and continue to wrap the entire query in single quotes. That would cover the double quote usage edge case.

Yeah this solution can work. Or all the way around, you replace "" with " and keep using $$. This solution MIGHT be better because, as you pointed out, queries with "" are rare and as such the replace will be here "just in case". But I definitely think this is worth handling this use case, otherwise your change will be breaking for any query using already "".

ferruzzi · 2023-07-31T17:29:22Z

tests/providers/amazon/aws/transfers/test_redshift_to_s3.py

+        ]
+
+        # Test a query with single quotes
+        select_query = "SELECT * FROM table WHERE column = 'value with single quotes'"


Do we maybe want to parameterize this test to include testing double quotes and maybe missing both types or quotes, just to ensure nothing is breaking?

cjames23 · 2023-08-04T06:41:29Z

tests/providers/amazon/aws/transfers/test_redshift_to_s3.py

+
+        # Assert that the query is wrapped with double dollar sign quotes
+        expected_unload_query = f"""
+            UNLOAD('SELECT * FROM table WHERE column = $$SELECT * FROM table WHERE column = 'value with single quotes'$$')


is this a typo? This does not seem to match the above select query being passed in.

github-actions · 2023-09-22T00:10:59Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

ferruzzi · 2023-09-22T14:22:30Z

@ashwaq06 Are you still working on this one or have we scared you off?

ashwaq06 · 2023-09-22T14:46:24Z

@ashwaq06 Are you still working on this one or have we scared you off?

Hi @ferruzzi ,

Thank you for your feedback on my pull request. I appreciate your input and am committed to improving the code to meet the project's standards.

I've made changes up to my current knowledge and understanding, but I'm open to learning and making the correct changes. If you could provide more specific guidance or pointers on what needs improvement, I would greatly appreciate it. Your assistance in helping me align with the project's requirements would be invaluable.

Looking forward to your guidance and collaboration.

vincbeck · 2023-10-06T14:45:57Z

Hey @ashwaq06, let's to go through your first PR :) I know it can be hard. Could you resolve first the static failures? If you go to "Files changes" in this PR, you'll see them all. Another (better) option is to run static checks on your side. Reading and following the documentation should help

This test case ensures that the RedshiftToS3Operator correctly handles a custom select_query containing single quotes. It validates that the operator generates a proper UNLOAD query that handles single quotes, thereby confirming its functionality in such scenarios.

ashwaq06 · 2023-10-08T13:52:22Z

@vincbeck I've addressed the static failures as per your suggestion. You can see the changes in the "Files changed" section of this PR.

Could you please take a look at the changes I've made and let me know your thoughts? Your review would be greatly appreciated.

cjames23 · 2023-10-08T16:12:45Z

tests/providers/amazon/aws/transfers/test_redshift_to_s3.py

+
+    @pytest.mark.parametrize("table_as_file_name, expected_s3_key", [[True, "key/table_"], [False, "key"]])
+    @mock.patch("airflow.providers.amazon.aws.hooks.s3.S3Hook.get_connection")
+    @mock.patch("airflow.models.connection.Connection")


Why are you mocking the connection rather than using the airflow.utils.db.merge_conn for your tests? I also think it would be preferable here given what the changes are that tests should use Moto which supports redshift rather than mocking in a way that will not confirm that the redshift engine handles them as expected and does not return any errors.

vincbeck · 2023-10-10T14:49:06Z

@vincbeck I've addressed the static failures as per your suggestion. You can see the changes in the "Files changed" section of this PR.

Could you please take a look at the changes I've made and let me know your thoughts? Your review would be greatly appreciated.

Thanks for doing it! There are still some static checks though. Did you follow/read the documentation. There are some unit tests failures as well. Please look at them at the bottom of the PR. Let me know if you need help

Fix handling of single quotes in RedshiftToS3Operator

c8870b6

ashwaq06 requested review from eladkal and o-nikolas as code owners July 28, 2023 13:02

boring-cyborg bot added area:providers provider:amazon-aws AWS/Amazon - related issues labels Jul 28, 2023

Merge branch 'main' into main

531c0a5

hussein-awala reviewed Jul 28, 2023

View reviewed changes

ashwaq06 requested a review from hussein-awala July 28, 2023 19:16

Merge branch 'apache:main' into main

07e96e8

eladkal requested review from ferruzzi and vincbeck July 29, 2023 13:04

ferruzzi reviewed Jul 31, 2023

View reviewed changes

cjames23 reviewed Aug 4, 2023

View reviewed changes

github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Sep 22, 2023

github-actions bot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Sep 23, 2023

ashwaq06 added 3 commits October 8, 2023 17:09

Merge branch 'apache:main' into main

996c736

Merge branch 'main' into main

750f120

cjames23 reviewed Oct 8, 2023

View reviewed changes

ashwaq06 closed this Nov 17, 2023

vincbeck mentioned this pull request Nov 30, 2023

Fix handling of single quotes in RedshiftToS3Operator #35986

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix handling of single quotes in RedshiftToS3Operator #32915

Fix handling of single quotes in RedshiftToS3Operator #32915

ashwaq06 commented Jul 28, 2023 •

edited by hussein-awala

Loading

boring-cyborg bot commented Jul 28, 2023

hussein-awala Jul 28, 2023

ashwaq06 Jul 28, 2023 •

edited

Loading

cjames23 Aug 4, 2023

vincbeck Aug 7, 2023

ferruzzi Jul 31, 2023

cjames23 Aug 4, 2023

github-actions bot commented Sep 22, 2023

ferruzzi commented Sep 22, 2023

ashwaq06 commented Sep 22, 2023

vincbeck commented Oct 6, 2023 •

edited

Loading

ashwaq06 commented Oct 8, 2023

cjames23 Oct 8, 2023

vincbeck commented Oct 10, 2023

Fix handling of single quotes in RedshiftToS3Operator #32915

Fix handling of single quotes in RedshiftToS3Operator #32915

Conversation

ashwaq06 commented Jul 28, 2023 • edited by hussein-awala Loading

boring-cyborg bot commented Jul 28, 2023

hussein-awala Jul 28, 2023

Choose a reason for hiding this comment

ashwaq06 Jul 28, 2023 • edited Loading

Choose a reason for hiding this comment

cjames23 Aug 4, 2023

Choose a reason for hiding this comment

vincbeck Aug 7, 2023

Choose a reason for hiding this comment

ferruzzi Jul 31, 2023

Choose a reason for hiding this comment

cjames23 Aug 4, 2023

Choose a reason for hiding this comment

github-actions bot commented Sep 22, 2023

ferruzzi commented Sep 22, 2023

ashwaq06 commented Sep 22, 2023

vincbeck commented Oct 6, 2023 • edited Loading

ashwaq06 commented Oct 8, 2023

cjames23 Oct 8, 2023

Choose a reason for hiding this comment

vincbeck commented Oct 10, 2023

ashwaq06 commented Jul 28, 2023 •

edited by hussein-awala

Loading

ashwaq06 Jul 28, 2023 •

edited

Loading

vincbeck commented Oct 6, 2023 •

edited

Loading