Pig
  1. Pig
  2. PIG-3492

ColumnPrune dropping used column due to LogicalRelationalOperator.fixDuplicateUids changes not propagating

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.11.1, 0.12.1, 0.13.0
    • Fix Version/s: 0.12.1
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      I don't have a testcase I can upload at the moment, but here's my observation.

      SplitFilter -> schemaResetter -> LOGenerate.getSchema -> LogicalRelationalOperator.fixDuplicateUids() creating a new UID but that UID is not propagated to the entire plan (since SplitFilter.reportChanges only returns subplan).

      As a result, I am seeing ColumnPruning cutting off those used columns.

      1. PIG-3492-trunk-delta.patch
        5 kB
        Daniel Dai
      2. pig-3492-trunk_04.patch
        51 kB
        Koji Noguchi
      3. pig-3492-v0.12_01.patch
        6 kB
        Koji Noguchi

        Issue Links

          Activity

          Koji Noguchi created issue -
          Hide
          Daniel Dai added a comment -

          Yes, I also see couple of places fixDuplicateUids is get misused. uid play a vital role in ColumnPruner. So every time we reassign uid, we need to make sure operator has the knowledge how does that uid get generated, and convey it to ColumnPruner.

          Show
          Daniel Dai added a comment - Yes, I also see couple of places fixDuplicateUids is get misused. uid play a vital role in ColumnPruner. So every time we reassign uid, we need to make sure operator has the knowledge how does that uid get generated, and convey it to ColumnPruner.
          Hide
          Koji Noguchi added a comment -

          We're only seeing this issue on complicate scripts with hundreds of lines.
          This is the shortest I got. This test needs to be called with '-t PushUpFilter'.

          pig> cat test.pig
          A = load './test.txt' as (a:int, b:chararray);
          B = FOREACH A generate a;
          C = GROUP B by a;
          D = filter C by group > 0 and group < 100;
          E = FOREACH D {
                   F = LIMIT B 1 ;
                   GENERATE B.a as mya, FLATTEN(F.a) as setting;
              }
          G = FOREACH E GENERATE mya, setting as setting;
          dump G;
          

          Relation G should contain two columns, 'mya' and 'setting'. But result only contains 1 column.

          pig> cat test.txt
          3       i
          3       i
          1       i
          2       i
          2       i
          3       i
          pig> pig -x local  -t PushUpFilter ./test.pig
          ({(1)})
          ({(2),(2)})
          ({(3),(3),(3)})
          

          By skipping ColumnMapKeyPrune or SplitFilter, you get a correct result of

          pig> pig -x local  -t PushUpFilter -t ColumnMapKeyPrune ./test.pig
          or
          pig> pig -x local  -t PushUpFilter -t SplitFilter  ./test.pig
          ({(1)},1)
          ({(2),(2)},2)
          ({(3),(3),(3)},3)
          

          Explain would show that second column was cut off.

          Incorrect case (-t PushUpFilter)
          G: (Name: LOStore Schema: mya#60:bag{#59:tuple(a#23:int)})ColumnPrune:InputUids=[63, 60]ColumnPrune:OutputUids=[63, 60]
          Correct case (-t PushUpFilter -t SplitFilter)
          G: (Name: LOStore Schema: mya#60:bag{#59:tuple(a#23:int)},setting#63:int)ColumnPrune:InputUids=[63, 60]ColumnPrune:OutputUids=[63, 60]
          
          Show
          Koji Noguchi added a comment - We're only seeing this issue on complicate scripts with hundreds of lines. This is the shortest I got. This test needs to be called with '-t PushUpFilter'. pig> cat test.pig A = load './test.txt' as (a:int, b:chararray); B = FOREACH A generate a; C = GROUP B by a; D = filter C by group > 0 and group < 100; E = FOREACH D { F = LIMIT B 1 ; GENERATE B.a as mya, FLATTEN(F.a) as setting; } G = FOREACH E GENERATE mya, setting as setting; dump G; Relation G should contain two columns, 'mya' and 'setting'. But result only contains 1 column. pig> cat test.txt 3 i 3 i 1 i 2 i 2 i 3 i pig> pig -x local -t PushUpFilter ./test.pig ({(1)}) ({(2),(2)}) ({(3),(3),(3)}) By skipping ColumnMapKeyPrune or SplitFilter, you get a correct result of pig> pig -x local -t PushUpFilter -t ColumnMapKeyPrune ./test.pig or pig> pig -x local -t PushUpFilter -t SplitFilter ./test.pig ({(1)},1) ({(2),(2)},2) ({(3),(3),(3)},3) Explain would show that second column was cut off. Incorrect case (-t PushUpFilter) G: (Name: LOStore Schema: mya#60:bag{#59:tuple(a#23:int)})ColumnPrune:InputUids=[63, 60]ColumnPrune:OutputUids=[63, 60] Correct case (-t PushUpFilter -t SplitFilter) G: (Name: LOStore Schema: mya#60:bag{#59:tuple(a#23:int)},setting#63:int)ColumnPrune:InputUids=[63, 60]ColumnPrune:OutputUids=[63, 60]
          Hide
          Koji Noguchi added a comment -

          I see three Jiras that added LogicalRelationalOperator.fixDuplicateUids.

          • PIG-3020 (LOJoin) "Duplicate uid in schema" error when joining two relations derived from the same load statement"
          • PIG-3144 (LOGenerate) "Erroneous map entry alias resolution leading to "Duplicate schema alias" errors"
          • PIG-3292 (LOCross) "Logical plan invalid state: duplicate uid in schema during self-join to get cross product"

          I'm skipping PIG-3292 since Daniel reviewed with the comment
          "Interplay with ColumnPruner is fine here since nested plan will include entire required plan branch"

          PIG-3020 (LOJoin) actually talks about two separate problems.
          (i-1) PigParser failing with 'Duplicate schema alias: age'. Only happened in 0.11.
          This was actually about ImplicitSplitInserter's new uid not propagating to the top foreach.
          I believe this issue was fixed later by PIG-3310 ("ImplicitSplitInserter does not generate new uids for nested schema fields, leading to miscomputations" fixed only in 0.12). Confirmed by running a simple test without LogicalRelationalOperator.fixDuplicateUids.

          (i-2) 'describe' showing incorrect schema due to duplicate UID. Happened on 0.10 and 0.11.
          This was due to 'describe' being called without LogicalPlanOptimizer.optimize() which includes some important rules like ImplicitSplitInserter and DuplicateForEachColumnRewrite.

          (ii) PIG-3144(LOGenerate) issue seems to have started after a completely unrelated Jira,

          PIG-2710 "Implement Naive CUBE operator" in 0.11.

          src/org/apache/pig/parser/LogicalPlanBuilder.java
          
          + 406     private void expandAndResetVisitor(SourceLocation loc,
          + 407       LogicalRelationalOperator lrop) throws ParserValidationException {
          + 408           try {
          + 409               (new ProjectStarExpander(lrop.getPlan())).visit();
          + 410               (new ProjStarInUdfExpander(lrop.getPlan())).visit();
          + 411               new SchemaResetter(lrop.getPlan(), true).visit();
          + 412           } catch (FrontendException e) {
          + 413               throw new ParserValidationException(intStream, loc, e);
          + 414           }
          + 415     }
          
           934     String buildForeachOp(SourceLocation loc, LOForEach op, String alias, String inputAlias, LogicalPlan innerPlan)
           935     throws ParserValidationException {
           936         op.setInnerPlan( innerPlan );
           937         alias = buildOp( loc, op, alias, inputAlias, null );
          
          -             (new ProjectStarExpander(op.getPlan())).visit(op);
          -             (new ProjStarInUdfExpander(op.getPlan())).visit(op);
          -             new SchemaResetter(op.getPlan(), true).visit(op);
          
          +938         expandAndResetVisitor(loc, op);
           939         return alias;
           940     }
          

          So basically we started traversing the entire plan (visit()) for every operator builds instead of just the operator it's working on (visit(op)).
          This has caused the 'alias' to get updated before LogicalPlanOptimizer.optimize() -> DuplicateForEachColumnRewrite and causing the
          "Duplicate schema alias" error. Rolling back this change seems to bring back the pre-0.11 behavior.

          Uploading an intial patch. Goal is to take out the LogicalRelationalOperator.fixDuplicateUids. from both PIG-3020(LOJoin) and PIG-3144(LOGenerate).

          (i-1) For release-0.12: No-op. For release-0.11: Backport pig-3310.
          (i-2) We can either fix it by forcing compilePp() before describe or moving ImplicitSplitInserter/DuplicateForEachColumnRewrite to PigServer.compile().
          There is a comment that says

          ./src/org/apache/pig/PigServer.java
          1692         private void compile(LogicalPlan lp) throws FrontendException  {
          ....
          1699
          1700             // TODO: move optimizer here from HExecuteEngine.
          1701             // TODO: input/output validation visitor
          1702
          

          For now, I'm taking an easy approach of calling compilePp() for describe.

          (ii) I'm rolling back small section of PIG-2710 in src/org/apache/pig/parser/LogicalPlanBuilder.java that was hopefully only for shortening the code and the change in behavior was unintended.

          For now, patch only applies to release 0.12 since it seems like location of LogicalPlanOptimizer.optimize() may change in the near future (PIG-3508).

          Show
          Koji Noguchi added a comment - I see three Jiras that added LogicalRelationalOperator.fixDuplicateUids. PIG-3020 (LOJoin) "Duplicate uid in schema" error when joining two relations derived from the same load statement" PIG-3144 (LOGenerate) "Erroneous map entry alias resolution leading to "Duplicate schema alias" errors" PIG-3292 (LOCross) "Logical plan invalid state: duplicate uid in schema during self-join to get cross product" I'm skipping PIG-3292 since Daniel reviewed with the comment "Interplay with ColumnPruner is fine here since nested plan will include entire required plan branch" PIG-3020 (LOJoin) actually talks about two separate problems. (i-1) PigParser failing with 'Duplicate schema alias: age'. Only happened in 0.11. This was actually about ImplicitSplitInserter's new uid not propagating to the top foreach. I believe this issue was fixed later by PIG-3310 ("ImplicitSplitInserter does not generate new uids for nested schema fields, leading to miscomputations" fixed only in 0.12). Confirmed by running a simple test without LogicalRelationalOperator.fixDuplicateUids. (i-2) 'describe' showing incorrect schema due to duplicate UID. Happened on 0.10 and 0.11. This was due to 'describe' being called without LogicalPlanOptimizer.optimize() which includes some important rules like ImplicitSplitInserter and DuplicateForEachColumnRewrite. (ii) PIG-3144 (LOGenerate) issue seems to have started after a completely unrelated Jira, PIG-2710 "Implement Naive CUBE operator" in 0.11. src/org/apache/pig/parser/LogicalPlanBuilder.java + 406 private void expandAndResetVisitor(SourceLocation loc, + 407 LogicalRelationalOperator lrop) throws ParserValidationException { + 408 try { + 409 (new ProjectStarExpander(lrop.getPlan())).visit(); + 410 (new ProjStarInUdfExpander(lrop.getPlan())).visit(); + 411 new SchemaResetter(lrop.getPlan(), true).visit(); + 412 } catch (FrontendException e) { + 413 throw new ParserValidationException(intStream, loc, e); + 414 } + 415 } 934 String buildForeachOp(SourceLocation loc, LOForEach op, String alias, String inputAlias, LogicalPlan innerPlan) 935 throws ParserValidationException { 936 op.setInnerPlan( innerPlan ); 937 alias = buildOp( loc, op, alias, inputAlias, null ); - (new ProjectStarExpander(op.getPlan())).visit(op); - (new ProjStarInUdfExpander(op.getPlan())).visit(op); - new SchemaResetter(op.getPlan(), true).visit(op); +938 expandAndResetVisitor(loc, op); 939 return alias; 940 } So basically we started traversing the entire plan (visit()) for every operator builds instead of just the operator it's working on (visit(op)). This has caused the 'alias' to get updated before LogicalPlanOptimizer.optimize() -> DuplicateForEachColumnRewrite and causing the "Duplicate schema alias" error. Rolling back this change seems to bring back the pre-0.11 behavior. Uploading an intial patch. Goal is to take out the LogicalRelationalOperator.fixDuplicateUids. from both PIG-3020 (LOJoin) and PIG-3144 (LOGenerate). (i-1) For release-0.12: No-op. For release-0.11: Backport pig-3310. (i-2) We can either fix it by forcing compilePp() before describe or moving ImplicitSplitInserter/DuplicateForEachColumnRewrite to PigServer.compile(). There is a comment that says ./src/org/apache/pig/PigServer.java 1692 private void compile(LogicalPlan lp) throws FrontendException { .... 1699 1700 // TODO: move optimizer here from HExecuteEngine. 1701 // TODO: input/output validation visitor 1702 For now, I'm taking an easy approach of calling compilePp() for describe. (ii) I'm rolling back small section of PIG-2710 in src/org/apache/pig/parser/LogicalPlanBuilder.java that was hopefully only for shortening the code and the change in behavior was unintended. For now, patch only applies to release 0.12 since it seems like location of LogicalPlanOptimizer.optimize() may change in the near future ( PIG-3508 ).
          Koji Noguchi made changes -
          Field Original Value New Value
          Attachment pig-3492-v0.12_01.patch [ 12607441 ]
          Koji Noguchi made changes -
          Attachment pig-3492-v0.12_01.patch [ 12607441 ]
          Hide
          Koji Noguchi added a comment -

          I carelessly uploaded a patch with a typo (and would fail on compile error).
          Running unittest, but appreciate your feedback.

          Show
          Koji Noguchi added a comment - I carelessly uploaded a patch with a typo (and would fail on compile error). Running unittest, but appreciate your feedback.
          Koji Noguchi made changes -
          Attachment pig-3492-v0.12_01.patch [ 12607600 ]
          Hide
          Rohini Palaniswamy added a comment -

          compilePp in dumpSchema makes couple of tests fail returning empty schema.

          Show
          Rohini Palaniswamy added a comment - compilePp in dumpSchema makes couple of tests fail returning empty schema.
          Hide
          Koji Noguchi added a comment -

          compilePp in dumpSchema makes couple of tests fail returning empty schema.

          Thanks Rohini. I guess my shortcut didn't work for some tests... For 0.11, we might want to take out compilePp hack and keep the rest. That would at least make 0.11 as good(bad) as 0.10 in terms of the bugs we're seeing. ('describe' bug would remain but LOGenerate/LOJoin bug fixed)

          Now, for the 0.12 and long term, Taking the latter approach from my previous comment.

          (i-2) We can either fix it by forcing compilePp() before describe or moving ImplicitSplitInserter/DuplicateForEachColumnRewrite to PigServer.compile().

          ImplicitSplitInserter/DuplicateForEachColumnRewrite seem to be an essential part of compilation for correctness and they aren't really an optimization. With that assumption, I rewrote them as Visitors and moved them from LogicalPlanOptimizer to PigServer.compile.

          With this change, 38 unit tests started failing.

          • 5 tests failing with NullPointerException in Illustrate. I believe this was due to a bug in pig/pen/LineageTrimmingVisitor.java. Added entry of LOSplitOutput for the fix.
          • 1 TestOptimizeLimit failing with typecasting LOLimit to LOForeach. This was due to change in logical plan having ImplicitSplitInserter/DuplicateForEachColumnRewrite as default irrespective of the optimizer the test picks.
          • 32 failures at TestMultiQueryCompiler/TestMultiQueryLocal since the tests were counting the number of Logical Operators. Updated the tests after making sure increase is only coming from LOSplit and LOSplitOutput.

          Trying to get e2e running with this patch.

          Show
          Koji Noguchi added a comment - compilePp in dumpSchema makes couple of tests fail returning empty schema. Thanks Rohini. I guess my shortcut didn't work for some tests... For 0.11, we might want to take out compilePp hack and keep the rest. That would at least make 0.11 as good(bad) as 0.10 in terms of the bugs we're seeing. ('describe' bug would remain but LOGenerate/LOJoin bug fixed) Now, for the 0.12 and long term, Taking the latter approach from my previous comment. (i-2) We can either fix it by forcing compilePp() before describe or moving ImplicitSplitInserter/DuplicateForEachColumnRewrite to PigServer.compile(). ImplicitSplitInserter/DuplicateForEachColumnRewrite seem to be an essential part of compilation for correctness and they aren't really an optimization. With that assumption, I rewrote them as Visitors and moved them from LogicalPlanOptimizer to PigServer.compile. With this change, 38 unit tests started failing. 5 tests failing with NullPointerException in Illustrate. I believe this was due to a bug in pig/pen/LineageTrimmingVisitor.java. Added entry of LOSplitOutput for the fix. 1 TestOptimizeLimit failing with typecasting LOLimit to LOForeach. This was due to change in logical plan having ImplicitSplitInserter/DuplicateForEachColumnRewrite as default irrespective of the optimizer the test picks. 32 failures at TestMultiQueryCompiler/TestMultiQueryLocal since the tests were counting the number of Logical Operators. Updated the tests after making sure increase is only coming from LOSplit and LOSplitOutput. Trying to get e2e running with this patch.
          Koji Noguchi made changes -
          Attachment pig-3492-trunk_04.patch [ 12608947 ]
          Koji Noguchi made changes -
          Assignee Koji Noguchi [ knoguchi ]
          Hide
          Koji Noguchi added a comment -

          Daniel asked me to summarize the changes. Here you go.

          (1) Move DuplicateForEachColumnRewrite and ImplicitSplitInserter from LogicalPlanOptimizer to PigServer.compile().
          Reason: Next visitor, TypeCheckingRelVisitor, was calling resetSchema/getSchema and fields with duplicate UIDs were getting incorrect aliases due to that. (Execution was fine since LogicalPlanOptimizer.optimize() was eventually called, but not for 'describe'. Also, even temporary, it's not good to have incorrect aliases assigned to LogicalOperators.

          (2) Fix the test cases that started failing due to (1).
          (2-1) LineageTrimmingVisitor (used in illustrate) was hitting with NullPointerException since LOSplitOutput was missing in the code.
          (2-2) TestOptimizeLimit failed due to changed logicalplan causing typecast error. Fixed.
          (2-3) Bunch of TestOptimizeLimit tests failed due to new logicalplan including LOSplit and LOSplitOUtput.

          (3) Rolling back changes in LogicalPlanBuilder from pig-2710 since ProjectStarExpander/ProjStarInUdfExpander/ProjStarInUdfExpander used to be called only for the corresponding LogicalOperator but the change started calling them for the entire plan each time. This change itself fixed (5) and brought back the 0.10 behavior.

          (4) Revert PIG-3020. Take out LogicalRelationalOperator.fixDuplicateUids from LOJoin. Fixed in PIG-3310.

          (5) Revert PIG-3144. Take out LogicalRelationalOperator.fixDuplicateUids from LOGenerate. Fixed by (1) and (3).

          Show
          Koji Noguchi added a comment - Daniel asked me to summarize the changes. Here you go. (1) Move DuplicateForEachColumnRewrite and ImplicitSplitInserter from LogicalPlanOptimizer to PigServer.compile(). Reason: Next visitor, TypeCheckingRelVisitor, was calling resetSchema/getSchema and fields with duplicate UIDs were getting incorrect aliases due to that. (Execution was fine since LogicalPlanOptimizer.optimize() was eventually called, but not for 'describe'. Also, even temporary, it's not good to have incorrect aliases assigned to LogicalOperators. (2) Fix the test cases that started failing due to (1). (2-1) LineageTrimmingVisitor (used in illustrate) was hitting with NullPointerException since LOSplitOutput was missing in the code. (2-2) TestOptimizeLimit failed due to changed logicalplan causing typecast error. Fixed. (2-3) Bunch of TestOptimizeLimit tests failed due to new logicalplan including LOSplit and LOSplitOUtput. (3) Rolling back changes in LogicalPlanBuilder from pig-2710 since ProjectStarExpander/ProjStarInUdfExpander/ProjStarInUdfExpander used to be called only for the corresponding LogicalOperator but the change started calling them for the entire plan each time. This change itself fixed (5) and brought back the 0.10 behavior. (4) Revert PIG-3020 . Take out LogicalRelationalOperator.fixDuplicateUids from LOJoin. Fixed in PIG-3310 . (5) Revert PIG-3144 . Take out LogicalRelationalOperator.fixDuplicateUids from LOGenerate. Fixed by (1) and (3).
          Hide
          Koji Noguchi added a comment -

          Forgot to add.

          For 0.11, asking if we can consider (3), (4) and (5). Leaving the 'describe' bug.
          For 0.12 and later, asking (1) to (5).

          Show
          Koji Noguchi added a comment - Forgot to add. For 0.11, asking if we can consider (3), (4) and (5). Leaving the 'describe' bug. For 0.12 and later, asking (1) to (5).
          Koji Noguchi made changes -
          Link This issue is related to PIG-3020 [ PIG-3020 ]
          Koji Noguchi made changes -
          Link This issue is related to PIG-3144 [ PIG-3144 ]
          Hide
          Daniel Dai added a comment -

          Thanks Koji Horaguchi for detailed explanation. This patch fixed several issues:
          1. Remove fixDuplicateUids from LOJoin/LOGenerate
          That's for sure need to be removed. We cannot arbitrary reassign uid. Every operator should have lineage knowledge and be able to trace to input uids which generate a particular output uid. Sometimes we do need to reassign the uid, but if this happens, we need to remember the mapping inside the operator, and update the logic ColumnDependencyVisitor of tracing every output uid to its input uid.

          Take LOJoin for example, each output column is derived from one input column without transformation, so we keep uid unchanged. If input#1 of join has the uid (1, 2, 3), input#2 has the uid (4, 5), the join output will have the uid (1, 2, 3, 4, 5). When ColumnDependencyVisitor try to prune uid 5, we know that is from second column of input#2. In the case we do want to regenerate output uid to (21, 22, 23, 24, 25), join operator need to remember the mapping (1->21, 2->22, 3->23, 4->24, 5->25), so that when ColumnDependencyVisitor try to prune uid 25, we know it is from second column of input#2 as well. One example of uid regeneration is LOSplit, which has a field uidMapping to remember the mapping. ColumnDependencyVisitor will make use of this information to trace the lineage.

          As we already mentioned, uid regeneration is already done in LOSplitOutput, so after ImplicitSplitInserter, we shall not have duplicate uids and should not have uid conflict. The issue seen in PIG-3020 is due to a bug in LOSplitOutput uid mapping process as Koji mentioned.

          2. LogicalPlanBuilder.expandAndResetVisitor change
          Not sure what drive the original change, seems there is no reason to visit the whole plan after building one operator. The logical plan is not in a consistent state at this moment, global SchemaResetter might not work

          3. move DuplicateForEachColumnRewrite and ImplicitSplitInserter
          When we design optimizer, we try to include every plan transformation into it. But now I think we may separate it into two part. One is to bring the plan into a valid state, the other is the real optimizer rule. The reason is at validate stage, the plan is still inconsistent and many optimizer rule cannot handle, and the global SchemaReset/UidResetter does not work. We may want to do some refactory and make the separation clear in another Jira. "describe" should happen after validation, but might not need to go through optimizer. Move DuplicateForEachColumnRewrite/ImplicitSplitInserter into compile for now seems to be ok.

          Overall, the patch looks pretty good. It fix above mentioned issue cleanly. We might want to preceed with clear validator/optmizer separation in anther Jira.

          Will commit if tests pass.

          Show
          Daniel Dai added a comment - Thanks Koji Horaguchi for detailed explanation. This patch fixed several issues: 1. Remove fixDuplicateUids from LOJoin/LOGenerate That's for sure need to be removed. We cannot arbitrary reassign uid. Every operator should have lineage knowledge and be able to trace to input uids which generate a particular output uid. Sometimes we do need to reassign the uid, but if this happens, we need to remember the mapping inside the operator, and update the logic ColumnDependencyVisitor of tracing every output uid to its input uid. Take LOJoin for example, each output column is derived from one input column without transformation, so we keep uid unchanged. If input#1 of join has the uid (1, 2, 3), input#2 has the uid (4, 5), the join output will have the uid (1, 2, 3, 4, 5). When ColumnDependencyVisitor try to prune uid 5, we know that is from second column of input#2. In the case we do want to regenerate output uid to (21, 22, 23, 24, 25), join operator need to remember the mapping (1->21, 2->22, 3->23, 4->24, 5->25), so that when ColumnDependencyVisitor try to prune uid 25, we know it is from second column of input#2 as well. One example of uid regeneration is LOSplit, which has a field uidMapping to remember the mapping. ColumnDependencyVisitor will make use of this information to trace the lineage. As we already mentioned, uid regeneration is already done in LOSplitOutput, so after ImplicitSplitInserter, we shall not have duplicate uids and should not have uid conflict. The issue seen in PIG-3020 is due to a bug in LOSplitOutput uid mapping process as Koji mentioned. 2. LogicalPlanBuilder.expandAndResetVisitor change Not sure what drive the original change, seems there is no reason to visit the whole plan after building one operator. The logical plan is not in a consistent state at this moment, global SchemaResetter might not work 3. move DuplicateForEachColumnRewrite and ImplicitSplitInserter When we design optimizer, we try to include every plan transformation into it. But now I think we may separate it into two part. One is to bring the plan into a valid state, the other is the real optimizer rule. The reason is at validate stage, the plan is still inconsistent and many optimizer rule cannot handle, and the global SchemaReset/UidResetter does not work. We may want to do some refactory and make the separation clear in another Jira. "describe" should happen after validation, but might not need to go through optimizer. Move DuplicateForEachColumnRewrite/ImplicitSplitInserter into compile for now seems to be ok. Overall, the patch looks pretty good. It fix above mentioned issue cleanly. We might want to preceed with clear validator/optmizer separation in anther Jira. Will commit if tests pass.
          Hide
          Koji Noguchi added a comment -

          Thanks for the review Daniel.

          Thanks Koji Horaguchi for detailed explanation. This patch fixed several issues:

          Different Koji It's Koji Noguchi.

          Show
          Koji Noguchi added a comment - Thanks for the review Daniel. Thanks Koji Horaguchi for detailed explanation. This patch fixed several issues: Different Koji It's Koji Noguchi .
          Hide
          Daniel Dai added a comment -

          Fix some e2e test failures (eg: udf_TOBAGandTOTUPLE_3), also include Koji's test case in PIG-3492-trunk-delta.patch.

          Show
          Daniel Dai added a comment - Fix some e2e test failures (eg: udf_TOBAGandTOTUPLE_3), also include Koji's test case in PIG-3492 -trunk-delta.patch.
          Daniel Dai made changes -
          Attachment PIG-3492-trunk-delta.patch [ 12609890 ]
          Hide
          Daniel Dai added a comment -

          pig-3492-trunk_04.patch and pig-3492-trunk-delta.patch are committed to both trunk and 0.12 branch. I don't think we will have additional 0.11 release, so skip 0.11 branch.

          I will open additional Jira to formalize the validator.

          Thanks Koji, that's a really important fix.

          Show
          Daniel Dai added a comment - pig-3492-trunk_04.patch and pig-3492-trunk-delta.patch are committed to both trunk and 0.12 branch. I don't think we will have additional 0.11 release, so skip 0.11 branch. I will open additional Jira to formalize the validator. Thanks Koji, that's a really important fix.
          Daniel Dai made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Hadoop Flags Reviewed [ 10343 ]
          Fix Version/s 0.12.1 [ 12324970 ]
          Resolution Fixed [ 1 ]
          Prashant Kommireddi made changes -
          Status Resolved [ 5 ] Closed [ 6 ]

            People

            • Assignee:
              Koji Noguchi
              Reporter:
              Koji Noguchi
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development