----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24778/#review54100 -----------------------------------------------------------
Generally looks good. There is a lot of noise in this patch due to other simple cleanups. These cleanups are veyr good but I think we should try to keep separate things separate as that makes it easier for the right person to review something. I've provided a bunch of feedback here but I need to spend a little more time looking at the RecordBatch and Template classes to review the logic and edgecases. Once I do that, I'll add any additional feedback on the logic there. I think that most the issues below are simple cosmetic things that shouldn't take too long. Let's see if we can get this merged tomorrow if you can get back the changes. common/src/main/java/org/apache/drill/common/expression/IfExpression.java <https://reviews.apache.org/r/24778/#comment94083> Can you separate out your patch for all your cleanups that have nothing to do with the window function? common/src/main/java/org/apache/drill/common/logical/data/Limit.java <https://reviews.apache.org/r/24778/#comment94067> It isn't clear why you removed the node builder. Can you explain? exec/java-exec/src/main/codegen/templates/AggrTypeFunctions1.java <https://reviews.apache.org/r/24778/#comment94084> Same as above (sum0) exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/Limit.java <https://reviews.apache.org/r/24778/#comment94085> Can you please break the Limit => LimitPOP into a separate patch? exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/filter/FilterRecordBatch.java <https://reviews.apache.org/r/24778/#comment94068> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/limit/LimitRecordBatch.java <https://reviews.apache.org/r/24778/#comment94069> Same as above. exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/WindowFrameRecordBatch.java <https://reviews.apache.org/r/24778/#comment94077> Can you call this Streaming[name] exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/WindowFrameRecordBatch.java <https://reviews.apache.org/r/24778/#comment94070> This looks wrong. Why do you need to match certain field names? I think this is a dangerous assumption that will cause problems depending on what planning is applied after Optiq generates initial field names. We should be able to get everything we need from the POP? exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/WindowFrameTemplate.java <https://reviews.apache.org/r/24778/#comment94076> Can you call this Streaming[name] exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/WindowFrameTemplate.java <https://reviews.apache.org/r/24778/#comment94071> You shouldn't commit any logging statements within an operator. If you want to leave them, use a static condition for whether they are enabled and then set to false for your commit. exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/WindowFramer.java <https://reviews.apache.org/r/24778/#comment94075> Can you call this Streaming[name] exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillRel.java <https://reviews.apache.org/r/24778/#comment94072> should be static final exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/WindowPrel.java <https://reviews.apache.org/r/24778/#comment94074> Can you call this Streaming[name] exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/DefaultSqlHandler.java <https://reviews.apache.org/r/24778/#comment94086> I think maybe you should create a SqlHandlerConfig object and hang context and the planners off it rather than passing in. Seems like otherwise if we need to add something else in future, we're again going to have to change a large number of classes. exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/DefaultSqlHandler.java <https://reviews.apache.org/r/24778/#comment94078> Have you reviewed all the handlers and confirmed that they all will handle the window rel correctly? e.g. create view and explain. exec/java-exec/src/main/java/org/apache/drill/exec/schema/Field.java <https://reviews.apache.org/r/24778/#comment94087> Again, please move into separate cleanup patch. exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestUtils.java <https://reviews.apache.org/r/24778/#comment94088> We're trying to deprecate SimpleRootExec. Is there a way you can similar tests using Jason's new testing validation framework? I don't really want to add new tests using SimpleRootExec. exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/window/TestWindowFrame.java <https://reviews.apache.org/r/24778/#comment94082> Can you please add a SQL test of some sort as well to show that a SQL statement will work. exec/java-exec/src/test/resources/window/oneKeyCountMultiBatch.json <https://reviews.apache.org/r/24778/#comment94081> formatting is weird. exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillResultSet.java <https://reviews.apache.org/r/24778/#comment94080> ? tools/drill-patch-review.py <https://reviews.apache.org/r/24778/#comment94079> can you please separate this into a separate patch review update patch? - Jacques Nadeau On Aug. 17, 2014, 12:45 a.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24778/ > ----------------------------------------------------------- > > (Updated Aug. 17, 2014, 12:45 a.m.) > > > Review request for drill and Jacques Nadeau. > > > 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 > > > Diffs > ----- > > common/src/main/java/org/apache/drill/common/expression/CastExpression.java > 7e5eea096b53ede676abb4764631fd92805b4ad1 > > common/src/main/java/org/apache/drill/common/expression/ExpressionStringBuilder.java > 0e778c5ebfba827a88826cd169d5e18e3f68440f > common/src/main/java/org/apache/drill/common/expression/IfExpression.java > f16d8fe0951681ca138025abf86b0f5d25b1a6e6 > common/src/main/java/org/apache/drill/common/expression/NullExpression.java > f515d1467475b04dade0b878b3121e35a6520bc4 > > common/src/main/java/org/apache/drill/common/expression/visitors/AggregateChecker.java > 4c75459e37b65623541c2556f2cc6eda64912c96 > > common/src/main/java/org/apache/drill/common/expression/visitors/ConstantChecker.java > a2a0d7fd279004dd8526728991a779f7b92c558f > > 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/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 > 87584449f2b0dfddc5ede5c766b79095bcc95603 > > common/src/main/java/org/apache/drill/common/logical/data/visitors/LogicalVisitor.java > 4bf9fbfcaac21ab077ce26993575f679010bf592 > > contrib/storage-hive/core/src/main/java/org/apache/drill/exec/expr/fn/HiveFunctionRegistry.java > c4813a75b25b8001e1b5543ae523a24ae12a4aef > exec/java-exec/src/main/codegen/data/AggrTypes1.tdd > 1bac07ee30558018932117d9e0d978b75742df83 > exec/java-exec/src/main/codegen/templates/AggrTypeFunctions1.java > aa9aeab7af7d387cff9da57af69d3c9a5a9498fd > exec/java-exec/src/main/codegen/templates/TypeHelper.java > 9bb25075730876996f02c5249f300cdda35dd882 > > exec/java-exec/src/main/java/org/apache/drill/exec/compile/sig/ConstantExpressionIdentifier.java > c65951de52296a8051d4c3b0a542d7fb9841cd2b > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/EvaluationVisitor.java > caf91793b08730596d4b2305ed8e18d1ad210890 > exec/java-exec/src/main/java/org/apache/drill/exec/memory/Accountor.java > cd0836749b941d0e01f0b48db7651372fc7f7bb2 > exec/java-exec/src/main/java/org/apache/drill/exec/opt/BasicOptimizer.java > 24d9cfe528c4ef7d930f216a7428557d29ffe357 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractPhysicalVisitor.java > 5e854251bc0c97efd584edfa2e1f91861b8d1077 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractSingle.java > 6c0b98f8650d21d743b95ca0ef6e05cde8524662 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/PhysicalOperator.java > 8f51390293fea89c278ae280bcf8bd5f8bb64a2a > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/PhysicalVisitor.java > f0b0b9a84642bc6bf74184be8b0fae58b9270da4 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/Limit.java > 0038e4e0aae9af926f1c26f7186b2bd711a5bc18 > > 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/aggregate/StreamingAggBatch.java > 39131125dfb08e1db11051049b33dbcd62620404 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/filter/FilterRecordBatch.java > 3ece98beb6c6aec4898000a8ca158aaebbb630ed > > 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 > 12ee40617fc3cdf82cb58f313c49f52cf2964ff2 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/producer/ProducerConsumerBatchCreator.java > 0fcf4f358966a0cddf70c32ebbdd5cb7d031f30a > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/project/ProjectRecordBatch.java > 1ba0103473b6d12981705a3abc16c2a84b2dbdaf > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/svremover/RemovingRecordBatch.java > 42f21283687538a00eea6afbc69a2044cbbf711c > > 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 > 3fc3b89d1d7dbed0e890b6f942064851c9d09353 > > 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/DrillLimitRule.java > c3b0d00c6bddf52cb88307a49df2574b720a4806 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillPushProjIntoScan.java > 829eb1406a68c447c2cc319e36c847174d7ea195 > > 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 > 63de69c95942661929513aae95aa7c23f2abb255 > > 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 > a4bb5035519f59dd746ada6efdf9200ce86f51e3 > > 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 > e34d3d15a4bfc5a5d0674b626215d702345a7896 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlAggOperator.java > 299712ef9afa0900a9966b21da060fcd80ad9842 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java > 321d79d4e1631077d06daeee7cc66595fdb3d987 > > 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 > 177331d9ac2b6bab4e62206e2985c042b51a97e9 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/DescribeTableHandler.java > e6f1fe1c4b8e7e0c4edb55fd5d669dda6274e7ab > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/ExplainHandler.java > 63db153ba4c8850be3c77433bc9696be085a5c01 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/ShowFileHandler.java > 17e80bd3ec09e47896a19d5e6fa24db874b8c01b > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/ShowSchemasHandler.java > 5e77628f022a1cec74196c6bb99a565204de0959 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/ShowTablesHandler.java > a1c5aee92ed90018b02ba28e772ba5cb3fb6ccc6 > > 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 > 01707354631ec371e7f1b819acf45ef265379809 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/SqlCreateView.java > 3666b4cccfd0608921febf96e1e60e67c95a6737 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/SqlDescribeTable.java > 15da822586f5a42f4ae4b66dc229d3793b0194b2 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/SqlDropView.java > 1d943901ab99ba3b14346a9fe850153c7f7332e8 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/SqlShowFiles.java > 690e5fd8c33eecf6dda6692e22c54b82843760c1 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/SqlShowSchemas.java > faa69d10700985f88c03ac7443f3c2e0b523431e > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/SqlShowTables.java > e66199a6c6c4eee05a3f1dd378ab3a69b2e7997f > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/SqlUseSchema.java > 76c7df3a5a7786f5b96c233787a820f22ebbcc76 > > exec/java-exec/src/main/java/org/apache/drill/exec/record/AbstractSingleRecordBatch.java > bea7bbfe9243e6d8d0bf415a336b9fe02dce7dad > > exec/java-exec/src/main/java/org/apache/drill/exec/record/VectorContainer.java > 49c739916426d727e1c088b2a6aa4fb5d9cc2c2e > exec/java-exec/src/main/java/org/apache/drill/exec/schema/Field.java > 14049efaa7b2684e3f537985ec42977bb1863657 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/NonRootFragmentManager.java > 48d14662e02c6371283419d42094600dcd23b6a5 > exec/java-exec/src/test/java/org/apache/drill/exec/ExecTest.java > e7c6dc06fc9c90a90fecdd9c2757f580d4be5fc8 > > 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/limit/TestSimpleLimit.java > 0d5d62216b43c104f74d6cf50bd255e3db3e54c2 > > 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 > bde0d3fac684ad1202b3566a4c688ce8de9d3b43 > exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestJdbcDistQuery.java > 39ba043c08fdcedfbeac4b0327e2bd7372d41596 > exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestJdbcQuery.java > b681719a8f93059c2b4f16abc112d5178048c44c > protocol/src/main/java/org/apache/drill/exec/proto/UserBitShared.java > d5b235245e7938fe31330f201c9f3fc7290a06f9 > > protocol/src/main/java/org/apache/drill/exec/proto/beans/CoreOperatorType.java > 0c83e06800e7323b5d0271b2ba39d211cf1530a9 > protocol/src/main/protobuf/UserBitShared.proto > 10dce1fb9095193b01671d0ef05ee26a85fb416a > tools/drill-patch-review.py c067ae2d805fa45707401953bb040f1f062ba920 > > Diff: https://reviews.apache.org/r/24778/diff/ > > > Testing > ------- > > Added few unit tests around window. > > > Thanks, > > Timothy Chen > >
