Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ARROW-1473: [JAVA] Refactoring Value Vector (Implementation Phase 1) #1198

Closed
wants to merge 1 commit into from
Closed

ARROW-1473: [JAVA] Refactoring Value Vector (Implementation Phase 1) #1198

wants to merge 1 commit into from

Conversation

siddharthteotia
Copy link
Contributor

@siddharthteotia siddharthteotia commented Oct 13, 2017 •
edited
Loading

creating the PR against refactor branch

see original PR on master for detailed discussion -- #1164

cc @jacques-n , @BryanCutler , @icexelloss , @StevenMPhillips , @elahrvivaz

@icexelloss
Copy link
Contributor

icexelloss commented Oct 13, 2017 •
edited
Loading

Can you change the base branch of the origin one so we don't lose the discussions?

@siddharthteotia
Copy link
Contributor Author

I will update this PR later today or tomorrow (just doing some cleanup, adding comments, TODOs) and then we can work towards merging this patch.

@siddharthteotia
Copy link
Contributor Author

This PR is ready to be merged to java-vector-refactor branch.

No outstanding correctness failures. There are 10 test errors and all are related to getMutator(), getAccessor() functions throwing UnsupportedOperationException. The reason and solution has already been discussed on the mailing list.

Added comments, few TODOs (at the top of abstract classes BaseNullableFixedWidthVector and BaseNullableVariableWidthVector).

@icexelloss
Copy link
Contributor

@siddharthteotia Can you change the base branch of #1164 so we can merge that one? Having two PR of the same thing is a bit confusing.

@icexelloss
Copy link
Contributor

Since all the discussion is on #1164 , I prefer to merge that one.

@wesm
Copy link
Member

wesm commented Oct 14, 2017

It won’t really matter. We should be cherry picking a single squashed commit into the branch. I can look tomorrow morning if it’s not done by then

@icexelloss
Copy link
Contributor

Ok. LGTM then. +1.

asfgit pushed a commit that referenced this pull request Oct 14, 2017
@siddharthteotia @wesm
ARROW-1473: ValueVector new hierarchy prototype (implementation phase 1)
7f45d86 
Close #1164
Close #1198

Change-Id: If18e42d2edfdfef83e83621334a5b65a390e9db9
asfgit pushed a commit that referenced this pull request Oct 14, 2017
@siddharthteotia @wesm
ARROW-1473: ValueVector new hierarchy prototype (implementation phase 1)
60a2ebd 
Close #1164
Close #1198

Change-Id: If18e42d2edfdfef83e83621334a5b65a390e9db9
@wesm
Copy link
Member

wesm commented Oct 14, 2017

Pushed as 60a2ebd

@siddharthteotia
Copy link
Contributor Author

Thanks @wesm and @icexelloss . Closing this. I am changing the JIRA number in other PR to the umbrella JIRA and we can keep it open to reference and follow up on discussions,

siddharthteotia added a commit to siddharthteotia/arrow that referenced this pull request Nov 14, 2017
@siddharthteotia
ARROW-1473: ValueVector new hierarchy prototype (implementation phase 1)
6493ebf 
Close apache#1164
Close apache#1198

Change-Id: If18e42d2edfdfef83e83621334a5b65a390e9db9
wesm pushed a commit that referenced this pull request Nov 15, 2017
@siddharthteotia @wesm
ARROW-1473: ValueVector new hierarchy prototype (implementation phase 1)
42353ba 
Close #1164
Close #1198

Change-Id: If18e42d2edfdfef83e83621334a5b65a390e9db9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@siddharthteotia @icexelloss @wesm