> On April 20, 2015, 9:27 p.m., Julian Hyde wrote: > > Difficult to tell the rationale behind each change, but all of the changes > > seem to make sense. > > > > I noticed that you had to move to the version of > > ProjectRemoveRule.isTrivial with a boolean parameter. That was temporary, > > and was removed shortly afterwards. See CALCITE-575 and CALCITE-577. > > Unavoidable, I suppose. > > > > At a later point, consider renaming Drill's RelNodes and Rules to match > > Calcite's new naming scheme. Strictly optional of course.
That's right. ProjectRemoveRule.isTrivial used in Drill is a deprecated version. The reason we use that version is because Drill's execution uses name-based logic, different from Calcite's ordinal-based executiion. A project with identical expressions, yet different names should not be removed in Drill's plan; otherwise, the query may not return correct result. We probably have to re-visit this deprecated code issue, when we keep going on with Calcite's new code. Agree that we should renaming Drill's Rules to match Calcite's new naming scheme. I'll do that in a follow-up patch. This patch is mainly focus on not causing any regression to the existing test workloads. - Jinfeng ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33136/#review80902 ----------------------------------------------------------- On April 18, 2015, 1:34 p.m., Jinfeng Ni wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33136/ > ----------------------------------------------------------- > > (Updated April 18, 2015, 1:34 p.m.) > > > Review request for drill and Aman Sinha. > > > Repository: drill-git > > > Description > ------- > > Drill currently uses a forked Optiq (Renamed to Calcite) version, dated back > in July 2014. The forked version has 10-20 Drill specific patches. However, > we did not rebase the forked version onto the on-going Calcite release. As > such, Drill misses some bug fixes/new feature development on Calcite side. > > This patch is trying to rebase the Drill's forked version onto Calcite > release 1.0. (More precisely, on commit of CALCITE-603, when this rebasing > work started). > > Most of changes happen due to Calcite's package structure / renaming (See > CALCITE-296]. > > Some Drill specific changes: > > 1. Provide a Drill specifc RelDataTypeSystem, to support decimal with > precision/scale up to 38. > 2. Modify Drill's parser, to allow * in Compound Identifier. > 3. Provide Drill specific FilterJoinRule, to enforce Drill only support > equal-join in JOIN operator. > 4. Modify Drill costing comparison, such that the costing oder is a total > order when compare different plans. > 5. Modify costing estimation for Drill Project operator. > 6. Use a ProjectRemove rule, such that it will honor parent's output field > name. > 7. Modify Calcite's Frameworks/planner interface, such that Drill will use > validatedRowType to construct a top-level project, to ensure the final output > field is what the query specified. (Calcite could inject "$F0", or "$EXPR0 > into converted RelNode tree, in Sql2RelConverter) > 8. Fix couple of Drill unit test cases, since the expected result by query > semantics are not fixed. > 9. Some type-related to Drill Sql operators. > > Some impact of such rebasing. > 1. TPCH Q16, or query with NOT IN predicate involving NULLABLE column could > hit CanNotPlanException. > > The previous plan for Q16, although return the correct result, is not valid, > when the column is nullable. See DRILL-1957 OR CALCITE-373. > > 2. Plan changed in some TPCH queries, which may cause timeout for Q5 in TPCH > scale factor 100 run. > We probably need continue refine the costing estimation formula, especially > for Join operator. > > > Diffs > ----- > > common/pom.xml 525b533 > contrib/storage-hive/core/pom.xml 9bd6293 > exec/java-exec/pom.xml f5313ca > exec/java-exec/src/main/codegen/includes/compoundIdentifier.ftl 50d8c20 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillJoinRelBase.java > 3b3aa1a > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillProjectRelBase.java > 7cf98cd > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillRelOptUtil.java > bbe7cf3 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/cost/DrillCostBase.java > 87a1ea3 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillFilterJoinRules.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillProjectRel.java > 14ea894 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillPushProjIntoScan.java > fcfced2 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillReduceExpressionsRule.java > 29175e5 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillRuleSets.java > 84a0b51 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/HashJoinPrel.java > f63057f > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java > 7bd48c8 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/CreateTableHandler.java > a17a604 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/DefaultSqlHandler.java > 5ee502d > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/ExplainHandler.java > 1cce6a5 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/SqlHandlerUtil.java > c347bef > exec/jdbc-all/pom.xml b369aed > exec/jdbc/pom.xml b4ec758 > pom.xml f6bcd91 > > Diff: https://reviews.apache.org/r/33136/diff/ > > > Testing > ------- > > Unit testcase clean run. (except for disabled testcases. See DRILL-2630, > DRILL-2761, etc) > > > Thanks, > > Jinfeng Ni > >
