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

Allow aql.transform to receive sql filepath (fix breaking change) #1879

Merged
merged 2 commits into from Mar 31, 2023

Conversation

tatiana
Copy link
Collaborator

@tatiana tatiana commented Mar 31, 2023

Fix aql.transform breaking change (pass a .sql file to the sql argument).

This PR contains:

  1. Test case which illustrates data team's usage of aql.transform, which raises an exception in 1.5.3 (and used to work in 1.3.3)
  2. Bug fix that allows users to continue using the Python SDK as they used with Python SDK 1.3.3.

Astronomer's Data team raised this issue in an internal slack: https://astronomer.slack.com/archives/C02B.8SPT93K/p1680202480707669

The alternative to this solution is to use aql.transform_file. However, since this feature was working before and stopped working between minor releases, we opted to fix it as well.

@tatiana tatiana added product/python-sdk Label describing products and removed product/sql-cli product/uto labels Mar 31, 2023
@tatiana tatiana changed the title Fix maggie bug Allow aql.transform to receive sql filepath (fix breaking change) Mar 31, 2023
@tatiana tatiana force-pushed the fix-maggie-bug branch 2 times, most recently from 6218ba0 to ceec0ab Compare March 31, 2023 00:12
@tatiana tatiana force-pushed the fix-maggie-bug branch 2 times, most recently from b94787e to 86f33dd Compare March 31, 2023 00:19
@tatiana tatiana added the bug Something isn't working label Mar 31, 2023
@tatiana tatiana added this to the sql-cli/0.5.0 milestone Mar 31, 2023
@codecov
Copy link

codecov bot commented Mar 31, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (fe16af2) 86.42% compared to head (a8275ab) 86.42%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1879   +/-   ##
=======================================
  Coverage   86.42%   86.42%           
=======================================
  Files         126      126           
  Lines        6813     6814    +1     
  Branches      672      672           
=======================================
+ Hits         5888     5889    +1     
  Misses        778      778           
  Partials      147      147           
Flag Coverage Δ
PythonSDK 92.30% <100.00%> (+<0.01%) ⬆️
SQL-CLI 97.67% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...thon-sdk/src/astro/sql/operators/base_decorator.py 95.78% <100.00%> (+0.02%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tatiana tatiana marked this pull request as ready for review March 31, 2023 00:58
@utkarsharma2
Copy link
Collaborator

utkarsharma2 commented Mar 31, 2023

@tatiana, Thanks for creating the PR with a very clear test case. I'm sure didn't we have this scenario in our tests leading to this situation.

IMO, we should be backward compatible and should only introduce breaking changes in 2.0. WDYT?

Copy link
Collaborator

@utkarsharma2 utkarsharma2 left a comment

Choose a reason for hiding this comment

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

LGTM!

@tatiana tatiana merged commit 5ebc668 into main Mar 31, 2023
45 checks passed
@tatiana tatiana deleted the fix-maggie-bug branch March 31, 2023 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working product/python-sdk Label describing products
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants