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: Incremental where clause generation from triggering TypeError #1688

Merged

Conversation

BuzzCutNorman
Copy link
Contributor

@BuzzCutNorman BuzzCutNorman commented May 9, 2023

This is an attempt to implement the third option listed in possible fixes for the bug described in #1676.

Closes #1676


📚 Documentation preview 📚: https://meltano-sdk--1688.org.readthedocs.build/en/1688/

@codecov
Copy link

codecov bot commented May 9, 2023

Codecov Report

Merging #1688 (43fa5c7) into main (c6a8933) will increase coverage by 0.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1688      +/-   ##
==========================================
+ Coverage   86.12%   86.24%   +0.11%     
==========================================
  Files          59       59              
  Lines        4944     4943       -1     
  Branches      809      809              
==========================================
+ Hits         4258     4263       +5     
+ Misses        485      481       -4     
+ Partials      201      199       -2     
Impacted Files Coverage Δ
singer_sdk/streams/sql.py 89.70% <100.00%> (+2.74%) ⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@BuzzCutNorman
Copy link
Contributor Author

@edgarrmondragon @kgpayne I am in need of your sage wisdom and advice. Codecov is showing query = query.where(replication_key_col >= start_val) is not covered by testing. I was not able to find a previous test that I could change to cover. I might have missed it though. I have two questions:

  1. Was there a previous covering test? If there was a previous test, please point me in its direction.
  2. If this is a new test where would you like it to live. My best guess would be in the test_tap_sqlite.py tests that I think are pulled from tap_test.py.

Thanks 🙏 in advance.

@edgarrmondragon
Copy link
Collaborator

@BuzzCutNorman this is a new test afaict. The code that currently seeds the data for the test_tap_sqlite.py (which makes the most for the new test to live in) is

@pytest.fixture
def _sqlite_sample_db(sqlite_connector):
"""Return a path to a newly constructed sample DB."""
with sqlite_connector._connect() as conn, conn.begin():
for t in range(3):
conn.execute(text(f"DROP TABLE IF EXISTS t{t}"))
conn.execute(
text(f"CREATE TABLE t{t} (c1 int PRIMARY KEY, c2 varchar(10))"),
)
for x in range(100):
conn.execute(
text(f"INSERT INTO t{t} VALUES ({x}, 'x={x}')"), # noqa: S608
)

so you'll probably want to add a column that acts as replication key and a test that uses a custom catalog.

@BuzzCutNorman BuzzCutNorman marked this pull request as ready for review May 24, 2023 20:42
Copy link
Collaborator

@edgarrmondragon edgarrmondragon left a comment

Choose a reason for hiding this comment

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

Thanks for updating the SQLite test @BuzzCutNorman! This LGTM, I'll let @kgpayne give it a look and merge it.

@kgpayne kgpayne merged commit c48517a into meltano:main Jun 27, 2023
@BuzzCutNorman BuzzCutNorman deleted the 1676-fix-incremental-where-clause-typeerror branch November 28, 2023 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: Default where clause generation for incremental replication_key fails with TypeError
4 participants