You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@impala.apache.org by Jin Chul Kim <ji...@gmail.com> on 2017/11/16 13:34:01 UTC
IMPALA-5078: break up expr-test.cc
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
Re: IMPALA-5078: break up expr-test.cc
Posted by Jim Apple <jb...@cloudera.com>.
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 <ji...@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 <jb...@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 <ji...@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
>>
Re: IMPALA-5078: break up expr-test.cc
Posted by Jin Chul Kim <ji...@gmail.com>.
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 <jb...@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 <ji...@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
>
Re: IMPALA-5078: break up expr-test.cc
Posted by Jim Apple <jb...@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 <ji...@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