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

ENH: Use pure SQL for return_type = 'id' queries #949

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

adelavega
Copy link
Collaborator

An alternative to #948 and #942

Instead of relying on looping over entities, allow the core logic of .get to return a subquery, rather than list of files.

This subquery can then be used to find the unique entities of a given target for a list files, in pure SQL.

This can a bit faster (22ms vs 6-7 ms on my test dataset w/ ~80 subs), which could make a difference on very large datasets.

@adelavega
Copy link
Collaborator Author

I would say we hold off on this until we have developed benchmarks that justify the potential added complexity

@codecov
Copy link

codecov bot commented Feb 16, 2023

Codecov Report

Patch coverage: 82.60% and project coverage change: +0.02% 🎉

Comparison is base (ae1bb00) 83.46% compared to head (0079807) 83.48%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #949      +/-   ##
==========================================
+ Coverage   83.46%   83.48%   +0.02%     
==========================================
  Files          38       38              
  Lines        4305     4311       +6     
  Branches     1096     1098       +2     
==========================================
+ Hits         3593     3599       +6     
  Misses        514      514              
  Partials      198      198              
Files Changed Coverage Δ
bids/layout/layout.py 88.60% <82.60%> (+0.13%) ⬆️

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

@effigies effigies marked this pull request as draft March 6, 2023 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant