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

search_after parameter is not properly supported #1099

Open
avelanarius opened this issue Dec 12, 2024 · 1 comment
Open

search_after parameter is not properly supported #1099

avelanarius opened this issue Dec 12, 2024 · 1 comment
Assignees
Labels
enhancement New feature or request

Comments

@avelanarius
Copy link
Member

The "Discover" view in Kibana shows 500 results by default, but the user can request additional results by clicking "Load more":

image

This makes Kibana send a query with search_after parameter in the query JSON body. Currently Quesma ignores this parameter, so the next pages don't load properly.

@avelanarius avelanarius added the enhancement New feature or request label Dec 12, 2024
@trzysiek trzysiek self-assigned this Dec 12, 2024
@trzysiek
Copy link
Member

Looking at it today

github-merge-queue bot pushed a commit that referenced this issue Jan 7, 2025
…strategies (#1104)

#1099 1/2 PR

After this PR instead of discarding `search_after` param, we simply add
`tuple(sort_field_1, sort_field_2, ...)<(search_after_1, search_after_2,
...) ` to the `WHERE` clause, so we'll (almost) properly return
paginated hits (never wrong ones, but we can skip some valid ones)
If `sort_field_i` is `asc`, we swap `sort_field_i` with `search_after_i`
in the expression above, I think it gives exactly what we want.

Timestamp fields use `fromUnixTimestampMilli64` conversion - it'll work
because the `search_after`param is what we returned in the previous
request ourselves, for previous hits. And for dates/timestamps we always
return `unix_timestamp in ms`.

This strategy is not fully correct, because we'll skip all hits with
exactly the same sort values (`timestamps in ms`, usually), but it's
some progress anyway. Before this PR, queries with this param basically
didn't work at all, now they have a small bug.

TODO (I'll finish in next PR): 
* Finish Bulletproof strategy, which fixes the limitation above. Should
be quick as I already got a lot of tests here, and there's not really a
lot of code in the logic itself.
* Fix conversion function, we always use `from...Milli64`. It'll always
work for Clickhouse, but for Hydrolix and `DateTime` field (without
`64`), it won't. It's a known issue, quick to fix. I'd do it in another
PR, as before this one, `search_after` didn't work anyway, so there's no
regression here.

Screens of this working after all the commits:
<img width="1439" alt="Screenshot 2024-12-27 at 00 58 42"
src="https://github.com/user-attachments/assets/c597a4a3-5f80-41c2-85dc-b79516a73948"
/>
<img width="1429" alt="Screenshot 2024-12-27 at 00 58 57"
src="https://github.com/user-attachments/assets/44fe9c0c-9ec2-4964-b136-91f934849d28"
/>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants