Uh oh!
There was an error while loading. Please reload this page.
ARROW-1300: [JAVA] Fix Tests for ListVector - #925
Conversation
siddharthteotia
commented
Aug 1, 2017
For some reason, git shows more diff in changes than I have actually made. I have added lines 42, 96-115 and 457-572 however, git shows a lot of additional space/tab changes which I haven't really made |
It looks like there's still a bunch of style issues in the codebase. Some time ago we added checkstyle checks but it's still showing a lot of warnings. It would be good to get checkstyle to a clean state cc @icexelloss |
siddharthteotia
commented
Aug 1, 2017
thanks @wesm In the latest commit, I have tried to get rid of most of the tab/space changes. |
icexelloss
commented
Aug 1, 2017
@wesm, I can help with clean up the style. Track here: https://issues.apache.org/jira/browse/ARROW-1304 |
| import org.junit.Test; | ||
| import java.util.List; | ||
| import java.util.ArrayList; |
There was a problem hiding this comment.
Should these two import switch order? sortImportsInGroupAlphabetically
| /* index 0 */ | ||
| Object result = accessor.getObject(0); | ||
| ArrayList<Long> resultSet = (ArrayList<Long>)result; |
There was a problem hiding this comment.
Need space between (ArrayList<Long>) and result
There was a problem hiding this comment.
Actually, google_checks.xml doesn't specify TypecastParenPad rule, so this is fine.
| assertEquals("Different data lengths at index: " + i + " and start: " + start, | ||
| dataLength1, dataLength2); | ||
| assertEquals("Different data lengths at index: " + i + " and start: " + start, dataLength1, dataLength2); |
There was a problem hiding this comment.
This line is more than 100 length
| assertEquals(4, resultSet.get(1).size()); /* size of second inner list */ | ||
| list = resultSet.get(0); | ||
| assertEquals(new Long(50), (Long)list.get(0)); |
There was a problem hiding this comment.
Why do you need to cast to Long here?
There was a problem hiding this comment.
Not needed. Addressed
| assertEquals(2, accessor.getValueCount()); | ||
| /* get listVector value at index 0 -- the value itself is a listvector */ | ||
| Object result = accessor.getObject(0); |
There was a problem hiding this comment.
Unrelated:
Is there a way to get unboxed value from nested list vector?
StevenMPhillips
commented
Aug 4, 2017
+1 |
siddharthteotia
commented
Aug 4, 2017
Addressed review comments in the latest commit. |
icexelloss
commented
Aug 4, 2017
+1 |
@StevenMPhillips Fixed the following: (1) TestListVector.java doesn't include tests for nested lists where the underlying dataVector for a listVector is also a listVector. (2) The copy test in TestListVector.java only checks the bit vector contents and doesn't verify the actual contents of list vector Author: siddharth <siddharth@dremio.com> Closesapache#925 from siddharthteotia/ARROW-1300 and squashes the following commits: 584c79e [siddharth] ARROW-1300: Fix tests for ListVector ff84253 [siddharth] ARROW-1300: Fix Tests for ListVector 9978199 [siddharth] ARROW-1300: Fix tests for ListVector 777e0de [siddharth] ARROW-1300: Fix Tests for ListVector
@StevenMPhillips
Fixed the following:
(1) TestListVector.java doesn't include tests for nested lists where the underlying dataVector for a listVector is also a listVector.
(2) The copy test in TestListVector.java only checks the bit vector contents and doesn't verify the actual contents of list vector