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/53b4b09606de5fe14f4 > >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 > >> > > >> > >> > >
