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

[PECO-1532] Ignore the excess records in query results #239

Merged
merged 5 commits into from
Mar 27, 2024

Conversation

kravets-levko
Copy link
Collaborator

@kravets-levko kravets-levko commented Mar 21, 2024

PECO-1532

Note for reviewers: This PR contains some refactoring needed to implement the fix, so probably it's easier to review commit by commit

When client library executes query and wants an Arrow-based or Cloudfetch results - server will return records as Arrow batches. Batch size may vary, server makes the decision on that depending on count of records, record size, etc. But usually all batches will have the same size, with the only exception - the last batch, which usually contains less records. And there are two possibilities:

  • server may make the last batch smaller and containing only the remaining records;
  • server may actually fetch more record than needed to make the last batch of the same size as others. But it also sets a rowCount field which defines how may "valid" records are in the batch. Client should use only that records and discard the remaining ones.

(I guess that different workspaces may be configured differently and will behave either as described in scenario 1 or 2)

Nodejs connector doesn’t use value from rowCount and therefore returns that extra records to user. This behavior is wrong, and this PR fixes it.

…th raw batch data

Signed-off-by: Levko Kravets <levko.ne@gmail.com>
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
Copy link
Contributor

@benc-db benc-db left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me, but I'm neither a node nor thrift semantics expert.

@kravets-levko kravets-levko merged commit 1dc16ac into main Mar 27, 2024
8 checks passed
@kravets-levko kravets-levko deleted the PECO-1532-ignore-excess-records branch March 27, 2024 12:16
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

2 participants