Mailing-List: contact dev-help@impala.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@impala.apache.org
MIME-Version: 1.0
In-Reply-To: <CAKrh3XQDf-=BxOC-wuepUJ3L30Vo9oEjs7Dx-JOK-g_eVU8W8w@mail.gmail.com>
References: <CAKrh3XQc2wHdhiNouA=kfRjcfTd+kU7CeMr7wk-hV4K78jgVqg@mail.gmail.com>
 <CAC-pSX0S2xuN-hLpQ5UBKvqdEBxLjowPQTYDdSZvPJnOns=tYQ@mail.gmail.com> <CAKrh3XQDf-=BxOC-wuepUJ3L30Vo9oEjs7Dx-JOK-g_eVU8W8w@mail.gmail.com>
From: Jim Apple <jbapple@cloudera.com>
Date: Thu, 16 Nov 2017 09:10:53 -0800
Message-ID: <CAC-pSX2R3wEin0dYktVtBSKhVkjrD85yQOmof10A+m2Sz8TAkA@mail.gmail.com>
Subject: Re: IMPALA-5078: break up expr-test.cc
To: dev@impala.apache.org
Content-Type: text/plain; charset="UTF-8"
archived-at: Thu, 16 Nov 2017 17:11:48 -0000

You might even consider doing #4 first - it's OK to have a file that
is only used by one other file, and that way would reduce the burden
to anyone who needs to make a change to a utility file, so they don't
have to make that change in multiple places.

On Thu, Nov 16, 2017 at 7:47 AM, Jin Chul Kim <jinchul@gmail.com> wrote:
> Sure. Here is a plan to do this carefully.
>
> 1. The first change is to move only string part to test-string-expr.cc,
> which just move/copy a chunk of code to a new file. There is a redundant
> code in the both files between test-expr.cc and test-string-expr.cc because
> we hope minimal code change.
> 2. Iterate #1 approach for the remaining parts. There are several changes.
> 3. Splitting test-expr.cc is finished.
> 4. We can find rooms to refactor the new files because there are
> redundant/unnecessary code. Several changes are required for refactoring by
> an incremental manner.
>
> Best regards,
> Jinchul
>
> 2017-11-16 23:51 GMT+09:00 Jim Apple <jbapple@cloudera.com>:
>
>> I like this idea.
>>
>> One thing to be careful of is to not modify the tests themselves when
>> you move them. It's hard to see such changes in gerrit and it's hard
>> to track them down in git history.
>>
>> On Thu, Nov 16, 2017 at 5:34 AM, Jin Chul Kim <jinchul@gmail.com> wrote:
>> > Hi,
>> >
>> > https://issues.apache.org/jira/browse/IMPALA-5078
>> >
>> > I'd like to get your advice if I will refactor expr-test.cc. Henry
>> suggests
>> > grouping tests by string instructions and move them to
>> expr-string-test.cc.
>> > I like his idea. You know filenames (e.g. cast-functions*,
>> > timestamp-functions*, aggregate-functions*) of backend kernel code in
>> exprs
>> > have a same pattern, so I would suggest a pair of kernel code and unit
>> > test. For example,
>> > (cast-functions*, expr-cast-test.cc),
>> > (timestamp-functions*, expr-timestamp-test.cc),
>> > (aggregate-functions*, expr-aggregate-test.cc),
>> > ...
>> >
>> > Do you have any suggestion or comment on this? Thanks.
>> >
>> > Best regards,
>> > Jinchul
>>