FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

GH-470: ListViewVector#getElementEndIndex should returns size not index by joyCurry30 · Pull Request #554 · apache/arrow-java · GitHub

/ arrow-java Public

GH-470: ListViewVector#getElementEndIndex should returns size not index - #554

Open
joyCurry30 wants to merge 2 commits into
apache:mainfrom
joyCurry30:GH-470
ConversationCommits (2)ChecksFiles changed
Open

GH-470: ListViewVector#getElementEndIndex should returns size not index#554
joyCurry30 wants to merge 2 commits into
apache:mainfrom
joyCurry30:GH-470

Conversation

joyCurry30 commented Jan 23, 2025

Copy link
Copy Markdown

Close #470

kou changed the title [GH-470]ListViewVector#getElementEndIndex should returns size not index. GH-470: ListViewVector#getElementEndIndex should returns size not index. Jan 23, 2025
kou changed the title GH-470: ListViewVector#getElementEndIndex should returns size not index. GH-470: ListViewVector#getElementEndIndex should returns size not index Jan 23, 2025
wgtmac reviewed Jan 24, 2025
View reviewed changes
Comment thread vector/src/main/java/org/apache/arrow/vector/complex/ListViewVector.java
@Override
public int getElementEndIndex(int index) {
return sizeBuffer.getInt(index * OFFSET_WIDTH);
return offsetBuffer.getInt((index + 1) * OFFSET_WIDTH);

wgtmac Jan 24, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Should line 954 be changed together? BTW, are these methods covered by any unit test?

wgtmac Jan 24, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

To answer my 1st question: no.

nbauernfeind Jan 24, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't see any unit tests for it; and if possible would be worth a small one.

lidavidm Jan 27, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yeah, the fact that tests passed means nothing's covering it. Unfortunately the new types were only partially implemented before the contributor left.

lidavidm Jan 27, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Could add a few basic assertions to testBasicListViewVector https://github.com/apache/arrow-java/blob/main/vector/src/test/java/org/apache/arrow/vector/TestListViewVector.java

joyCurry30 Jan 27, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Good suggest. I will add UT for it.

lidavidm Jan 28, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks!

lidavidm Mar 12, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Shouldn't the end index be offset + size + 1? (Assuming noninclusive)

nbauernfeind commented Jan 24, 2025

Copy link
Copy Markdown
Contributor

@joyCurry30 Thank you so much for taking care of this!

joyCurry30 requested a review from jbonofre as a code owner March 11, 2025 13:35

This comment has been minimized.

Sign in to view

joyCurry30 commented Mar 11, 2025 •
edited
Loading

Copy link
Copy Markdown
Author

I am sorry for silence for so long. I have just add the ut for ListViewVector#getElementEndIndex. Could you please take a look when you are free? I'd be happy to address any feedback or make further improvement if needed. @nbauernfeind @wgtmac

lidavidm added the bug-fix PRs that fix a big. label Mar 11, 2025
github-actions Bot added this to the 18.3.0 milestone Mar 11, 2025
jbonofre removed this from the 18.3.0 milestone May 8, 2025
This file contains hidden or 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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

wgtmac wgtmac left review comments
lidavidm Awaiting requested review from lidavidm lidavidm is a code owner
laurentgo Awaiting requested review from laurentgo laurentgo is a code owner
jbonofre Awaiting requested review from jbonofre jbonofre is a code owner
+1 more reviewer
nbauernfeind nbauernfeind left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug-fix PRs that fix a big.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ListViewVector#getElementEndIndex Returns Size not Index

5 participants

Back | FazBrowse Home | New Git URL