Refactor Confusing Dwrf Reader RowNumber Method #11029
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
|
This pull request was exported from Phabricator. Differential Revision: D62412793 |
|
This pull request was exported from Phabricator. Differential Revision: D62412793 |
macvincent
pushed a commit
to macvincent/velox
that referenced
this pull request
Sep 18, 2024
) Summary: Pull Request resolved: facebookincubator#11029 We noticed that the `RowNumber` method in Dwrf reader produces two different outputs based on when it is called. It returns the current position of the reader after a seek operation and then the previous position of the reader before a `next` read operation was called. This makes it almost unusable without an understanding of its underlying implementation. In this diff, we modify it's design to return the current position of the reader for all calls. Differential Revision: D62412793
macvincent
force-pushed
the
export-D62412793
branch
from
809dd21
to
26a020d
Compare
macvincent
pushed a commit
to macvincent/velox
that referenced
this pull request
Sep 18, 2024
) Summary: Pull Request resolved: facebookincubator#11029 We noticed that the `RowNumber` method in Dwrf reader produces two different outputs based on when it is called. It returns the current position of the reader after a seek operation and then the previous position of the reader before a `next` read operation was called. This makes it almost unusable without an understanding of its underlying implementation. In this diff, we modify it's design to return the current position of the reader for all calls. Differential Revision: D62412793
macvincent
force-pushed
the
export-D62412793
branch
from
26a020d
to
64158ef
Compare
|
This pull request was exported from Phabricator. Differential Revision: D62412793 |
1 similar comment
|
This pull request was exported from Phabricator. Differential Revision: D62412793 |
macvincent
force-pushed
the
export-D62412793
branch
from
64158ef
to
ad72088
Compare
macvincent
pushed a commit
to macvincent/velox
that referenced
this pull request
Sep 18, 2024
) Summary: Pull Request resolved: facebookincubator#11029 We noticed that the `RowNumber` method in Dwrf reader produces two different outputs based on when it is called. It returns the current position of the reader after a seek operation and then the previous position of the reader before a `next` read operation was called. This makes it almost unusable without an understanding of its underlying implementation. In this diff, we modify it's design to return the current position of the reader for all calls. Differential Revision: D62412793
macvincent
pushed a commit
to macvincent/velox
that referenced
this pull request
Sep 18, 2024
) Summary: Pull Request resolved: facebookincubator#11029 We noticed that the `RowNumber` method in Dwrf reader produces two different outputs based on when it is called. It returns the current position of the reader after a seek operation and then the previous position of the reader before a `next` read operation was called. This makes it almost unusable without an understanding of its underlying implementation. In this diff, we modify it's design to return the current position of the reader for all calls. Differential Revision: D62412793
macvincent
force-pushed
the
export-D62412793
branch
from
ad72088
to
163f06d
Compare
|
This pull request was exported from Phabricator. Differential Revision: D62412793 |
macvincent
pushed a commit
to macvincent/velox
that referenced
this pull request
Sep 18, 2024
) Summary: Pull Request resolved: facebookincubator#11029 We noticed that the `RowNumber` method in Dwrf reader produces two different outputs based on when it is called. It returns the current position of the reader after a seek operation and then the previous position of the reader before a `next` read operation was called. This makes it almost unusable without an understanding of its underlying implementation. In this diff, we modify it's design to return the current position of the reader for all calls. Differential Revision: D62412793
macvincent
force-pushed
the
export-D62412793
branch
from
163f06d
to
7c9ea98
Compare
|
This pull request was exported from Phabricator. Differential Revision: D62412793 |
macvincent
force-pushed
the
export-D62412793
branch
from
7c9ea98
to
f1f8112
Compare
|
This pull request was exported from Phabricator. Differential Revision: D62412793 |
macvincent
pushed a commit
to macvincent/velox
that referenced
this pull request
Sep 19, 2024
) Summary: Pull Request resolved: facebookincubator#11029 We noticed that the `RowNumber` method in Dwrf reader produces two different outputs based on when it is called. It returns the current position of the reader after a seek operation and then the previous position of the reader before a `next` read operation was called. This makes it almost unusable without an understanding of its underlying implementation. In this diff, we modify it's design to return the current position of the reader for all calls. Differential Revision: D62412793
macvincent
force-pushed
the
export-D62412793
branch
from
f1f8112
to
c75e627
Compare
|
This pull request was exported from Phabricator. Differential Revision: D62412793 |
1 similar comment
|
This pull request was exported from Phabricator. Differential Revision: D62412793 |
macvincent
force-pushed
the
export-D62412793
branch
from
c75e627
to
7d3eea6
Compare
macvincent
pushed a commit
to macvincent/velox
that referenced
this pull request
Sep 19, 2024
) Summary: Pull Request resolved: facebookincubator#11029 We noticed that the `RowNumber` method in Dwrf reader produces two different outputs based on when it is called. It returns the current position of the reader after a seek operation and then the previous position of the reader before a `next` read operation was called. This makes it almost unusable without an understanding of its underlying implementation. In this diff, we modify it's design to return the current position of the reader for all calls. Differential Revision: D62412793
|
This pull request was exported from Phabricator. Differential Revision: D62412793 |
macvincent
pushed a commit
to macvincent/velox
that referenced
this pull request
Sep 19, 2024
) Summary: Pull Request resolved: facebookincubator#11029 We noticed that the `RowNumber` method in Dwrf reader produces two different outputs based on when it is called. It returns the current position of the reader after a seek operation and then the previous position of the reader before a `next` read operation was called. This makes it almost unusable without an understanding of its underlying implementation. In this diff, we modify it's design to return the current position of the reader for all calls. Differential Revision: D62412793
macvincent
force-pushed
the
export-D62412793
branch
from
7d3eea6
to
bf950ce
Compare
macvincent
pushed a commit
to macvincent/velox
that referenced
this pull request
Sep 25, 2024
) Summary: Pull Request resolved: facebookincubator#11029 We noticed that the `RowNumber` method in Dwrf reader produces two different outputs based on when it is called. It returns the current position of the reader after a seek operation and then the previous position of the reader before a `next` read operation was called. This makes it almost unusable without an understanding of its underlying implementation. In this diff, we modify it's design to return the current position of the reader for all calls. Reviewed By: helfman Differential Revision: D62412793
macvincent
force-pushed
the
export-D62412793
branch
from
d738674
to
3914e42
Compare
|
This pull request was exported from Phabricator. Differential Revision: D62412793 |
1 similar comment
|
This pull request was exported from Phabricator. Differential Revision: D62412793 |
macvincent
force-pushed
the
export-D62412793
branch
from
3914e42
to
4979504
Compare
macvincent
pushed a commit
to macvincent/velox
that referenced
this pull request
Sep 27, 2024
) Summary: Pull Request resolved: facebookincubator#11029 We noticed that the `RowNumber` method in Dwrf reader produces two different outputs based on when it is called. It returns the current position of the reader after a seek operation and then the previous position of the reader before a `next` read operation was called. This makes it almost unusable without an understanding of its underlying implementation. In this diff, we modify it's design to return the current position of the reader for all calls. Reviewed By: helfman Differential Revision: D62412793
|
This pull request was exported from Phabricator. Differential Revision: D62412793 |
macvincent
pushed a commit
to macvincent/velox
that referenced
this pull request
Sep 27, 2024
) Summary: Pull Request resolved: facebookincubator#11029 We noticed that the `RowNumber` method in Dwrf reader produces two different outputs based on when it is called. It returns the current position of the reader after a seek operation and then the previous position of the reader before a `next` read operation was called. This makes it almost unusable without an understanding of its underlying implementation. In this diff, we modify it's design to return the current position of the reader for all calls. Reviewed By: helfman Differential Revision: D62412793
macvincent
force-pushed
the
export-D62412793
branch
from
4979504
to
a5657f4
Compare
|
This pull request was exported from Phabricator. Differential Revision: D62412793 |
macvincent
pushed a commit
to macvincent/velox
that referenced
this pull request
Sep 29, 2024
) Summary: Pull Request resolved: facebookincubator#11029 We noticed that the `RowNumber` method in Dwrf reader produces two different outputs based on when it is called. It returns the current position of the reader after a seek operation and then the previous position of the reader before a `next` read operation was called. This makes it almost unusable without an understanding of its underlying implementation. In this diff, we modify it's design to return the current position of the reader for all calls. Reviewed By: helfman Differential Revision: D62412793
macvincent
force-pushed
the
export-D62412793
branch
from
a5657f4
to
f981ee7
Compare
|
This pull request was exported from Phabricator. Differential Revision: D62412793 |
macvincent
pushed a commit
to macvincent/velox
that referenced
this pull request
Sep 30, 2024
) Summary: Pull Request resolved: facebookincubator#11029 We noticed that the `RowNumber` method in Dwrf reader produces two different outputs based on when it is called. It returns the current position of the reader after a seek operation and then the previous position of the reader before a `next` read operation was called. This makes it almost unusable without an understanding of its underlying implementation. In this diff, we modify it's design to return the current position of the reader for all calls. Reviewed By: helfman Differential Revision: D62412793
macvincent
force-pushed
the
export-D62412793
branch
from
f981ee7
to
4119013
Compare
pedroerp
approved these changes
Oct 1, 2024
|
This pull request was exported from Phabricator. Differential Revision: D62412793 |
macvincent
force-pushed
the
export-D62412793
branch
from
4119013
to
b5f50c5
Compare
macvincent
pushed a commit
to macvincent/velox
that referenced
this pull request
Oct 2, 2024
) Summary: Pull Request resolved: facebookincubator#11029 We noticed that the `RowNumber` method in Dwrf reader produces two different outputs based on when it is called. It returns the current position of the reader after a seek operation and then the previous position of the reader before a `next` read operation was called. This makes it almost unusable without an understanding of its underlying implementation. In this diff, we modify it's design to return the current position of the reader for all calls. Reviewed By: helfman Differential Revision: D62412793
|
This pull request was exported from Phabricator. Differential Revision: D62412793 |
macvincent
pushed a commit
to macvincent/velox
that referenced
this pull request
Oct 2, 2024
) Summary: Pull Request resolved: facebookincubator#11029 We noticed that the `RowNumber` method in Dwrf reader produces two different outputs based on when it is called. It returns the current position of the reader after a seek operation and then the previous position of the reader before a `next` read operation was called. This makes it almost unusable without an understanding of its underlying implementation. In this diff, we modify it's design to return the current position of the reader for all calls. Reviewed By: helfman Differential Revision: D62412793
macvincent
force-pushed
the
export-D62412793
branch
from
b5f50c5
to
e7272ad
Compare
) Summary: Pull Request resolved: facebookincubator#11029 We noticed that the `RowNumber` method in Dwrf reader produces two different outputs based on when it is called. It returns the current position of the reader after a seek operation and then the previous position of the reader before a `next` read operation was called. This makes it almost unusable without an understanding of its underlying implementation. In this diff, we modify it's design to return the current position of the reader for all calls. Reviewed By: helfman Differential Revision: D62412793
|
This pull request was exported from Phabricator. Differential Revision: D62412793 |
macvincent
force-pushed
the
export-D62412793
branch
from
e7272ad
to
1e0cc33
Compare
|
This pull request has been merged in f94aa6a. |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary: We noticed that the
RowNumbermethod in Dwrf reader produces two different outputs based on when it is called. It returns the current position of the reader after a seek operation and then the previous position of the reader before anextread operation was called. This makes it almost unusable without an understanding of its underlying implementation. In this diff, we modify it's design to return the current position of the reader for all calls.Differential Revision: D62412793