Some context, for the benefit of others on the thread. The JIRA case is https://issues.apache.org/jira/browse/CALCITE-2128 <https://issues.apache.org/jira/browse/CALCITE-2128>, and Jonathan’s branch is https://github.com/shmushkis/calcite/tree/branch-1.15 <https://github.com/shmushkis/calcite/tree/branch-1.15>. We’re extending Calcite’s JDBC adapter to be able to connect to Jethrodata.
Jonathan, since you haven’t rebased I’m guessing that you still have test failures. Tomorrow I’ll review your change, and I’ll also rebase and squash, and run the system tests. At that point you must not make any further changes to your branch; it will be difficult to merge them. Julian > On Feb 11, 2018, at 5:07 AM, Jonathan Doron <[email protected]> wrote: > > Hi, > > Following our thread I have updated my branch according to your(Julian) > comments, I haven't touched the failing tests as they have already been > fixed by you, If its fine by you please push my branch to calcite. > > Thanks > > On Thu, Feb 8, 2018 at 10:54 PM, Julian Hyde <[email protected] > <mailto:[email protected]>> wrote: > >> >> Dev list is the Jira comment right? >> >> >> This is the dev list and how to subscribe: https://mail- >> archives.apache.org/mod_mbox/calcite-dev/. >> >> Re 1.As for now I am creating a JdbcConvention object per each jdbc table >> scan (in the CalcitePlanner.java), so actually I think it will not work as >> we wan and will not allow join and union of jethro tables from the same >> instance. >> >> >> Well fair enough, but if you want to push down joins and unions you will >> need to make sure that you create one JdbcConvention per Jethro instance. >> >> Re.2 Do you really think that exposing SUPPORTED_JETHRO_FUNCTIONS and >> JethrodataSqlDialec.getSupportedFunctions() is right? those are purely >> implementation details, I prefer to use @VisibleForTesting if you don't >> mind. >> >> >> People are not supposed to use the dialect constructor - they’re supposed >> to use the dialect factory - so adding a parameter is not a problem. I >> don’t think we need to make SUPPORTED_JETHRO_FUNCTIONS >> or JethrodataSqlDialect.getSupportedFunctions() public. I’m not a huge >> fan of @VisibleForTesting but a bigger problem is doing non-trivial things >> in a constructor. Also I don’t like static caches - they cause memory leaks. >> >> >> Re supportsFunction are you sure? because when quering the following sql >> query: >> "SELECT ib_income_band_sk, COUNT(ib_lower_bound) OVER (PARTITION BY >> ib_upper_bound) FROM ext_income_band;" >> which is a legit sql on other databases that hive, it generates a RexOver >> with a count function(), but since this a special case of count the Jethro >> does not support, I need to prevent from sending it to Jethro, so what do >> you suggest me to do? Because in a later state Jethro might implement it's >> own analytical function and I might want to push is to Jethro. >> >> >> OVER is not a function - it is an operator. It is too complex to model as >> a function. Suppose we wanted to check whether a database supports LIMIT - >> would we pretend that that was a function? I don’t think so. We’d add some >> other mechanism to the dialect to figure out which non-function features >> the database supports. >> >> You changed >> >>> boolean supportsFunction(SqlOperator operator, RelDataType type, List< >> RelDataType> paramsList) >> >> to >> >>> boolean supportsFunction(RexCall call) >> >> and I don’t agree with that change. SqlDialect does not depend upon >> anything in the org.apache.calcite.rex package and I want to keep it that >> way. >> >> >> " >> from JdbcSortRule you've half-fixed https://issues.apac >> he.org/jira/browse/CALCITE-1265. >> <https://issues.apache.org/jira/browse/CALCITE-1265 >> <https://issues.apache.org/jira/browse/CALCITE-1265>> Can you fixed the >> Hsqldb dialect for the SQL so generated. Also gather together the changes >> into a single commit that fixes CALCITE-1265. >> " >> >> Is that a question? explanation? >> >> >> I’m asking you to re-organize your code into commits that fix specific >> problems. And also pointing out that your change broke those tests, so you >> need to fix them. >> >> As it happens I’ve already committed a fix for CALCITE-1265, and my >> https://github.com/julianhyde/calcite/commits/2128-jethro branch contains >> your changes re-based onto that fix. So I’ve probably already done what I >> was asking you to do. >> >> Julian
