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

perf(taps): Improved discovery performance for SQL taps #2793

Merged
merged 8 commits into from
Dec 3, 2024

Conversation

edgarrmondragon
Copy link
Collaborator

@edgarrmondragon edgarrmondragon commented Dec 2, 2024

TODO:

  • Measure it πŸ“ˆ
  • Document it
  • Add a Codspeed benchmark? 🏎️

πŸ“š Documentation preview πŸ“š: https://meltano-sdk--2793.org.readthedocs.build/en/2793/

@edgarrmondragon edgarrmondragon linked an issue Dec 2, 2024 that may be closed by this pull request
@edgarrmondragon edgarrmondragon self-assigned this Dec 2, 2024
@edgarrmondragon edgarrmondragon force-pushed the 2166-discovery-performance branch from 4818ea2 to c0d33ab Compare December 2, 2024 23:30
Copy link

codspeed-hq bot commented Dec 2, 2024

CodSpeed Performance Report

Merging #2793 will improve performances by 36.49%

Comparing 2166-discovery-performance (304e206) with main (b989747)

Summary

⚑ 1 improvements
βœ… 6 untouched benchmarks

Benchmarks breakdown

Benchmark main 2166-discovery-performance Change
⚑ test_bench_discovery 988.5 ms 724.2 ms +36.49%

Copy link

codecov bot commented Dec 2, 2024

Codecov Report

Attention: Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.

Project coverage is 91.27%. Comparing base (b989747) to head (304e206).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
singer_sdk/connectors/sql.py 94.73% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2793      +/-   ##
==========================================
+ Coverage   91.24%   91.27%   +0.02%     
==========================================
  Files          62       62              
  Lines        5188     5182       -6     
  Branches      670      669       -1     
==========================================
- Hits         4734     4730       -4     
+ Misses        321      319       -2     
  Partials      133      133              

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

@edgarrmondragon edgarrmondragon added this to the v0.43 milestone Dec 3, 2024
@edgarrmondragon edgarrmondragon force-pushed the 2166-discovery-performance branch 3 times, most recently from b323742 to 14a5d5c Compare December 3, 2024 04:02
@@ -969,21 +1039,30 @@ def discover_catalog_entries(self) -> list[dict]:
result: list[dict] = []
engine = self._engine
inspected = sa.inspect(engine)
object_kinds = (
(reflection.ObjectKind.TABLE, False),
(reflection.ObjectKind.ANY_VIEW, True),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The regression in MeltanoLabs/tap-postgres#528 was probably caused by ignoring views in the updated discovery process.

See MeltanoLabs/tap-postgres#535 (comment).

@edgarrmondragon edgarrmondragon force-pushed the 2166-discovery-performance branch 9 times, most recently from 83a0731 to 6c69502 Compare December 3, 2024 16:17
@edgarrmondragon edgarrmondragon force-pushed the 2166-discovery-performance branch from 6c69502 to 6eaca46 Compare December 3, 2024 16:32
@edgarrmondragon edgarrmondragon marked this pull request as ready for review December 3, 2024 22:53
@edgarrmondragon edgarrmondragon merged commit d3dcc8e into main Dec 3, 2024
34 of 36 checks passed
@edgarrmondragon edgarrmondragon deleted the 2166-discovery-performance branch December 3, 2024 23:03
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.

Discovery performance
1 participant