Hive
  1. Hive
  2. HIVE-123

refactor DDL code (both DDLWork and DDLTask)

    Details

      Description

      It might be good to break DDLTask into separate tasks. The abstract class DDLWork can have various subclasses:
      showTablesWork, DescribeTableWork etc. and a separate task for each of them. This will make them completely independent.

      1. ASF.LICENSE.NOT.GRANTED--HIVE-123.D1485.1.patch
        1 kB
        Phabricator
      2. ASF.LICENSE.NOT.GRANTED--HIVE-123.D1485.2.patch
        1 kB
        Phabricator
      3. ASF.LICENSE.NOT.GRANTED--HIVE-123.D1803.1.patch
        4 kB
        Phabricator
      4. hive-123.patch
        38 kB
        Ashutosh Chauhan

        Activity

        Hide
        Ashutosh Chauhan added a comment -

        I wont have any time in near future for this. Feel free to take it up.

        Show
        Ashutosh Chauhan added a comment - I wont have any time in near future for this. Feel free to take it up.
        Hide
        Phabricator added a comment -

        dmitrys requested code review of "HIVE-123 [jira] refactor DDL code (both DDLWork and DDLTask)".
        Reviewers: kevinwilfong, JIRA

        Task #675564 Throw a Hive error if we're in strict mode and a the types of a partition comparison do not match.

        Added the check in strict mode.

        Task ID: #675564

        It might be good to break DDLTask into separate tasks. The abstract class DDLWork can have various subclasses:
        showTablesWork, DescribeTableWork etc. and a separate task for each of them. This will make them completely independent.

        TEST PLAN
        Run the negative test select_partcol_diff_types.q

        Revert Plan: revert the changes of the commit

        Tags: hive

        REVISION DETAIL
        https://reviews.facebook.net/D1803

        AFFECTED FILES
        ql/src/java/org/apache/hadoop/hive/ql/parse/ErrorMsg.java
        ql/src/java/org/apache/hadoop/hive/ql/plan/ExprNodeGenericFuncDesc.java
        ql/src/test/queries/clientnegative/select_partcol_diff_types.q
        ql/src/test/results/clientnegative/select_partcol_diff_types.q.out

        MANAGE HERALD DIFFERENTIAL RULES
        https://reviews.facebook.net/herald/view/differential/

        WHY DID I GET THIS EMAIL?
        https://reviews.facebook.net/herald/transcript/3843/

        Tip: use the X-Herald-Rules header to filter Herald messages in your client.

        Show
        Phabricator added a comment - dmitrys requested code review of " HIVE-123 [jira] refactor DDL code (both DDLWork and DDLTask)". Reviewers: kevinwilfong, JIRA Task #675564 Throw a Hive error if we're in strict mode and a the types of a partition comparison do not match. Added the check in strict mode. Task ID: #675564 It might be good to break DDLTask into separate tasks. The abstract class DDLWork can have various subclasses: showTablesWork, DescribeTableWork etc. and a separate task for each of them. This will make them completely independent. TEST PLAN Run the negative test select_partcol_diff_types.q Revert Plan: revert the changes of the commit Tags: hive REVISION DETAIL https://reviews.facebook.net/D1803 AFFECTED FILES ql/src/java/org/apache/hadoop/hive/ql/parse/ErrorMsg.java ql/src/java/org/apache/hadoop/hive/ql/plan/ExprNodeGenericFuncDesc.java ql/src/test/queries/clientnegative/select_partcol_diff_types.q ql/src/test/results/clientnegative/select_partcol_diff_types.q.out MANAGE HERALD DIFFERENTIAL RULES https://reviews.facebook.net/herald/view/differential/ WHY DID I GET THIS EMAIL? https://reviews.facebook.net/herald/transcript/3843/ Tip: use the X-Herald-Rules header to filter Herald messages in your client.
        Hide
        Phabricator added a comment -

        hobbymanyp updated the revision "HIVE-123 [jira] refactor DDL code (both DDLWork and DDLTask)".
        Reviewers: JIRA, heyongqiang, cwsteinbach

        Use the correct JIRA id HIVE-2765

        REVISION DETAIL
        https://reviews.facebook.net/D1485

        AFFECTED FILES
        hbase-handler/src/java/org/apache/hadoop/hive/hbase/HBaseStorageHandler.java

        Show
        Phabricator added a comment - hobbymanyp updated the revision " HIVE-123 [jira] refactor DDL code (both DDLWork and DDLTask)". Reviewers: JIRA, heyongqiang, cwsteinbach Use the correct JIRA id HIVE-2765 REVISION DETAIL https://reviews.facebook.net/D1485 AFFECTED FILES hbase-handler/src/java/org/apache/hadoop/hive/hbase/HBaseStorageHandler.java
        Hide
        Phabricator added a comment -

        hobbymanyp has commented on the revision "HIVE-123 [jira] refactor DDL code (both DDLWork and DDLTask)".

        According to the steps, I think i am using HIVE-123. Should I change it to "arc diff --jira HIVE-123x" (instead of "arc diff --jira HIVE-123") ?

        https://cwiki.apache.org/Hive/phabricatorcodereview.html
        From a git checkout (e.g. from git clone git://github.com/apache/hive.git), a typical usage sequence is as follows:

        ant arc-setup
        git checkout -b HIVE-123-dev-branch
        edit some files to implement HIVE-123
        git commit -a -m "initial description of how I have gone about making DDL code suck less"
        arc diff --jira HIVE-123
        optionally, visit diff URL provided by Phabricator to do things like add reviewers and CC's

        REVISION DETAIL
        https://reviews.facebook.net/D1485

        Show
        Phabricator added a comment - hobbymanyp has commented on the revision " HIVE-123 [jira] refactor DDL code (both DDLWork and DDLTask)". According to the steps, I think i am using HIVE-123 . Should I change it to "arc diff --jira HIVE-123 x" (instead of "arc diff --jira HIVE-123 ") ? https://cwiki.apache.org/Hive/phabricatorcodereview.html From a git checkout (e.g. from git clone git://github.com/apache/hive.git), a typical usage sequence is as follows: ant arc-setup git checkout -b HIVE-123 -dev-branch edit some files to implement HIVE-123 git commit -a -m "initial description of how I have gone about making DDL code suck less" arc diff --jira HIVE-123 optionally, visit diff URL provided by Phabricator to do things like add reviewers and CC's REVISION DETAIL https://reviews.facebook.net/D1485
        Hide
        Phabricator added a comment -

        cwsteinbach has commented on the revision "HIVE-123 [jira] refactor DDL code (both DDLWork and DDLTask)".

        @hobbymanyp: The directions you cited are correct, as far as I know. I suspect that you referenced the wrong JIRA when you initially submitted your request. Can you please confirm that this patch is for HIVE-123x, and not HIVE-123?

        REVISION DETAIL
        https://reviews.facebook.net/D1485

        Show
        Phabricator added a comment - cwsteinbach has commented on the revision " HIVE-123 [jira] refactor DDL code (both DDLWork and DDLTask)". @hobbymanyp: The directions you cited are correct, as far as I know. I suspect that you referenced the wrong JIRA when you initially submitted your request. Can you please confirm that this patch is for HIVE-123 x, and not HIVE-123 ? REVISION DETAIL https://reviews.facebook.net/D1485
        Hide
        Phabricator added a comment -

        hobbymanyp has commented on the revision "HIVE-123 [jira] refactor DDL code (both DDLWork and DDLTask)".

        I am following up the instruction on https://cwiki.apache.org/Hive/phabricatorcodereview.html . If this is not correct, where is the right one? (or instruction?)

        REVISION DETAIL
        https://reviews.facebook.net/D1485

        Show
        Phabricator added a comment - hobbymanyp has commented on the revision " HIVE-123 [jira] refactor DDL code (both DDLWork and DDLTask)". I am following up the instruction on https://cwiki.apache.org/Hive/phabricatorcodereview.html . If this is not correct, where is the right one? (or instruction?) REVISION DETAIL https://reviews.facebook.net/D1485
        Hide
        Carl Steinbach added a comment -

        -1. Supplied patch has nothing to do with DDL code. Please resubmit against the right JIRA issue.

        Show
        Carl Steinbach added a comment - -1. Supplied patch has nothing to do with DDL code. Please resubmit against the right JIRA issue.
        Hide
        Phabricator added a comment -

        cwsteinbach has requested changes to the revision "HIVE-123 [jira] refactor DDL code (both DDLWork and DDLTask)".

        It looks like this request was filed against the wrong JIRA. Please resubmit against the correct JIRA.

        REVISION DETAIL
        https://reviews.facebook.net/D1485

        Show
        Phabricator added a comment - cwsteinbach has requested changes to the revision " HIVE-123 [jira] refactor DDL code (both DDLWork and DDLTask)". It looks like this request was filed against the wrong JIRA. Please resubmit against the correct JIRA. REVISION DETAIL https://reviews.facebook.net/D1485
        Hide
        Phabricator added a comment -

        kevinwilfong has commented on the revision "HIVE-123 [jira] refactor DDL code (both DDLWork and DDLTask)".

        +1 Looks good to me.
        Need someone else to commit it though.

        REVISION DETAIL
        https://reviews.facebook.net/D1485

        Show
        Phabricator added a comment - kevinwilfong has commented on the revision " HIVE-123 [jira] refactor DDL code (both DDLWork and DDLTask)". +1 Looks good to me. Need someone else to commit it though. REVISION DETAIL https://reviews.facebook.net/D1485
        Hide
        Phabricator added a comment -

        hobbymanyp has added reviewers to the revision "HIVE-123 [jira] refactor DDL code (both DDLWork and DDLTask)".
        Added Reviewers: heyongqiang

        REVISION DETAIL
        https://reviews.facebook.net/D1485

        Show
        Phabricator added a comment - hobbymanyp has added reviewers to the revision " HIVE-123 [jira] refactor DDL code (both DDLWork and DDLTask)". Added Reviewers: heyongqiang REVISION DETAIL https://reviews.facebook.net/D1485
        Hide
        Phabricator added a comment -

        hobbymanyp has added CCs to the revision "HIVE-123 [jira] refactor DDL code (both DDLWork and DDLTask)".
        Added CCs: stuart983, njain

        REVISION DETAIL
        https://reviews.facebook.net/D1485

        Show
        Phabricator added a comment - hobbymanyp has added CCs to the revision " HIVE-123 [jira] refactor DDL code (both DDLWork and DDLTask)". Added CCs: stuart983, njain REVISION DETAIL https://reviews.facebook.net/D1485
        Hide
        Phabricator added a comment -

        hobbymanyp requested code review of "HIVE-123 [jira] refactor DDL code (both DDLWork and DDLTask)".
        Reviewers: JIRA

        Make hbase handler compatible with further version of 0.89

        It might be good to break DDLTask into separate tasks. The abstract class DDLWork can have various subclasses:
        showTablesWork, DescribeTableWork etc. and a separate task for each of them. This will make them completely independent.

        TEST PLAN
        EMPTY

        REVISION DETAIL
        https://reviews.facebook.net/D1485

        AFFECTED FILES
        hbase-handler/src/java/org/apache/hadoop/hive/hbase/HBaseStorageHandler.java

        MANAGE HERALD DIFFERENTIAL RULES
        https://reviews.facebook.net/herald/view/differential/

        WHY DID I GET THIS EMAIL?
        https://reviews.facebook.net/herald/transcript/3075/

        Tip: use the X-Herald-Rules header to filter Herald messages in your client.

        Show
        Phabricator added a comment - hobbymanyp requested code review of " HIVE-123 [jira] refactor DDL code (both DDLWork and DDLTask)". Reviewers: JIRA Make hbase handler compatible with further version of 0.89 It might be good to break DDLTask into separate tasks. The abstract class DDLWork can have various subclasses: showTablesWork, DescribeTableWork etc. and a separate task for each of them. This will make them completely independent. TEST PLAN EMPTY REVISION DETAIL https://reviews.facebook.net/D1485 AFFECTED FILES hbase-handler/src/java/org/apache/hadoop/hive/hbase/HBaseStorageHandler.java MANAGE HERALD DIFFERENTIAL RULES https://reviews.facebook.net/herald/view/differential/ WHY DID I GET THIS EMAIL? https://reviews.facebook.net/herald/transcript/3075/ Tip: use the X-Herald-Rules header to filter Herald messages in your client.
        Hide
        Ashutosh Chauhan added a comment -

        @Namit,
        Does that help?

        Show
        Ashutosh Chauhan added a comment - @Namit, Does that help?
        Hide
        Ashutosh Chauhan added a comment -

        Sure. DDLWork was sort of a container class which held various *Desc object out of which only one was active at a time. DDLTask used to check which desc of DDLWork is non-null, execute it and return. This patch makes all the *Desc in DDLWork extend from DDLDesc so DDLWork can refer to generic DDLDesc. This helps in getting rid of setting and getting of all the different Desc and thus making DDLWork extremely small and lightweight. There is one downside of it though. Instead of specific Explain annotation on get*Desc(), now there can only be a generic annotation on getDesc(). So, in explain of create Table DDL earlier used to appear "Create Table Operator" now appears "DDL Operator".

        Show
        Ashutosh Chauhan added a comment - Sure. DDLWork was sort of a container class which held various *Desc object out of which only one was active at a time. DDLTask used to check which desc of DDLWork is non-null, execute it and return. This patch makes all the *Desc in DDLWork extend from DDLDesc so DDLWork can refer to generic DDLDesc. This helps in getting rid of setting and getting of all the different Desc and thus making DDLWork extremely small and lightweight. There is one downside of it though. Instead of specific Explain annotation on get*Desc(), now there can only be a generic annotation on getDesc(). So, in explain of create Table DDL earlier used to appear "Create Table Operator" now appears "DDL Operator".
        Hide
        Namit Jain added a comment -

        Thanks for taking this, Ashutosh.
        Can you write a quick 10 line note on what this patch does ?
        It will help tremendously in the review.

        Show
        Namit Jain added a comment - Thanks for taking this, Ashutosh. Can you write a quick 10 line note on what this patch does ? It will help tremendously in the review.
        Hide
        Ashutosh Chauhan added a comment -

        Patch for parameterizing DDLWork on DDLDesc. This helps in getting rid of ton of unnecessary code.

        Show
        Ashutosh Chauhan added a comment - Patch for parameterizing DDLWork on DDLDesc. This helps in getting rid of ton of unnecessary code.
        Hide
        Namit Jain added a comment -

        That's OK - you can merge 102 for now. We can do this cleanup later. I dont think I will be
        able to look at it right now.

        Show
        Namit Jain added a comment - That's OK - you can merge 102 for now. We can do this cleanup later. I dont think I will be able to look at it right now.
        Hide
        Johan Oskarsson added a comment -

        If you want to ignore my patch and go straight to this one instead I'm fine with that, as long as something is done
        I just slapped together mine in two minutes with Eclipse, not much serious thought went into it.

        Show
        Johan Oskarsson added a comment - If you want to ignore my patch and go straight to this one instead I'm fine with that, as long as something is done I just slapped together mine in two minutes with Eclipse, not much serious thought went into it.
        Hide
        Namit Jain added a comment -

        Johan has a patch for 102, where he is breaking up DDLTask into multiple functions. This is a follow-up and requires that
        the tasks be different classes

        Show
        Namit Jain added a comment - Johan has a patch for 102, where he is breaking up DDLTask into multiple functions. This is a follow-up and requires that the tasks be different classes

          People

          • Assignee:
            Unassigned
            Reporter:
            Namit Jain
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:

              Development