Skip to content
New issue

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

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

Already on GitHub? # to your account

fix(targets): Make sink assertion compatible with stream maps #2589

Merged
merged 5 commits into from
Aug 7, 2024

Conversation

JCotton1123
Copy link
Contributor

@JCotton1123 JCotton1123 commented Aug 5, 2024

The change here is necessary to ensure the assertion utilizes the aliases stream name (vs the original stream name).

Given a stream_maps config with an __alias__ configuration, see example below, a target will produce the following error:

singer_sdk.exceptions.RecordsWithoutSchemaException: A record for stream 'Account' was encountered before a corresponding schema. Check that the Tap correctly implements the Singer spec.
# meltano.yaml
...
  loaders:
  - config:
      athena_database: db
      s3_path: s3://bucket/data/
      stream_maps:
        Account:
          __alias__: Account_v2
    name: target-s3-parquet

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

Copy link

codspeed-hq bot commented Aug 5, 2024

CodSpeed Performance Report

Merging #2589 will not alter performance

Comparing JCotton1123:fix-sink-assert (532d980) with main (21821bd)

Summary

✅ 6 untouched benchmarks

Copy link

codecov bot commented Aug 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.47%. Comparing base (21821bd) to head (532d980).
Report is 118 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2589   +/-   ##
=======================================
  Coverage   89.47%   89.47%           
=======================================
  Files          58       58           
  Lines        4798     4800    +2     
  Branches      936      937    +1     
=======================================
+ Hits         4293     4295    +2     
  Misses        352      352           
  Partials      153      153           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@edgarrmondragon edgarrmondragon changed the title Move sink exists assertion to be compatible with stream_maps fix: Move sink exists assertion to be compatible with stream_maps Aug 5, 2024
@edgarrmondragon edgarrmondragon self-assigned this Aug 5, 2024
@edgarrmondragon edgarrmondragon changed the title fix: Move sink exists assertion to be compatible with stream_maps fix: Move sink exists assertion to be compatible with stream maps Aug 5, 2024
JCotton1123 added a commit to SaaSWorksInc/target-s3-parquet that referenced this pull request Aug 5, 2024
@edgarrmondragon
Copy link
Collaborator

@JCotton1123 thanks for the PR!

Can you expand on what problem this solves? The original code does smell like a bug, but I'd like to understand better what errors you're running into with the current implementation.

@JCotton1123
Copy link
Contributor Author

@JCotton1123 thanks for the PR!

Can you expand on what problem this solves? The original code does smell like a bug, but I'd like to understand better what errors you're running into with the current implementation.

I just added a bit more detail. Let me know if I can provide more.

@edgarrmondragon edgarrmondragon changed the title fix: Move sink exists assertion to be compatible with stream maps fix: Move sink assertion to be compatible with stream maps Aug 6, 2024
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 @JCotton1123!

@edgarrmondragon edgarrmondragon changed the title fix: Move sink assertion to be compatible with stream maps fix(targets): Make sink assertion compatible with stream maps Aug 7, 2024
@edgarrmondragon edgarrmondragon added this pull request to the merge queue Aug 7, 2024
Merged via the queue into meltano:main with commit 5f2aa19 Aug 7, 2024
32 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants