ARROW-1463: [JAVA - WIP] refactoring #1164

siddharthteotia · 2017-10-04T18:35:06Z

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

Prototype code (WIP) for BaseNullableFixedWidthVector abstract class and the implementation of subclass NullableIntVector.

ValueVector -> FixedWidthVector -> BaseNullableFixedVector (implements FieldVector) -> NullableIntVector

(1) No mutators and accessors
(2) Minimal delegation -- directly works with buffers for validity and actual data.
(3) No code generation
(4) Majority of the functionality abstracted out in the base class.
(5) Short call stack on hot paths.
(6) Buffer sharing is not yet done -- planning to first flush out the basic interfaces and get the infrastructure ready with new hierarchy.
(7) Need to figure out how to implement getFieldBuffers, loadFieldBuffers and other related methods that rely on vectors being BufferBacked.

siddharthteotia · 2017-10-05T09:25:01Z

Pushed new changes based on comments on design spec.

elahrvivaz · 2017-10-05T13:07:36Z

I like the general direction. would it make sense to have some of the basic getter/setting methods pulled out to the base class with type parameters? or would that be bad for performance... i.e.

BaseNullableFixedWidthVector<T> {
  public void set(int index, T value);
  public T get(int index);
}

One of the pain points I've experienced is no consistent get/set methods, so when working with different vectors they all have to be cast to the exact type.

jacques-n · 2017-10-04T19:21:28Z

java/vector/src/main/java/org/apache/arrow/vector/PrototypeIntVectorNonCodegen.java

can go in base class

jacques-n · 2017-10-04T19:22:22Z

java/vector/src/main/java/org/apache/arrow/vector/PrototypeIntVectorNonCodegen.java

can we get to only one specific transfer pair construction method (possibly protected/internal only)

jacques-n · 2017-10-04T19:22:51Z

java/vector/src/main/java/org/apache/arrow/vector/PrototypeIntVectorNonCodegen.java

let's avoid skipping over arrowbuf.

jacques-n · 2017-10-04T19:23:59Z

java/vector/src/main/java/org/apache/arrow/vector/PrototypeIntVectorNonCodegen.java

let's add a base vector method that does the while(realloc) pattern.

jacques-n · 2017-10-04T19:24:22Z

java/vector/src/main/java/org/apache/arrow/vector/PrototypeIntVectorNonCodegen.java

let's move this out of here.

jacques-n · 2017-10-04T19:25:21Z

java/vector/src/main/java/org/apache/arrow/vector/PrototypeIntVectorNonCodegen.java

Let's take these out of the vector classes and have shim classes that can be constructed for Mutator and Accessor if people want them.

jacques-n · 2017-10-04T19:26:28Z

java/vector/src/main/java/org/apache/arrow/vector/PrototypeIntVectorNonCodegen.java

any chance we can move most of this to a base class and use a generic parameter? Just have the copyValueSafe be per class?

icexelloss · 2017-10-05T15:04:02Z

@siddharthteotia This looks good so far. What I am concerned with are code duplication and maintainability if we hand write all the vector class. Can you maybe show (or describe) how NullableLongVector and IntVector would work? Specially for those setter/getter method.

I think @wesm suggested not having non-nullable/nullable vectors be two separate class to reduce the number of classes. We should discussion more on this.

icexelloss · 2017-10-05T15:06:46Z

@elahrvivaz I think boxing/unboxing will be a performance issue if we use generics. So that's not an option here?

icexelloss · 2017-10-05T15:07:57Z

@siddharthteotia

nit: Can you reformat the description in markdown? You can do list like this:

Something
Something else

jacques-n · 2017-10-05T15:17:00Z

@icexelloss: My preference is also to avoid having separate int and nullable int vectors. Only have NullableIntVector. As far as duplication goes, I think the second commit here seems to be small enough number of things that we could skip the templates and just manage a little more code.

@elahrvivaz I'm not entirely clear on how the casting helps with generics. one of the holder methods could be made to follow this pattern. Can you provide an example pattern where the generic setter will help avoid any casting? (Or are you just talking about less casting?) We built the reader/writer interfaces specifically to avoid casting (in cases where people don't use generated code).

wesm · 2017-10-05T15:25:34Z

should we create a wip-java-refactor where this work can take place without disrupting master? I presume we will require multiple patches

icexelloss · 2017-10-05T15:36:50Z

@jacques-n I see. Yeah the NullableIntVector does seem manageable.

Edit: doesn't -> does

jacques-n · 2017-10-05T16:11:54Z

@icexelloss, I was saying the NullableIntVector does seem manageable. Were you agreeing or disagreeing?

icexelloss · 2017-10-05T16:32:07Z

@jacques-n Ahh, sorry, there was a typo! Yes I agree.

elahrvivaz · 2017-10-05T17:50:57Z

@jacques-n I haven't been able to use the writer framework, as it doesn't support all the operations we're doing (like dictionary encoding and fixed size lists, if I remember correctly). Our main data structure is a GeoTools SimpleFeature, which is basically just an Array<Object> with well-defined types at each index. For a lot of our work, I can set things up front to be the right types, and then say have an array of Accessors and call .getObject() on them. However, Mutator doesn't have a generic setter method, so instead I have to cast down to the exact implementing class to call all the set methods.
@icexelloss possibly we could have the generic boxed methods, and specific unboxed methods in each impl? For me specifically I have to box everything anyway, but maybe that's not worth complicating the api for

BryanCutler · 2017-10-05T18:14:20Z

Wow, this is great so far! I really like that you were able to able to simplify and rid of Accessor and Mutator and a +1 for me for having a single NullableIntVector class without codegen. I think these are fairly stable now, so maintenance for all the vector types shouldn't be a big headache.

Is it possible we could rename vector classes too NullableIntVector to just IntVector since there is just one now, or is that too confusing?

BryanCutler · 2017-10-05T18:29:49Z

java/vector/src/main/java/org/apache/arrow/vector/BaseNullableFixedWidthVector.java

Is it possible to get typewidth from fieldtype? Or maybe add a mappng in ArrowType that you could query here?

Nvm, I guess we need the TYPE_WIDTH in the concrete class for get/set methods anyway

elahrvivaz · 2017-10-05T18:50:00Z

java/vector/src/main/java/org/apache/arrow/vector/NullableIntVector.java

this might be a good time to remove the MinorTypes and just use FieldTypes - I believe @jacques-n discussed this a while ago, that they don't actually capture all the field information.

If we could add a getType to return the ArrowType obj, that would be great. Something like what #972 proposed to avoid having to cast would be helpful. But I'd hate to add more work to this, maybe better to discuss after the main stuff is done.

I also found MinorType to be weird. But it's probably hard to remove due to compability? We could consider deprecating those though.

Added to requirement 7 in the requirement doc. Let's keep this out of this PR and discuss separately?

Yes, best to discuss separately. We will be merging multiple multiple patches to the new branch "java vector refactor" . We can do this once rest of the new code is stabilized, existing functionality is not broken etc.

Later on we can merge this branch into master once refactoring is done or this particular thing can be addressed directly on master once refactor branch has been merged. I am open to ideas.

jacques-n · 2017-10-05T15:22:01Z

java/vector/src/main/java/org/apache/arrow/vector/BaseNullableFixedWidthVector.java

How would that ever be the case? They should always move together.

jacques-n · 2017-10-05T15:22:11Z

java/vector/src/main/java/org/apache/arrow/vector/BaseNullableFixedWidthVector.java

jacques-n · 2017-10-05T15:23:21Z

java/vector/src/main/java/org/apache/arrow/vector/BaseNullableFixedWidthVector.java

why public?

jacques-n · 2017-10-05T15:24:40Z

java/vector/src/main/java/org/apache/arrow/vector/BaseNullableFixedWidthVector.java

We have too many instance objects here. Let's figure out how to reduce.

jacques-n · 2017-10-05T15:25:32Z

java/vector/src/main/java/org/apache/arrow/vector/BaseNullableFixedWidthVector.java

jacques-n · 2017-10-05T15:35:12Z

java/vector/src/main/java/org/apache/arrow/vector/BaseNullableFixedWidthVector.java

I think we can avoid passing field type and storing Field. Field is actually pretty heap memory heavy and can be derived based on other values here.

siddharthteotia · 2017-10-09T18:50:35Z

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

Notes on recent changes:

Addressed some comments. Some in progress.

The current patch has the basic implementation of interfaces for Nullable Fixed and Variable Width vectors with corresponding concrete implementation of respective subclasses - NullableIntVector, NullableVarCharVector.
Question: do we need allocationMonitor in our vector code? It complicates the logic of allocateNewSafe() and realloc().
Question: do we need setting reader and writer index on arrow bufs inside vectors? I think it doesn't really matter and vector get/set methods should still work fine.
I have made BaseValueVector as the top level abstract super class for the new hierarchy. Moved some methods there. I am not yet sure if we should have a complete different top level class like BaseNullableVector or use BaseValueVector. I think this is important since regardless of fixed or variable width property of vectors, there is still some common behavior which can be abstracted out in top level class.

siddharthteotia · 2017-10-09T19:00:13Z

My initial thoughts are to solidify this prototype -- flush out all APIs with complete implementation and tests and address comments. Right now we have concrete implementation for NullableInt and NullableVarChar in this prototype.

Can we have a separate project trunk/branch where we could merge patches for this work? Later on we can move the changes to Arrow master branch?

As we do this, we also need to think about getting rid of existing concrete implementation of vectors that is code generated. For example, once this prototype patch gets merged, we will need to remove NullableInt and NullableVarchar classes that are code generated.

cc @wesm, @jacques-n

icexelloss · 2017-10-11T15:27:28Z

java/vector/src/main/codegen/templates/NullableValueVectors.java

I wonder is it easier to have a separate template for Legency Vectors instead of if statements?

I am not sure about that. I am actually thinking if instead of having legacy vectors, can we just put all the new vectors in a different package. Changing the template code to generate legacy vectors and do delegations to new vectors is like a throwaway work because eventually we will get rid of template all-together.

nvm, I have made the changes to template to delegate the ops for Nullable Int and Nullable VarChar. Building things now locally and trying to run tests.

icexelloss · 2017-10-11T15:30:01Z

java/vector/src/main/codegen/templates/NullableValueVectors.java

Add "Use XXX instead" in deprecation warning?

icexelloss · 2017-10-11T15:31:19Z

java/vector/src/main/codegen/templates/UnionVector.java

UnionVector is another mess. Should we keep UnionVector out of ARROW-1463?

icexelloss · 2017-10-11T15:34:01Z

java/vector/src/main/java/org/apache/arrow/vector/BaseNullableFixedWidthVector.java

Maybe refactor allocationMonitor to it's own class?

I am trying to err on the side of less objects and less inner classes. So that is why thought we can maintain the allocationMonitor state through a simple counter. What do you think?

I think it's fine to not have a object, but maybe put the allocationMonitor logic in base class?

Right now we have other vector types inheriting from BaseValueVector. Once the entire hierarchy under BaseValueVector is refactored, we can move few things to BaseValueVector. There are a few things which are duplicated between BaseNullableFixedWidthVector and BaseNullableVariableWidthVector. Later on they can be moved to top level base class -- BaseValueVector.

In that case, I am leaning towards refactor allocationMonitor logic to a standalone class (if the logic is indeed the same among different vectors). I'd rather not maintaining the same logic in many different places...

I am just trying to phase out the work. Does the above explanation make some sense?

Yes it does make sense. It's fine. We can always refactor in a later patch if we want to.

That sounds good to me @siddharthteotia to move it to a top level class later. Do you think you could add a comment in the code for places that you anticipate changing in later phases?

Sure, I will add note(s) in the code. Thanks @BryanCutler and @icexelloss

icexelloss · 2017-10-11T15:41:30Z

java/vector/src/main/java/org/apache/arrow/vector/BaseNullableFixedWidthVector.java

Is this function not performance critical so it's fine to recompute?

Sorry, I don't understand. Are you saying we memoize the output of getNullCount()?

Yeah, kind of, something like keeping a nullCount variable in the class

It might be surprising to user this function is not O(1)

Ok, let's think about it.

I don't think it's necessarily to resolve in this PR. Can you put a TODO for this?

icexelloss · 2017-10-11T15:44:33Z

java/vector/src/main/java/org/apache/arrow/vector/BaseNullableVariableWidthVector.java

This function seems to be same as getNullCount in fix length vector

icexelloss · 2017-10-11T15:47:43Z

java/vector/src/main/java/org/apache/arrow/vector/BaseNullableVariableWidthVector.java

Should these be final protected?

Yes, that is correct. I missed it.

icexelloss · 2017-10-11T15:50:01Z

java/vector/src/main/java/org/apache/arrow/vector/BaseNullableVariableWidthVector.java

Is this common/well-defined enough to be moved to BitVectorHelper?

icexelloss · 2017-10-11T15:53:29Z

java/vector/src/main/java/org/apache/arrow/vector/NullableIntVector.java

Hmm..Do we need the logger method here?

Nvm I see different vector has different logger.

icexelloss · 2017-10-11T15:56:26Z

java/vector/src/main/java/org/apache/arrow/vector/NullableIntVector.java

This method seems generic enough, can this be in base class?

Yes, you are right.

icexelloss · 2017-10-11T17:06:31Z

java/vector/src/test/java/org/apache/arrow/vector/TestBufferOwnershipTransfer.java

Is getMutator removed from the new API?

Yes, one of the goals is to simplify the access and reduce the number of objects/classes. So all the getters and setters are directly accessible.

Ok, the existing Spark code won' compile with this then. @BryanCutler or I can make the change in Spark, but in general I wonder if this is too disruptive.

Doubling the number of getter/setter API is also not great, maybe we can add deprecated method and remove them in the next two releases after 0.8 or sth. This is more a question for @wesm and @jacques-n to if we are willing to break user code in 0.8.

I think the potential plan is to have the following:
Legacy Nullable Vector
Nullable Vector -- the ones being implemented as part of refactoring

The latter one is not mutator/accessor based.

The existing consumers of these Nullable vectors should be able to continue using them through the LegacyNullable vector which is merely a delegation to new code for each API.

For example, if in Dremio code, we replace each reference to NullableIntVector with LegacyNullableIntVector, the code should still work without any surprises.

We can keep the legacy code around for now and then remove them all together (along with NullableValueVectors template) in the next release or so.

I think it's ok to remove accessor/mutator here.

After thinking about this a bit more, I feel maintaining backward compatibility of the new vectors might not to possible. The reasons are because I think we want to rename some vectors properly:

DateDayVector, DateMillisVector ->DateVector

IntervalDayVector -> IntervalVector

Timestamp*Vector -> TimestampVector

MapVector -> StructVector

Since backward compatibility is broken anyway, we might as well just introduce new classes of vectors that is are slightly different (e.g, no accessor/mutator).

Also, I assume we would want ArrowReaders to return VectorSchemaRoot with new classes of vector in 0.8, is that correct?

Also, ArrowWriters should work with both legency and new vectors?

Btw, I updated the requirement doc and added the renaming requirement.

+1 on renaming. Are we also planning on removing the "Nullable" prefix to vectors? like NullableIntVector -> IntVector. I know that's currently what the inner vectors are called but after the refactor it won't really make sense to call it NullableIntVector because the inner vectors will be gone and all vectors are nullable right?

Yes, that's probably right. @jacques-n , what do you think?

icexelloss · 2017-10-11T21:35:56Z

java/vector/src/main/java/org/apache/arrow/vector/BaseNullableFixedWidthVector.java

If we are going to remove Mutator/Accessor, we should remove these function so it's compile time error rather than runtime error for the user.

Yes, for that we need to remove the generic Accessor, Mutator interface all together from ValueVector and its subinterfaces.

Since Legacy vectors are having mutator/accessor based access -- we may still need these temporary 2 functions getMutator() and getAccessor() just to comply with the interface until Legacy vectors have been removed.

icexelloss · 2017-10-11T21:41:11Z

java/vector/src/main/java/org/apache/arrow/vector/BitVectorHelper.java

This class has no public static methods?

Yes, only used in the java vector package to manipulate the validity buffer

I found a public util class that has no public methods to be weird. Maybe making this class package private if that's the intention?

Sounds good.

siddharthteotia · 2017-10-12T01:44:26Z

The patch finally builds fine :) Running existing tests now.

icexelloss · 2017-10-12T15:00:42Z

@siddharthteotia Can you create a branch and change this PR to be merged to that branch?

wesm · 2017-10-12T15:50:14Z

I created a java-vector-refactor branch for you to collaborate in https://git-wip-us.apache.org/repos/asf?p=arrow.git;a=shortlog;h=refs/heads/java-vector-refactor

icexelloss · 2017-10-12T16:25:43Z

Thanks Wes!

…

On Thu, Oct 12, 2017 at 11:50 AM Wes McKinney ***@***.***> wrote: I created a java-vector-refactor branch for you to collaborate in https://git-wip-us.apache.org/repos/asf?p=arrow.git;a=shortlog;h=refs/heads/java-vector-refactor — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1164 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAwbrN8s9m3b0w48sXOHPtQI8QvFE0QIks5srjU4gaJpZM4PuDJ7> .

Close #1164 Close #1198 Change-Id: If18e42d2edfdfef83e83621334a5b65a390e9db9

siddharthteotia · 2017-11-14T04:55:50Z

Closing this one.

Close apache#1164 Close apache#1198 Change-Id: If18e42d2edfdfef83e83621334a5b65a390e9db9

Close #1164 Close #1198 Change-Id: If18e42d2edfdfef83e83621334a5b65a390e9db9

Close apache#1164 Close apache#1198 Change-Id: If18e42d2edfdfef83e83621334a5b65a390e9db9

siddharthteotia force-pushed the ARROW-1473 branch from e90ccd9 to cfa07e5 Compare October 5, 2017 09:18

jacques-n reviewed Oct 5, 2017

View reviewed changes

BryanCutler reviewed Oct 5, 2017

View reviewed changes

elahrvivaz reviewed Oct 5, 2017

View reviewed changes

jacques-n reviewed Oct 5, 2017

View reviewed changes

siddharthteotia changed the title ~~ARROW-1473: [JAVA] initial prototype hierarchy~~ ARROW-1473: [JAVA - WIP] initial prototype hierarchy Oct 9, 2017

icexelloss reviewed Oct 11, 2017

View reviewed changes

siddharthteotia force-pushed the ARROW-1473 branch from dcf5ef3 to 27770fd Compare October 12, 2017 08:38

siddharthteotia mentioned this pull request Oct 13, 2017

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

Closed

ARROW-1473: ValueVector new hierarchy prototype (implementation phase 1)

94888e5

siddharthteotia force-pushed the ARROW-1473 branch from 88b89fe to 94888e5 Compare October 14, 2017 07:35

asfgit pushed a commit that referenced this pull request Oct 14, 2017

ARROW-1473: ValueVector new hierarchy prototype (implementation phase 1)

60a2ebd

Close #1164 Close #1198 Change-Id: If18e42d2edfdfef83e83621334a5b65a390e9db9

siddharthteotia changed the title ~~ARROW-1473: [JAVA - WIP] initial prototype hierarchy~~ ARROW-1463: [JAVA - WIP] refactoring Oct 14, 2017

siddharthteotia closed this Nov 14, 2017

siddharthteotia added a commit to siddharthteotia/arrow that referenced this pull request Nov 14, 2017

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

ARROW-1473: ValueVector new hierarchy prototype (implementation phase 1)

42353ba

Close #1164 Close #1198 Change-Id: If18e42d2edfdfef83e83621334a5b65a390e9db9

asfimport mentioned this pull request Jul 8, 2018

[JAVA] Restructure ValueVector hierarchy to minimize compile-time generated code #17483

Closed

18 tasks

pribor pushed a commit to GlobalWebIndex/arrow that referenced this pull request Oct 24, 2025

ARROW-1473: ValueVector new hierarchy prototype (implementation phase 1)

5c61741

Close apache#1164 Close apache#1198 Change-Id: If18e42d2edfdfef83e83621334a5b65a390e9db9

ARROW-1463: [JAVA - WIP] refactoring #1164

ARROW-1463: [JAVA - WIP] refactoring #1164

Uh oh!

Conversation

siddharthteotia commented Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

siddharthteotia commented Oct 5, 2017

Uh oh!

elahrvivaz commented Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

icexelloss commented Oct 5, 2017

Uh oh!

icexelloss commented Oct 5, 2017

Uh oh!

icexelloss commented Oct 5, 2017

Uh oh!

jacques-n commented Oct 5, 2017

Uh oh!

wesm commented Oct 5, 2017

Uh oh!

icexelloss commented Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacques-n commented Oct 5, 2017

Uh oh!

icexelloss commented Oct 5, 2017

Uh oh!

elahrvivaz commented Oct 5, 2017

Uh oh!

BryanCutler commented Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siddharthteotia commented Oct 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

siddharthteotia commented Oct 4, 2017 •

edited

Loading

elahrvivaz commented Oct 5, 2017 •

edited

Loading

icexelloss commented Oct 5, 2017 •

edited

Loading

BryanCutler commented Oct 5, 2017 •

edited

Loading

siddharthteotia commented Oct 9, 2017 •

edited

Loading

icexelloss Oct 11, 2017 •

edited

Loading