locked
How to use local variable in a group by clause RRS feed

  • Question

  • I have a procedure in which i am using a cursor and each time a cursor fetches next i am providing many local variables to be used in the cursor.

        DECLARE @tempTotalValue TABLE (Id int, IT float, Total float,  PlannedDate smalldatetime, PeriodStart smalldatetime)

        DECLARE curs CURSOR FOR

        SELECT
         ...,
         ...,  
         ...,
         ...,
         ...

        FROM
        Order



    Now, inside the cursor i am executing another procedure (the design of this procedure is fixed and cannot be changed). That procedure returns some value which i am insert into some table variable which i am using it further in my procedure to calculate some values and return it.

        OPEN curs

        FETCH NEXT FROM curs INTO @Id, @IP ,@FromDate, @ToDate, @Length, @Amount

        WHILE @@FETCH_STATUS = 0

            BEGIN

                DECLARE @tempIT TABLE(TI nvarchar(20), PlannedDate smalldatetime, PeriodStart smalldatetime, PeriodLength smallint, PeriodAmount float)

                INSERT INTO @tempIT EXEC IPDates @IP, @FromDate, @ToDate, @Length, @Amount //cannot change this procedure

                DELETE FROM @tempIT WHERE (YEAR(PlannedDate) < YEAR(DATEADD(YY, DATEDIFF(yy, 0, GETDATE()), 0))) OR YEAR(PlannedDate) > YEAR(DATEADD(YY, DATEDIFF(yy, 0, GETDATE()), 1));

                INSERT INTO @tempTotalValue

                SELECT     
                    @Id, // *this is the problem*
                    0,
                    PeriodAmount,
                    PlannedDate,
                    PeriodStart
                FROM
                    @tempIT
                GROUP BY
                    --@Id // Not possible but i want to do it

            FETCH NEXT FROM curs INTO @Id, @IP, @FromDate, @ToDate, @Length, @Amount

            DELETE FROM @tempIT

        END
        CLOSE curs
        DEALLOCATE curs

        Select * from @tempTotalValue
        END


    The problem i am facing is that that the one of the local variable named 'Id' is in my cursor and i am directly using that variable in the table which i am returning for my procedure. Now, i want to group by my result according to this Id. But thats not possible. For example in the picture below i want to group by the 4th and 5th row as they are of the same id and in the future when i have some values in the second (IT) column. I want to display just one idinstead of same ids multiple time and in the IT column want to show the addition of all the same id's.

    //EDIT : As suggested by Celko thats my complete procedure

    SET ANSI_NULLS ON
    GO
    SET QUOTED_IDENTIFIER ON
    GO
    CREATE PROCEDURE [TCVCalculation] @OppId as int
    AS
    BEGIN
    SET NOCOUNT ON;
        Declare @OppItemId int
        Declare @InvoicingPeriod smallint
        Declare  @FromDate smalldatetime
        Declare @ToDate smalldatetime
        Declare @FromMinDate smalldatetime
        Declare @Length int
        Declare @Amount float
        Declare @EndOfPeriod smalldatetime

    DECLARE @tempOrderTotalValue TABLE (OppItemId int, InvoiceTime float, TotalItem float,  PlannedDate smalldatetime, PeriodStart smalldatetime)

    DECLARE curs CURSOR FOR

    SELECT
        OI.Id,
        OI.InvoicingPeriod,  
        CASE OI.InvoicingPeriod
                 WHEN 110 THEN DATEADD(month, DATEDIFF(month, 0, OI.ValidUntil), 0)
                 ELSE DATEADD(month, DATEDIFF(month, 0, OI.ValidFrom), 0)
                END,
        CASE OI.InvoicingPeriod
                 WHEN 100 THEN DATEADD(month, DATEDIFF(month, 0, OI.ValidFrom), 0) -- 100 is Advance payment
                 ELSE DATEADD(month, DATEDIFF(month, 0, OI.ValidUntil), 0) -- Take care for Payment after delivery, 50% advance/50% after delivery, Prepaid and Postpaid
                END,
        0,
        OI.Quantity * OI.SellingPrice * (1-OI.Discount/100)
     
    FROM
        OppItem OI
        INNER JOIN Opp O ON OI.OppId = O.Id
    WHERE
        OI.OppId = @OppId

    OPEN curs

    FETCH NEXT FROM curs INTO @OppItemId, @InvoicingPeriod,@FromDate, @ToDate,@Length, @Amount

    WHILE @@FETCH_STATUS = 0
    BEGIN
        DECLARE @tempInvoiceTime TABLE(TimeInterval nvarchar(20), PlannedDate smalldatetime, PeriodStart smalldatetime, PeriodLength smallint, PeriodAmount float)
        INSERT INTO @tempInvoiceTime EXEC P_GetInvoicePositionDates @InvoicingPeriod, @FromDate, @ToDate, @Length, @Amount
        DELETE FROM @tempInvoiceTime WHERE (YEAR(PlannedDate) < YEAR(DATEADD(YY, DATEDIFF(yy, 0, GETDATE()), 0))) OR YEAR(PlannedDate) > YEAR(DATEADD(YY, DATEDIFF(yy, 0, GETDATE()), 1));
        
        INSERT INTO @tempOrderTotalValue
        SELECT
              @OppItemId ,
              SUM(SUM(PeriodLength)/ ISNULL(NULLIF(MAX(PeriodLength),0),1)) OVER (Partition BY @OppItemId) ,
              PeriodAmount  * SUM(SUM(PeriodLength)/ ISNULL(NULLIF(MAX(PeriodLength),0),1)) OVER (Partition BY @OppItemId) AS ItemTotal,
              PlannedDate,
              PeriodStart
        FROM
            @tempInvoiceTime
        GROUP BY
            PlannedDate,PeriodStart,PeriodAmount,PeriodLength;
            
        FETCH NEXT FROM curs INTO @OpportunityItemId, @InvoicingPeriod, @FromDate, @ToDate, @Length, @Amount
        DELETE FROM @tempInvoiceTime
    END
    CLOSE curs
    DEALLOCATE curs

    INSERT #tmpTCVCalc (OppItemId, InvoiceTime, Total)
    Select Distinct OppId, ISNULL(InvoiceTime,0), ISNULL(TotalItem,0) FROM @tempOrderTotalValue
    END

    Also, I want to use the return from this procedure into another procedure and SQL wouldnt allows me to use the nested INSERT_EXEC so i have using a Temp Table to share data. The other procedure related part looks something like

    SET ANSI_NULLS ON
    GO
    SET QUOTED_IDENTIFIER ON
    GO
    CREATE PROCEDURE OpenOppsByItems](@UserId as int)
    AS

    SET NOCOUNT ON;

    DECLARE @OppId int

    CREATE TABLE #tmpTCVCalc(OppItemId int, InvoiceTime float, Total float)
                            
    DECLARE curs CURSOR LOCAL FAST_FORWARD FOR

    SELECT
        Opp.Id        
    FROM
        Opp
        LEFT OUTER JOIN AccHasPers ON Opp.ContactId = AcctHasPers.PersonId

    OPEN curs

    FETCH NEXT FROM curs INTO @OppId;

    WHILE @@FETCH_STATUS = 0
    BEGIN

        EXEC TCVCalculationForOpp @OppId;
        
    FETCH NEXT FROM curs INTO @OppId;

    END
    CLOSE curs;
    DEALLOCATE curs;



                                           
    Saturday, April 6, 2013 3:11 PM

All replies

  • There is no need to group by a constant or a local variable.  If you want to aggregate rows in your select result, then every column in the select list must either be a constant, a local variable, in the group by clause, or be an aggregate function.

    You say you want to (for example) combine the row 4 and 5 in the above, to do that you need to have aggregate functions on PeriodAmount, PlannedDate and PeriodStart.  I presume you want SUM for PeriodAmount, I'm not sure what you want for the aggregate function for the other two columns.  But, for example, if you wanted MIN(PlannedDate) and MAX(PeriodStart), then you can do

                INSERT INTO @tempTotalValue
        
                SELECT     
                    @Id,
                    0,
                    SUM(PeriodAmount),
                    MIN(PlannedDate),
                    MAX(PeriodStart)
                FROM
                    @tempIT

    Note that since every column is a constant, local variable, or an aggregate function, you do not need an ORDER BY clause.

    Tom

    Saturday, April 6, 2013 3:36 PM
  • >> I have a procedure in which I am using a cursor and each time a cursor fetches next I am providing many local variables to be used in the cursor. <<

    Good SQL programmers do not use cursors. My rule of thumb is that you will use no more than five of them in your entire career. Cursors are how nion-SQL programmers mimic magnetic tape file systems in SQL. 

    Functional/declarative programming languages do not use local variables. Local tables are even worse; they are how non-SQL programmers mount those magnetic tapes . 

    There is no magical universal “id” in RDBMS; it has to be the identifier of something in particular. There is no magical universal “total” in RDBMS; it has to be the total of something in particular. We do not use FLOAT for money; it is illegal and does not work anyway. Table names are plural or collective because they model sets. Files use the name of a record because they process data record-by-record. That is why you have “Order” and an SQL programmer would have “Orders” instead. Why are you using // for comments? SQL uses dashes. Wasn't the double slash used by Autocoder with the early tape systems based on IBM's OS/JCL? 

    “EXEC IPDates” is another way to keep using COBOL in SQL. An SQL Programmers would have a CTE, VIEW or subquery. And in keep with the COBOL mindset, I see you use temporal to string or temporal to numeric functions. That is 1960's COBOL, which does not have temporal data types like SQL. 

    Having some experience fixing bad SQL, I will guess that code is 2-3 orders of magnitude slower and bigger than it needs to be and that it has no data integrity. 

    Your entire approach to RDBMS is fundamentally wrong. You have no idea how to do a data model. You do not know any of the coding practices in RDBMS (ISO-11179, ISO-8601, etc). Since you did not post any DDL, we cannot help you learn to do this right. 

    Look at my credentials, and see if you think I might be right. If you want to follow Netiquette and post DDL with specs, we can try to replace this mess. 



    --CELKO-- Books in Celko Series for Morgan-Kaufmann Publishing: Analytics and OLAP in SQL / Data and Databases: Concepts in Practice Data / Measurements and Standards in SQL SQL for Smarties / SQL Programming Style / SQL Puzzles and Answers / Thinking in Sets / Trees and Hierarchies in SQL

    Saturday, April 6, 2013 6:22 PM
  • Thanks Celko, For pointing out so many mistakes. I am new to Databases in general and SQL Server in particular.

    I agree with you that i have also come across 'dont use cursors' but what to replace them with is what i do not know. Same goes for local tables and local variables.

    Thanks for also pointing out some naming mistakes and commenting rules.

    I am totally convinced by what you have said and want to replace this mess with something efficient but am not completely sure what do you mean by 'post DDL with specs'. Anyways, I will edit my question above with some more details about the procedure. If you need something more that that you can tell me and i will add that information too.

    Thanks and have a nice day

    Sunday, April 7, 2013 2:20 PM
  • but am not completely sure what do you mean by 'post DDL with specs'.

    DDL = Data Definition Language. That is CREATE TABLE statements for your tables, including definition of keys etc.

    I don't think it's needed in this particular case; it seems that you got good answers from other people in the thread. (But there are certainly questions, where it is a good idea to supply this.)

    To avoid the cursor, you would have to replace the stored procedure with one that operates on the full set of the data. This may be non-trivial.

    Overall, I recommend that you ignore this Celko figure. His answers are far from always to the point. Particularly, he has little sense for the practical situation we might face as a programmer.


    Erland Sommarskog, SQL Server MVP, esquel@sommarskog.se
    Sunday, April 7, 2013 4:47 PM