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

QueryBuilder #1153

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

QueryBuilder #1153

wants to merge 1 commit into from

Conversation

sanmadjack
Copy link
Contributor

I've got a fair number of changes I've built up over the last few years, but a lot of things are dependent on a more featured query builder I created awhile back. This QueryBuilder allows extensions to be able to build much optimized queries while still working with the simple Querylets that existing code uses.

Along with this comes a fix for a tag count issue, and a new "limit" search term that allows restricting the total number of items that are found independent of the current page size. This limit tag is useful for limiting the scope of large bulk operations.

@shish
Copy link
Owner

shish commented May 15, 2024

At a quick glance this looks pretty cool :O

Heads up I am currently focussed on tying up loose ends at my day job, in preparation for taking a year out and doing all the open-source stuff that I've been wishing I had time for - so I'm probably going to be in "major bugfixes only" mode for a few more weeks, and after that full steam ahead with PR-merging and development work /o/

@discomrade
Copy link
Contributor

You mentioned this query builder was created some time ago, is your re-implementation up to date with any changes made in the main codebase after the query builder was first created? Or have they diverged? (I haven't checked yet)

a fix for a tag count issue

Out of curiosity, what was the issue?

@sanmadjack
Copy link
Contributor Author

That's fine, I'm still getting it updated to work with all the changes and the new tests you've set up since I last contributed. As the tests show, I still have work to do.

@sanmadjack
Copy link
Contributor Author

You mentioned this query builder was created some time ago, is your re-implementation up to date with any changes made in the main codebase after the query builder was first created? Or have they diverged? (I haven't checked yet)

Definitely some divergence, but not nearly as much as expected.

a fix for a tag count issue

Out of curiosity, what was the issue?

On review I think I've mis-interpreted the code while fixing something else with the new query builder. I'm reverting my incorrect fix.

Added limit search term
Fixed tag count evaluation for single-tag searches
@shish
Copy link
Owner

shish commented Sep 7, 2024

Sorry for taking forever on this ^^;

Some disconnected thoughts:

  • Trying to understand it in chunks to get my head around it, my first thought in that direction is that QueryBuilder feels like a stand-alone PHP package, which could be tested and released independently and then shimmie adds it as a dependency - would you like to do that? Would you mind if I did it?
  • adding limit=X as a meta-tag feels like a separate feature, and one that needs some more thought -- I tried pulling that out and merging that by itself, but then realised that it doesn't make sense to have limit=X directly map to SELECT * FROM ... LIMIT X, given results coming in pages (consider eg page size 10, user searches limit=20, I would expect that to generate two full pages of results - but at the moment it ignores the limit being larger than page size (sensible for preventing DoS), and if we did allow limit=X to override page size, then we'd have one page of 20 results instead of two pages of 10)
  • the numeric_score changes got merged as those seem unrelated to the rest of the commit
  • class TermConditions does seem like a better idea than an array of search bits, I'll merge that now, though renamed SearchParameters for clarity since Term and Condition already have different meanings in this context

shish added a commit that referenced this pull request Sep 7, 2024
This seems close to working, except that "page size" and "total number of search results" are two distinct concepts, and SQL LIMIT is used for page size already, so it doesn't work to also use it for total number of search results...
shish added a commit that referenced this pull request Sep 7, 2024
This seems close to working, except that "page size" and "total number of search results" are two distinct concepts, and SQL LIMIT is used for page size already, so it doesn't work to also use it for total number of search results...
shish added a commit that referenced this pull request Sep 7, 2024
This seems close to working, except that "page size" and "total number of search results" are two distinct concepts, and SQL LIMIT is used for page size already, so it doesn't work to also use it for total number of search results...
@sanmadjack
Copy link
Contributor Author

No hurry on this, I've been distracted off by work stuff so it is what it is.

I agree QueryBuilder could be its own thing. I think I would like to do that, I haven't made a PHP package before and I'd like to take a crack at it.

The limit tag was a headache to get working. The purpose for me adding it was so that I could limit how many posts would be affected by extremely large bulk operations. Currently it's either select a whole page, or all of the results total. I actually have a page size switcher in my custom branch, but even that is limited by the browser needing to load potentially thousands of thumbnails when doing a controlled size operation. the limit tag allowed me to affect exactly as many posts as I want while only needing to load up one page's worth. The scenarios you are mentioning are what I did deal with, and I did come up with a system of overrides that allows limit=20 to produce two pages of 10. I'll look at the WIP branch and see what I missed in the PR that makes it all work.

I'll update my code for the new SearchCondition nomenclature, targeting the QueryBuilder package and update this pull request once I have all that worked out.

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.

3 participants