Skip to content

Conversation

@kravets-levko
Copy link
Contributor

@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
@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.

3 participants