Skip to content
This repository has been archived by the owner on Nov 14, 2024. It is now read-only.

getRowsColumnRangeIterator #4120

Merged
merged 8 commits into from
Jul 11, 2019
Merged

Conversation

sandorw
Copy link
Contributor

@sandorw sandorw commented Jul 10, 2019

Goals (and why):
Adds a new method to tables getRowsColumnRangeIterator that is similar to getRowsColumnRange, except it returns an Iterator instead of a BatchingVisitable.

Implementation Description (bullets):
Mostly modifications to generated tables that are checked in.

Testing (What was existing testing like? What have you done to improve it?):
Copied existing tests using getRowsColumnRange. I didn't want to combine the testing in case any implementation changes in the future, and the additional tests should be cheap. The contract should be the same, so existing tests should cover the functionality.

Concerns (what feedback would you like?):

Where should we start reviewing?:
Skip all the generated files, everything else should be relevant and fairly quick.

Priority (whenever / two weeks / yesterday):

@changelog-app
Copy link

changelog-app bot commented Jul 10, 2019

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Added a new method to generated table schemas, getRowsColumnRangeIterator, that returns an iterator for each row rather than a BatchingVisitable.

Check the box to generate changelog(s)

  • Generate changelog entry

@@ -745,7 +823,38 @@ public void testReadMyWritesColumnRangePagingTransaction() {
}

@Test
public void testReadMyDeletesColumnRangePagingTransaction() {
public void testReadMyWritesColumnRangePagingTransaction_iterator() {
Transaction t = startTransaction();
Copy link
Contributor

Choose a reason for hiding this comment

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

a better/easier/clearer test would be doing the setup as normal, and then assert in the batching visitable view, and then do the iterator view, and then assert that the contents are the same? at the same time, not too fussed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, got stuck in this pattern after touching a few tests where it was not appropriate to combine the two

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I consolidated in the few cases where it was safe to do so without making the test harder to understand or where the it was important to the test itself

@sandorw
Copy link
Contributor Author

sandorw commented Jul 11, 2019

Added a few more tests to explicitly cover the case where we do read a result from the row, but only conflict with another writer if we attempt to read past the first result.

Copy link
Contributor

@felixdesouza felixdesouza left a comment

Choose a reason for hiding this comment

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

Small question about a test, but is likely just misunderstanding, so will approve.

@sandorw sandorw merged commit 592355d into develop Jul 11, 2019
@delete-merged-branch delete-merged-branch bot deleted the sandorw/getRowsColumnRangeIterator branch July 11, 2019 11:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants