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(taps): Use nulls_first when available to order NULL results in incremental SQL streams #2094

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

edgarrmondragon
Copy link
Collaborator

@edgarrmondragon edgarrmondragon commented Dec 8, 2023

RDBMS Supports NULLS FIRST Default NULLs order in ASC
SQlite 3.30.0+ First
MySQL No First
Postgres 8.3+ Last
Snowflake Yes Last

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

Copy link

codspeed-hq bot commented Dec 8, 2023

CodSpeed Performance Report

Merging #2094 will not alter performance

Comparing edgar/fix/sql-nullsfirst (0c07836) with main (d755024)

Summary

✅ 4 untouched benchmarks

Copy link

codecov bot commented Dec 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d755024) 87.71% compared to head (0c07836) 87.72%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2094   +/-   ##
=======================================
  Coverage   87.71%   87.72%           
=======================================
  Files          60       60           
  Lines        4942     4946    +4     
  Branches     1007     1007           
=======================================
+ Hits         4335     4339    +4     
  Misses        425      425           
  Partials      182      182           

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

@edgarrmondragon edgarrmondragon marked this pull request as ready for review December 8, 2023 02:32
@edgarrmondragon edgarrmondragon changed the title fix: Use nullsfirst to order results in incremental SQL streams fix: Use nullsfirst to order NULL results in incremental SQL streams Dec 8, 2023
@edgarrmondragon edgarrmondragon changed the title fix: Use nullsfirst to order NULL results in incremental SQL streams fix(taps): Use nullsfirst to order NULL results in incremental SQL streams Dec 8, 2023
@edgarrmondragon edgarrmondragon changed the title fix(taps): Use nullsfirst to order NULL results in incremental SQL streams fix(taps): Use nulls_first to order NULL results in incremental SQL streams Dec 8, 2023
@edgarrmondragon edgarrmondragon force-pushed the edgar/fix/sql-nullsfirst branch 2 times, most recently from 5c658c2 to 54469e9 Compare December 8, 2023 16:41
@edgarrmondragon edgarrmondragon changed the title fix(taps): Use nulls_first to order NULL results in incremental SQL streams fix(taps): Use nulls_first when available to order NULL results in incremental SQL streams Dec 8, 2023
@edgarrmondragon edgarrmondragon force-pushed the edgar/fix/sql-nullsfirst branch from 54469e9 to 0c07836 Compare December 8, 2023 16:45
@edgarrmondragon edgarrmondragon requested a review from a team as a code owner December 8, 2023 16:45
@edgarrmondragon edgarrmondragon merged commit 5293f50 into main Dec 8, 2023
30 checks passed
@edgarrmondragon edgarrmondragon deleted the edgar/fix/sql-nullsfirst branch December 8, 2023 17:20
edgarrmondragon added a commit to MeltanoLabs/tap-postgres that referenced this pull request Dec 12, 2023
~This was fixed upstream in meltano/sdk#1487,
but this tap overrides `get_records`.~

I think meltano/sdk#2094 is the only difference
between the two implementations?

UPDATE: Using the private `._mapping` attribute makes me nervous so I
opened meltano/sdk#2095 and updated this PR.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant