On Wed, Aug 28, 2019, 7:42 PM Micah Kornfield <emkornfi...@gmail.com> wrote:
> 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. > I both agree and disagree. Change always introduce risk. With highly used code, the bar for cosmetic change needs to be higher. If the maintenance burden is high than it can make more sense. In this case, this seems like cosmetic change on something that changes rarely and thus has a low burden of maintebabce. Add to this that we just refactored all of this code, among other things trying hard to minimize complex class hierarchies. During that time we tried several different class hierarchies before settling on this one. > 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. Yeah, this is an excellent point. When these things are in someone's head (mine in this case), it can feel very arbtrirary to others. I think a bunch of this was covered extensively on the list and in the reviews during the major refractor done last year. I'll also see if I can dig up some more background. > 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 > > > > > > > > > > > > > > >
