Hi Jacques,
> What problem are you trying to solve? It seems like you're proposing > refactoring for refactoring's sake. I think Liya Fan is trying to eliminate duplicate code, which as long as it doesn't violate design principles seems like a good thing. >From the PR [1]: > One of the fundamentals of vector design is single value level access > doesn't go through subclasses. This change violates that design criteria. Is there a design document someplace that enumerates the design fundamentals someplace and the rationale for them? I think it would help maintainers and contributors alike to understand the invariants the code is supposed to adhere to and why. Thanks, Micah [1] https://github.com/apache/arrow/pull/5213 On Wed, Aug 28, 2019 at 12:11 PM Jacques Nadeau <jacq...@apache.org> wrote: > What problem are you trying to solve? It seems like you're proposing > refactoring for refactoring's sake. > > On Mon, Aug 26, 2019, 7:13 PM Fan Liya <liya.fa...@gmail.com> wrote: > > > Hi Jacques, > > > > Thanks for the valuable feedback. > > > > I agree that it is a good idea to explicitly make field vectors final. > > > > However, I think there are still lots of duplicated code for > > getInt/getLong/setInt/setLong/setIntSafe/setLongSafe. In addition, we > have > > a large number of time related vector classes. > > > > So after making field vectors final, maybe we can: > > 1. Extract static utility methods for get/set int/long methods. > > 2. Push the get/set int/long methods to BaseFixedWidthVector base class. > > > > What do you think? > > > > Best, > > Liya Fan > > > > On Tue, Aug 27, 2019 at 7:16 AM Jacques Nadeau <jacq...@apache.org> > wrote: > > > > > I think you'd have to refractor differently since one of the patterns > is > > > get(index, object holder) is consistently matched. That means you > should > > > never subclass a field vector I believe. In fact we should probably > just > > > formally finalize them. > > > > > > Once you do that, I'm not sure how much duplicated code you're actually > > > removing. > > > > > > On Mon, Aug 26, 2019, 6:13 PM Fan Liya <liya.fa...@gmail.com> wrote: > > > > > > > Dear all, > > > > > > > > Currently, we have two classes of time related vectors. One class are > > > named > > > > TimeXXVector, while the other class are named TimeStampXXVector. We > > found > > > > there are some redundant code for these classes, so we want to do > some > > > > refactorings for them. > > > > > > > > 1. For TimeXXVector, all classes extend BaseFixedWidthVector. > > Internally, > > > > they are just IntVector or BigIntVector. The only differences are: > > > > > > > > 1) minor type > > > > 2) the result of getObject method. > > > > > > > > Proposed way of refactoring: we make them extend IntVector or > > > BigIntVector, > > > > so much get/set code for int/big int can be reused. > > > > > > > > 2. For TimeStampXXVector, all classes extend TimeStampVector. > > Internally, > > > > they are BigIntVectors. > > > > > > > > Proposed way of refactoring: we remove the TimeStampVector, and make > > all > > > > classes extend BigIntVector, so much of the get/set methods can be > > reused > > > > from super class. > > > > > > > > Please give your valuable feedback. > > > > > > > > Best, > > > > Liya Fan > > > > > > > > > >
