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(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).

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
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.

2 participants