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

[DISCUSSION] Remove totalHits feature from pagination in V2 #1581

Closed
Yury-Fridlyand opened this issue Apr 25, 2023 · 4 comments
Closed

[DISCUSSION] Remove totalHits feature from pagination in V2 #1581

Yury-Fridlyand opened this issue Apr 25, 2023 · 4 comments
Labels
enhancement New feature or request pagination Pagination feature, ref #656

Comments

@Yury-Fridlyand
Copy link
Collaborator

Yury-Fridlyand commented Apr 25, 2023

Is your feature request related to a problem?

A discussion started from this post in #1497 raised a question: should we implement this feature or remove it (revert)?

If we keep this feature

Procs

  1. Compatibility with legacy engine
  2. Proper use of total and size in the response - these values were always equal before

Cons

  1. Rework required, implementation is not good
  2. It is not used; main consumer of JDBC response format is the JDBC driver which ignores both these values

If we remove this feature

Procs

  1. Reduce impact of pagination feature

Cons

  1. A breaking change: legacy engine properly sets total and size for paging requests
    Note: it is still not used by JDBC (ref: JsonQueryResponse).

Please, feel free to edit procs/cons lists above and post your votes. Removal was already discussed in meetings and in the slack channel, but should be posted/copied here for visibility.

@Yury-Fridlyand Yury-Fridlyand added enhancement New feature or request untriaged labels Apr 25, 2023
@penghuo
Copy link
Collaborator

penghuo commented Apr 27, 2023

Is it possible,

  1. if the request from JDBC, execute in new engine, ignore total field.
  2. if the request from others, fallback to legacy.

@Yury-Fridlyand Yury-Fridlyand added the pagination Pagination feature, ref #656 label May 1, 2023
@Yury-Fridlyand
Copy link
Collaborator Author

@penghuo,
Could you please clarify a bit:

if the request from JDBC, execute in new engine, ignore total field.

What does mean ignore? Return total equal to size as it was before (breaking with V1) or not to return it at all (breaking at all)?

if the request from others, fallback to legacy.

That means any paging request in non-jdbc format will be processed in V1. Why?
Note: legacy engine does not support paging for non-jdbc format too - it returns non-paged results (aka ignores fetch_size).

@penghuo
Copy link
Collaborator

penghuo commented May 1, 2023

  1. If the request from JDBC driver, does total hits field required? if not required, we can ignore it.
  2. Ignore my comments of "if the request from others, fallback to legacy"

@Yury-Fridlyand
Copy link
Collaborator Author

Complete by #1649

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request pagination Pagination feature, ref #656
Projects
None yet
Development

No branches or pull requests

3 participants