Mailing-List: contact dev-help@drill.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@drill.apache.org
Content-Type: multipart/alternative;
 boundary="===============7948506350301727033=="
MIME-Version: 1.0
Subject: Re: Review Request 33136: DRILL-1384: Rebase Drill's forked Optiq
 library onto Calcite release 1.0.
From: "Jinfeng Ni" <jni@maprtech.com>
To: "Aman Sinha" <asinha@maprtech.com>
Cc: "drill" <drill-dev@incubator.apache.org>, "Jinfeng Ni" <jni@maprtech.com>
Date: Sun, 19 Apr 2015 17:11:39 -0000
Message-ID: <20150419171139.2948.17927@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Jinfeng Ni" <noreply@reviews.apache.org>
References: <20150418231033.1422.40104@reviews.apache.org>
In-Reply-To: <20150418231033.1422.40104@reviews.apache.org>
Reply-To: "Jinfeng Ni" <jni@maprtech.com>

--===============7948506350301727033==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


> On April 18, 2015, 4:10 p.m., Aman Sinha wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/planner/cost/DrillCostBase.java, line 61
> > <https://reviews.apache.org/r/33136/diff/3/?file=934734#file934734line61>
> >
> >     Is this factor actually helping with any of the plans ? If not, we should skip it since having more factors makes it difficult to estimate the cost.

This is used when in DrillCostBase we compare the total cost of cpu/io/network/memory of two different plans. Previously, memory cost is excluded, while in HashAgg/HashJoin/StreamAgg, the memory cost is one important factor when choose a plan. For io/network, seems we calculated in terms of # of instruction cycles, yet memory cost is calcuted propotionly to bytes. That's why we need this ratio, when combine them together. I understand this ratio for now is just a magic number, requiring further tuning in the near future.


> On April 18, 2015, 4:10 p.m., Aman Sinha wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillRuleSets.java, line 123
> > <https://reviews.apache.org/r/33136/diff/3/?file=934739#file934739line123>
> >
> >     Remove all the commented out old rule names that are not valid in the master branch of Calcite since it adds confusion.

I'll remove. Originally, I kept them, so that we know what is the prior rule name, before the rebasing work, since we're more faimilar with the old rule name.


> On April 18, 2015, 4:10 p.m., Aman Sinha wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/HashJoinPrel.java, line 78
> > <https://reviews.apache.org/r/33136/diff/3/?file=934740#file934740line78>
> >
> >     Since this logic has moved to DrillJoinRelBase, you should just remove it from here.

remove.


- Jinfeng


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33136/#review80617
-----------------------------------------------------------


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


--===============7948506350301727033==--