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

Pattern parameter in S3ToSnowflakeOperator #24571

Merged
merged 1 commit into from
Jun 22, 2022

Conversation

Taragolis
Copy link
Contributor

closes: #24360

Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

No expertise to judge whether pattern needs special escaping (e.g. what if the pattern contains a '?) but files is also currently not escaped or validated, so perhaps not? And if there’s a bug, we need to fix them all anyway, and that should happen in a separate PR, not this one.

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Jun 21, 2022
@github-actions
Copy link

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@uranusjr
Copy link
Member

Doc build failure is unrelated (see #24575).

@Taragolis
Copy link
Contributor Author

No expertise to judge whether pattern needs special escaping (e.g. what if the pattern contains a '?) but files is also currently not escaped or validated, so perhaps not? And if there’s a bug, we need to fix them all anyway, and that should happen in a separate PR, not this one.

This is real good question. About pattern it should always be escaped by single quote (doc) but It unclear what if pattern itself should contain single quote. I agree that at least it should be mention in documentation/docstring.

Never use option files for Snowflake, seems like it should also escaped, but documentation unclear about this.

Anyway I will try to check on actual Snowflake during the day different cases

@Taragolis
Copy link
Contributor Author

Taragolis commented Jun 21, 2022

files is also currently not escaped or validated,

Just check and found that files actually also quoted however not validated.

files = ", ".join(f"'{key}'" for key in self.s3_keys)

Single quote is a safe to use special character for S3 key/prefixes.
image

That mean if user has ' in S3 key in current implementation most probably operator execution would fail.

@josh-fell
Copy link
Contributor

CC @grzegorzb1990 since the issue was assigned to you.

@Taragolis
Copy link
Contributor Author

Taragolis commented Jun 21, 2022

Test on real Snowflake: in COPY INTO instruction all ' in PATTERN should be replaced by two single quotes as well as in FILES

@Taragolis Taragolis force-pushed the s3-to-snowflake-pattern branch from 5c94926 to 5be9f4b Compare June 21, 2022 16:30
@uranusjr uranusjr merged commit 66e8400 into apache:main Jun 22, 2022
@Taragolis Taragolis deleted the s3-to-snowflake-pattern branch August 6, 2022 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers full tests needed We need to run full set of tests for this PR to merge kind:documentation provider:snowflake Issues related to Snowflake provider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pattern parameter in S3ToSnowflakeOperator
5 participants