impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jim Apple <jbap...@cloudera.com>
Subject Re: IMPALA-5078: break up expr-test.cc
Date Thu, 16 Nov 2017 17:10:53 GMT
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
>>

Mime
View raw message