drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Aman Sinha" <asi...@maprtech.com>
Subject Re: Review Request 21371: Patch for DRILL-705
Date Thu, 15 May 2014 18:41:33 GMT

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


I took a first pass at this...sending out initial review comments. 


common/src/main/java/org/apache/drill/common/logical/data/Window.java
<https://reviews.apache.org/r/21371/#comment77138>

    Is 'Within' implying the OVER clause ?  We should support empty OVER clause .. for example:
SELECT SUM(a1) OVER ().  Basically, no PARTITION BY or ORDER BY is specified, so I want to
do the aggregate over entire data set. 



exec/java-exec/src/main/codegen/data/AggrTypes1.tdd
<https://reviews.apache.org/r/21371/#comment77137>

    It isn't clear why a separate class called Sum0 is needed ..is the existing Sum not sufficient
? If it is needed for Sum, what about other aggregate functions ?



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/WindowFrameRecordBatch.java
<https://reviews.apache.org/r/21371/#comment77140>

    Don't you need a next() interface in the WindowFrameRecordBatch ? If a downstream operator
calls next() on the incoming, how does this class respond ? Note that normally we have the
doWork() in the template class, not in the record batch class.   



exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/WindowPrel.java
<https://reviews.apache.org/r/21371/#comment77154>

    It sounds like a single physical WindowPOP is being created for a list of logical Windows.
Suppose there are 2 Window Functions: 
      SUM(a1) OVER (PARTITION BY b1), 
      SUM(a1) OVER (PARTITION BY c1)
    
    then, I believe Optiq would have created 2 logical window rels since the two Partition-By
 clauses are not compatible with each other. So, we will need two separate physical window
operators, right ? 
    
    On the other hand, suppose the 2 window functions are: 
      RANK() OVER (ORDER BY a1, b1), 
      RANK() OVER (ORDER BY a1)
    
    then we could do both in a single physical Window operator by doing ordering only once
on (a1, b1).  



exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/WindowPrule.java
<https://reviews.apache.org/r/21371/#comment77149>

    This syntax of embedding a function within a function makes it hard to follow the overall
logic. I am not too familiar with this pattern...so maybe someone else can comment on it..


- Aman Sinha


On May 13, 2014, 9:01 p.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21371/
> -----------------------------------------------------------
> 
> (Updated May 13, 2014, 9:01 p.m.)
> 
> 
> Review request for drill.
> 
> 
> Bugs: DRILL-705
>     https://issues.apache.org/jira/browse/DRILL-705
> 
> 
> Repository: drill-git
> 
> 
> Description
> -------
> 
> Currently only supports partitioning/ordering, not yet preceding or after offsets.
> 
> This patch also addresses the CASE syntax bug (DRILL-665)
> 
> Also fixed the Drill review patch tool.
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/drill/common/expression/CastExpression.java 7e5eea096b53ede676abb4764631fd92805b4ad1

>   common/src/main/java/org/apache/drill/common/expression/ExpressionStringBuilder.java
95888639f1f1de232c4df8b4baa38e7ca3d12ffb 
>   common/src/main/java/org/apache/drill/common/expression/IfExpression.java 280952dac4552b000ce05bc8922f8f82e979a4b0

>   common/src/main/java/org/apache/drill/common/expression/TypedNullConstant.java 1efb029224c759caf7da0260704ad7756b1d521d

>   common/src/main/java/org/apache/drill/common/expression/visitors/AbstractExprVisitor.java
526275fc88d8c90a254ecbeb76343f37ec4f0695 
>   common/src/main/java/org/apache/drill/common/expression/visitors/AggregateChecker.java
bf67a6bd27f34d3dfb7e256b581238f0c4f531b6 
>   common/src/main/java/org/apache/drill/common/expression/visitors/ConstantChecker.java
ab94987811d6c988a55525ac8b8e041e11034f77 
>   common/src/main/java/org/apache/drill/common/expression/visitors/ExprVisitor.java 799c9ddfed42e986e9ccc5235ef73ff13f7dfaff

>   common/src/main/java/org/apache/drill/common/expression/visitors/ExpressionValidator.java
e9bd03a7f9154b739847f9bf76510deefe847d82 
>   common/src/main/java/org/apache/drill/common/logical/data/AbstractBuilder.java 69cb4eb73ab71c6424752a1027ee190ee95104c4

>   common/src/main/java/org/apache/drill/common/logical/data/AbstractSingleBuilder.java
e733fddc554f462dadcc96e21db29191000f6ea1 
>   common/src/main/java/org/apache/drill/common/logical/data/Limit.java 110204be8bac10b960013d8cc52d552c50f15995

>   common/src/main/java/org/apache/drill/common/logical/data/LogicalOperator.java 531e6a6244b110a845833c5d94b2077df1f26460

>   common/src/main/java/org/apache/drill/common/logical/data/LogicalOperatorBase.java
69a1c3c5e7e9911fc6fe1cccb16f8e6fb50dc293 
>   common/src/main/java/org/apache/drill/common/logical/data/NamedExpression.java 4c006c609e281a03607101fb83ae368bd4e43200

>   common/src/main/java/org/apache/drill/common/logical/data/SingleInputOperator.java
0a5015c15a1e9a9b6c3b092ca644ec425d142957 
>   common/src/main/java/org/apache/drill/common/logical/data/Window.java PRE-CREATION

>   common/src/main/java/org/apache/drill/common/logical/data/WindowFrame.java 09524061601209bd0d322fabade3df4ebdcd07bd

>   common/src/main/java/org/apache/drill/common/logical/data/visitors/AbstractLogicalVisitor.java
0099bb9a8a29f6c7e23ff7f45a80a1d714cb3ddd 
>   common/src/main/java/org/apache/drill/common/logical/data/visitors/LogicalVisitor.java
4bf9fbfcaac21ab077ce26993575f679010bf592 
>   exec/java-exec/src/main/codegen/data/AggrTypes1.tdd 71931dfbadde2f3981746e61d9ad40baf3c4890b

>   exec/java-exec/src/main/codegen/templates/AggrTypeFunctions1.java aa9aeab7af7d387cff9da57af69d3c9a5a9498fd

>   exec/java-exec/src/main/codegen/templates/TypeHelper.java 4755e9210728967f17fbeddd2ef8b0d6c501d641

>   exec/java-exec/src/main/java/org/apache/drill/exec/compile/sig/ConstantExpressionIdentifier.java
c0c8484005374ab273f5129e9f18afba0d2cb4f3 
>   exec/java-exec/src/main/java/org/apache/drill/exec/expr/EvaluationVisitor.java d700bf3f045b21c8d15b0841f6568b39574c227d

>   exec/java-exec/src/main/java/org/apache/drill/exec/expr/ExpressionTreeMaterializer.java
0267be3ad54edf3e4da18582ffa8c83f7d8864fc 
>   exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionConverter.java 44210914d1b6f819716b366020d116c89e92587f

>   exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/HiveFunctionImplementationRegistry.java
e5c890eb81320ecb3affe67b74e900636296c480 
>   exec/java-exec/src/main/java/org/apache/drill/exec/memory/Accountor.java 624042e03fb8f7fc980405597266044fea832e41

>   exec/java-exec/src/main/java/org/apache/drill/exec/opt/BasicOptimizer.java 78c0e3cb9ec90de70fbf1e036d13f0853af4f8ad

>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractPhysicalVisitor.java
01071012a9aebc5861dde6c4a2306efd3319cfbf 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/PhysicalVisitor.java
7a1440a4b2d7321efa086cc07f2a45468fa8a733 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/Limit.java b926e3ed7839fad6c107476f7c8ca9c2bbe0be1d

>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/MergeJoinPOP.java
5bb378a8bed208e290b5c0d0437c066e0e5e1251 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/WindowPOP.java PRE-CREATION

>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/aggregate/InternalBatch.java
3e6def128034ccfce2dab38cef1ccff61553a104 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/filter/FilterRecordBatch.java
566dfe0aaf8ea87584c6fcf14613ed745c805b6d 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/limit/LimitBatchCreator.java
ccbf755faa8f1332df68e34bee130c7a27f503e0 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/limit/LimitRecordBatch.java
ed56e796b84d713e697296017056e590af37169d 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/project/ProjectRecordBatch.java
b94f403731609ffb57574c32ffe299a0172ea91c 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/svremover/RemovingRecordBatch.java
62af0b2a395d7b61ed334761f0e878f6affdc9d6 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/trace/TraceRecordBatch.java
b012cec1d8a4d81ab6508de76147562fc2f9f75e 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/WindowFrameBatchCreator.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/WindowFrameRecordBatch.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/WindowFrameTemplate.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/WindowFramer.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillWindowRelBase.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/StatsCollector.java
6d720a7d1d3afba72109230f8642e8c7cc3b60f5 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillAggregateRel.java
fe5130ced1acfbe548b3af9b42d0cb80b7840faa 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillJoinRel.java
a5593e762256c89cd6659c9dff5b3d9171f5555b 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillLimitRule.java
c3b0d00c6bddf52cb88307a49df2574b720a4806 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillOptiq.java
7efd7144153310f86b78e8b85c606f172b0ff672 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillPushProjIntoScan.java
0dd9b9ebba42f63db594e36564be47c2665870ee 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillRel.java 7eca54e9b9a79f63d0b9df1bebc7400f46996028

>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillRuleSets.java
c07fee35d3895b6d7bfbbd30a108d58a9f6abb60 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillWindowRel.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillWindowRule.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/LimitPrel.java
7c8d7672cbc147fee793b0ed4d59cbfcd1136322 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/WindowPrel.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/WindowPrule.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillOperatorTable.java
772b3b9322b58f2cfdc84ee73c3717caac3fad4b 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlAggOperator.java
b074ba0d5ae874ff663210f9fb873eb9b7f2a1cd 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java
747744026bc795d6ae6af2f98ee4af1937a8cf6e 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/CreateTableHandler.java
b7d9bd7f3e70a8754f9ae6a56986c4ea7f99e432 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/DefaultSqlHandler.java
d107c29c0ef80426ec90ecd6e761a57cbf995455 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/DescribeTableHandler.java
d6849f4cab0084a282945deb661b0cdc3f545d5b 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/ExplainHandler.java
86ce6c56cfd0eda4f1e0f2e326eabd785c2643b2 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/ShowFileHandler.java
4f7c424b72cbee48439370df681cb810c835c18b 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/ShowSchemasHandler.java
2a6dc6a33e679e1ff7f657727f2bc4628ef984e3 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/ShowTablesHandler.java
6905c1773ffbbe1a97d92201ee06aa9e5ac4ed39 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/DrillSqlCall.java
5fa592a13c0e8e12f04121e2302339d48189ad63 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/SqlCreateTable.java
7be40cbe932efcf73586575ce611c4ce694308cb 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/SqlCreateView.java
b362a20e149e7db5bf0687b970387f3dc160666c 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/SqlDescribeTable.java
30c7e433bef7f82794200026de17a05fca0009cf 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/SqlDropView.java
bbb3a7f81f92c02410c4c879b357b02f3cf418af 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/SqlShowFiles.java
f882ba9652528f357a2714952c18415d6eaa04ba 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/SqlShowSchemas.java
34695d950bc6b5e4b6369e91cc854f4580816d51 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/SqlShowTables.java
26d4fa216334d128ab92fba7dddd5868443a9424 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/SqlUseSchema.java
42a3914a7d103e4ccf04afd246bdc01e06aa683a 
>   exec/java-exec/src/main/java/org/apache/drill/exec/record/AbstractSingleRecordBatch.java
dd2cfe059a191f223509f0b7cd8e8c6c8c097e89 
>   exec/java-exec/src/main/java/org/apache/drill/exec/record/NullExpression.java 15435504cd1aa5953c423192b6dc6b5c4794d1c5

>   exec/java-exec/src/main/java/org/apache/drill/exec/service/ServiceEngine.java cfbde739511af4f96649479fe26677c05f86d064

>   exec/java-exec/src/test/java/org/apache/drill/exec/ExecTest.java 7be8cc5f7498980a06e50dadd42105537fc4fa84

>   exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestSimpleFragmentRun.java
842aa8fa66aff7c2a704f377af1fccca7534e167 
>   exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestUtils.java PRE-CREATION

>   exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/window/TestWindowFrame.java
PRE-CREATION 
>   exec/java-exec/src/test/resources/window/mediumData.json PRE-CREATION 
>   exec/java-exec/src/test/resources/window/oneKeyCount.json PRE-CREATION 
>   exec/java-exec/src/test/resources/window/oneKeyCountData.json PRE-CREATION 
>   exec/java-exec/src/test/resources/window/oneKeyCountMultiBatch.json PRE-CREATION 
>   exec/java-exec/src/test/resources/window/twoKeys.json PRE-CREATION 
>   exec/java-exec/src/test/resources/window/twoKeysData.json PRE-CREATION 
>   exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillResultSet.java be56b967f54ac1b27bd0ea4ae8a2015aecb99929

>   exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestJdbcDistQuery.java 30a7144521117969db119791b0cb0896763c0494

>   exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestJdbcQuery.java d087f7d6661a079cb37655cf67995de7f30f9a2a

>   tools/drill-patch-review.py c067ae2d805fa45707401953bb040f1f062ba920 
> 
> Diff: https://reviews.apache.org/r/21371/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>


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