drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jinfeng Ni" <...@maprtech.com>
Subject Re: Review Request 33136: DRILL-1384: Rebase Drill's forked Optiq library onto Calcite release 1.0.
Date Tue, 21 Apr 2015 04:45:28 GMT

> 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:

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
> 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
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillProjectRelBase.java
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillRelOptUtil.java
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/cost/DrillCostBase.java
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillFilterJoinRules.java
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillProjectRel.java
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillPushProjIntoScan.java
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillReduceExpressionsRule.java
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillRuleSets.java
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/HashJoinPrel.java
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/CreateTableHandler.java
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/DefaultSqlHandler.java
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/ExplainHandler.java
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/SqlHandlerUtil.java
>   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,
> Thanks,
> Jinfeng Ni

  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message