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 <[email protected]> wrote: > > > On Nov 16, 2014, at 10:45 PM, Vladimir Sitnikov < > [email protected]> 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 > >
