I have created CALCITE-738 to follow-up on the tests issue. Jesús
On 5/19/15, 1:49 AM, "Julian Hyde" <[email protected]> wrote: >Wow, you're right. It's not used anywhere in Calcite. While I'd prefer >that >everything is tested, since it's not used, adding a test before the >release >is low priority. > >The easiest way to write a test is probably to follow the model of >RexProgramTest. Create RexNodes, apply methods, test the results by >applying toString or whatever. > >Better would be to look over the Hive code that uses splitJoinCondition >(it >seems to be called from HiveCalciteUtil and CalcitePlanner) and figure out >whether any of it could/should be in Calcite, packaged for re-use (e.g. as >a rule) and tested. There is a benefit to having well-tested library code >but I recognize there is also a cost, so I'll leave it to you guys. > >Julian > > >On Mon, May 18, 2015 at 2:40 PM, Jesus Camachorodriguez < >[email protected]> wrote: > >> I just checked it back and the problem is that the util function >> splitCondition is not called in join.oq (neither from Calcite itself). >> >> What do you think? Should I create a new unit test specific to that >> function? What would be the best place to do that? >> >> Jesús >> >> >> >> >> >> On 5/18/15, 8:02 PM, "Julian Hyde" <[email protected]> wrote: >> >> >I've rebased your change and committed to my master branch[1] for >>testing. >> >I will go ahead and commit to apache master in about 24 hours. If you >> >manage to create a better test case before then I will include it. But >>if >> >not I will still include your fix in the release. >> > >> >Julian >> > >> >[1] >> > >> >>https://github.com/julianhyde/incubator-calcite/commit/53b4b09606de5fe14f >>4 >> >9e65a6d5b4346ec930a62 >> > >> >On Mon, May 18, 2015 at 11:25 AM, Julian Hyde <[email protected]> >> >wrote: >> > >> >> I¹m not sure that your test case demonstrates the problem. I tried >>the >> >> test case (join.oq) without your code change (RelOptUtil.java) and it >> >> passed. >> >> >> >> On May 18, 2015, at 6:19 AM, Jesus Camachorodriguez < >> >> [email protected]> wrote: >> >> >> >> > I have created a new pull request with the test case for >>CALCITE-688. >> >> > >> >> > Thanks, >> >> > Jesús >> >> > >> >> > On 5/17/15, 7:12 AM, "Julian Hyde" <[email protected]> wrote: >> >> > >> >> >> I'm close to making the first release candidate. The issues we >>saw in >> >> Hive >> >> >> have been cleared up. >> >> >> >> >> >> I'm going to commit >> >>https://github.com/apache/incubator-calcite/pull/86 >> >> >> and >> >> >> https://github.com/apache/incubator-calcite/pull/79 (both just >>doc, >> >>so >> >> >> won't impact stability). >> >> >> >> >> >> And I'll need to write release notes. >> >> >> >> >> >> Jesus, I'd like to commit >> >> >> https://github.com/apache/incubator-calcite/pull/78 but I need a >> test >> >> >> case. >> >> >> In join.oq would be the best place. >> >> >> >> >> >> I'll look at https://issues.apache.org/jira/browse/CALCITE-259, >> >> >> https://issues.apache.org/jira/browse/CALCITE-712 and commit them >>if >> >> >> they're ready. >> >> >> >> >> >> Other pull requests >> https://github.com/apache/incubator-calcite/pulls >> >> or >> >> >> jira cases tagged "next"[1] don't look ready. Correct me if I've >> >>missed >> >> >> something. >> >> >> >> >> >> Julian >> >> >> >> >> >> [1] >> >> >> >> >> >> >> >> >>https://issues.apache.org/jira/issues/?jql=project%20%3D%20CALCITE%20AND% >> >>2 >> >> >> 0fixVersion%20%3D%20%22next%22%20 >> >> > >> >> >> >> >> >>
