Helping with CALCITE-466

[Jacques Nadeau <ja...@apache.org>]

Hey Julian,

I see you're working on the package reorganization /modularization.  Want
to meet up this week and we can pair tackle it for a day? I'm more than
willing to drop by and lend a hand.

Jacques

Re: Helping with CALCITE-466

[Jacques Nadeau <ja...@apache.org>]

I agree that this is non-trivial.  That is part of the reason I think pair
programming to a solution will be helpful.  I also believe that this
separation of execution and planning is critical for the library.

We need to make sure the test coverage covers how people use the library.
I know of at least two big systems that use Calcite outside the execution
part, Hive & Drill.  I'm sure there are others as well.  It is critically
important that the core code is tested for this use case.  If we include
execution in that module, it means that we risk those APIs not being
rigidly set and well defined in function.  It also makes it much more
difficult to create new test cases if there aren't well defined paradigms
to prove things.  (See nearly every patch the Drill team has brought to the
Calcite community.)

To this point, I'm arguing that there should be ZERO execution code in the
core (or super-core if we want to maintain the existing core coordinates
for everyone's ease of use.  We also discussed actually making the parser a
separate module since Hive doesn't even use that.  I'm more neutral on
whether that should be separated.

The other alternative, which I think is much more difficult/worse, is
adding pre-commit testing of external frameworks use of the library as a
requirement to commit.

Jacques



On Sun, Nov 16, 2014 at 11:17 PM, Julian Hyde <ju...@hydromatic.net> wrote:

>
> > On Nov 16, 2014, at 10:45 PM, Vladimir Sitnikov <
> sitnikov.vladimir@gmail.com> wrote:
> >
> > Re 'integration tests',
> > You'll need to double test set if you want to make sure interpreter
> returns
> > proper values.
> > Otherwise it might silently return wrong data.
>
> No. A bit more testing (say, executing each adapter on 100 tricky queries)
> but not double.
>
> > I do not see how execution of integration tests in a separate module
> lowers
> > the test coverage. On contrary, it makes clear if it is possible to
> create
> > out-of-core executor.
>
> If it doesn't matter which module code lives in, why is Jacques arguing to
> move enumerable adapter out of core?
>
> IIRC Jacques' argument for moving the enumerable adapter out of core was
> that it imposes a burden on people adding new operation to core, and that
> they shouldn't have to implement that operation in the enumerable adapter.
>
> But adding an operation to core should not have zero burden. You should
> add tests for the units that it touches (parser, validator etc.) And you
> should add to the reference implementation (since we use a test suite in
> lieu of a detailed spec).
>
> I think that if tests were moved out of core they would be run less often,
> and added to less often. Since test running time is not a problem, there's
> no reason to move the tests.
>
> > In the mean time I allmost got my EnumerableCorrelate working, so I hope
> > you won't break much if you move things around.
>
> We'll try not to. See if you can get a patch submitted before we start
> working on it.
>
> Julian
>
>

Re: Helping with CALCITE-466

[Julian Hyde <ju...@hydromatic.net>]

> On Nov 16, 2014, at 10:45 PM, Vladimir Sitnikov <si...@gmail.com> wrote:
> 
> Re 'integration tests',
> You'll need to double test set if you want to make sure interpreter returns
> proper values.
> Otherwise it might silently return wrong data.

No. A bit more testing (say, executing each adapter on 100 tricky queries) but not double.

> I do not see how execution of integration tests in a separate module lowers
> the test coverage. On contrary, it makes clear if it is possible to create
> out-of-core executor.

If it doesn't matter which module code lives in, why is Jacques arguing to move enumerable adapter out of core?

IIRC Jacques' argument for moving the enumerable adapter out of core was that it imposes a burden on people adding new operation to core, and that they shouldn't have to implement that operation in the enumerable adapter.

But adding an operation to core should not have zero burden. You should add tests for the units that it touches (parser, validator etc.) And you should add to the reference implementation (since we use a test suite in lieu of a detailed spec).

I think that if tests were moved out of core they would be run less often, and added to less often. Since test running time is not a problem, there's no reason to move the tests.

> In the mean time I allmost got my EnumerableCorrelate working, so I hope
> you won't break much if you move things around.

We'll try not to. See if you can get a patch submitted before we start working on it.

Julian

Re: Helping with CALCITE-466

[Vladimir Sitnikov <si...@gmail.com>]

Re 'integration tests',
You'll need to double test set if you want to make sure interpreter returns
proper values.
Otherwise it might silently return wrong data.

I do not see how execution of integration tests in a separate module lowers
the test coverage. On contrary, it makes clear if it is possible to create
out-of-core executor.

In the mean time I allmost got my EnumerableCorrelate working, so I hope
you won't break much if you move things around.

Regards,
Vladimir Sitnikov
17 нояб. 2014 г. 8:27 пользователь "Julian Hyde" <ju...@gmail.com>
написал:

> Jacques,
>
> I appreciate the offer, and I'd like to do it. I'm tied up this week, but
> free Mon and Tue next week, though. (Contact me back-channel and we can
> schedule.)
>
> Moving org.apache.calcite.adapter.enumerable into its own module presents
> some interesting challenges.
>
> 1. Quite a few other modules (e.g. mongo) depend on it. We could let them
> continue to do that, or we could migrate them to the new ScannableTable SPI.
>
> 2. A lot of tests depend on enumerable, but they are not just tests of the
> enumerable adapter; they are integration tests for the whole system. If we
> move these into the enumerable adapter we lose a lot of coverage, and I
> can't accept that. I think we should leave a basic query execution
> capability in core, and implementations of the operators and a few
> functions via the interpreter.
>
> These are not unrelated -- if a table implements ScannableTable then
> Calcite will fire up an interpreter to read from it.
>
> Julian
>
>
> > On Nov 15, 2014, at 3:05 PM, Jacques Nadeau <ja...@apache.org> wrote:
> >
> > Hey Julian,
> >
> > I see you're working on the package reorganization /modularization.  Want
> > to meet up this week and we can pair tackle it for a day? I'm more than
> > willing to drop by and lend a hand.
> >
> > Jacques
>
>

Re: Helping with CALCITE-466

[Julian Hyde <ju...@gmail.com>]

Jacques,

I appreciate the offer, and I'd like to do it. I'm tied up this week, but free Mon and Tue next week, though. (Contact me back-channel and we can schedule.)

Moving org.apache.calcite.adapter.enumerable into its own module presents some interesting challenges.

1. Quite a few other modules (e.g. mongo) depend on it. We could let them continue to do that, or we could migrate them to the new ScannableTable SPI.

2. A lot of tests depend on enumerable, but they are not just tests of the enumerable adapter; they are integration tests for the whole system. If we move these into the enumerable adapter we lose a lot of coverage, and I can't accept that. I think we should leave a basic query execution capability in core, and implementations of the operators and a few functions via the interpreter. 

These are not unrelated -- if a table implements ScannableTable then Calcite will fire up an interpreter to read from it.

Julian


> On Nov 15, 2014, at 3:05 PM, Jacques Nadeau <ja...@apache.org> wrote:
> 
> Hey Julian,
> 
> I see you're working on the package reorganization /modularization.  Want
> to meet up this week and we can pair tackle it for a day? I'm more than
> willing to drop by and lend a hand.
> 
> Jacques