MS AdventureWorks dbo.ufnGetProductListPrice

187 views Asked by At

I'm studing the Ms SQL AdventureWorks 2014, to model an internal dbase for our company

I usually work on Postgres, and I'm trying to "understand" Ms SQL stored procedures... :-)) BUT ....

The store Procedure [dbo.ufnGetProductListPrice ] SEEMS STRANGE to me. the SQL code is found here: (https://dataedo.com/samples/html/AdventureWorks/doc/AdventureWorks_2/functions/dbo_ufnGetProductListPrice_116.html)

 CREATE FUNCTION [dbo].[ufnGetProductListPrice](@ProductID [int], @OrderDate [datetime])
RETURNS [money] 
AS 
BEGIN
    DECLARE @ListPrice money;

    SELECT @ListPrice = plph.[ListPrice] 
    FROM [Production].[Product] p 
        INNER JOIN [Production].[ProductListPriceHistory] plph 
        ON p.[ProductID] = plph.[ProductID] 
            AND p.[ProductID] = @ProductID 
            AND @OrderDate BETWEEN plph.[StartDate] AND COALESCE(plph.[EndDate], CONVERT(datetime, '99991231', 112)); -- Make sure we get all the prices!

    RETURN @ListPrice;
END;

The function is making use of the following two tables:

In particular my doubts are:

  1. the function is getting the parameter @ProductID. The same @ProductID is used as the primary key of the Production.Product AND as part of the primary key for the table Production.ProductListPriceHistory so is seems of no help making a join on Product.[ProductID] = ProductListPriceHistory.[ProductID] when we can test the ProductID directly on the ProductListPriceHistory.[ProductID]
    Why to create such a join ? Seems of no help...

  2. The given @Orderdate datetime received as second parameter, is checked in the JOIN against the following condition

AND @OrderDate BETWEEN plph.[StartDate] AND COALESCE(plph.[EndDate], CONVERT(datetime, '99991231', 112)); -- Make sure we get all the prices!

BUT, if we are calling the store procedure for @Orderdate = 1/1/2022,

  • considering that [EndDate] could be NULL,
  • and we could have in ProductListPriceHistory.StartDate two records with our @ProductID, the first with StartDate=1/1/2020 and the second StartDate=1/1/2021,

such "BETWEEN" condition should match both of them, when obviously we would expect the last one .... is it a bug ?

2

There are 2 answers

2
Charlieface On

You are right, there are a number of serious flaws in this code, and I would recommend finding better tutorials instead.

I've noted comments on each flaw

CREATE FUNCTION [dbo].[ufnGetProductListPrice](@ProductID [int], @OrderDate [datetime])
RETURNS [money]   -- money is a bad data type due to rounding problems, decimal should be used
                  -- scalar UDFs are very slow, this should be a inline Table Valued Function
AS 
BEGIN
    DECLARE @ListPrice money;

    SELECT @ListPrice = plph.[ListPrice]    -- as you point out, there should be some kind of aggregation
    FROM [Production].[Product] p           -- as you point out: join is unnecessary
        INNER JOIN [Production].[ProductListPriceHistory] plph 
        ON p.[ProductID] = plph.[ProductID] 
            AND p.[ProductID] = @ProductID 
                           -- BETWEEN should never be used on dates, use ">= AND <"
            AND @OrderDate BETWEEN plph.[StartDate] AND COALESCE(plph.[EndDate], CONVERT(datetime, '99991231', 112));   -- ISNULL is better for performance than COALESCE

    RETURN @ListPrice;
END;

A better function would be this

CREATE FUNCTION dbo.ufnGetProductListPrice (
  @ProductID int,
  @OrderDate datetime
)
RETURNS TABLE
AS RETURN

SELECT TOP (1)
  plph.ListPrice
FROM Production.ProductListPriceHistory plph 
WHERE plph.ProductID = @ProductID 
  AND @OrderDate >= plph.StartDate
  AND @OrderDate < ISNULL(plph.EndDate, CONVERT(datetime, '99991231', 112))
ORDER BY
  plph.StartDate DESC;

Assuming it's possible for there to be multiple active prices (I don't think it's possible in AdventureWorks), then you also need TOP (1) and ORDER BY plph.StartDate DESC. If this is not possible then you can leave that out.


Instead of doing this with a scalar UDF

SELECT p.*, dbo.ufnGetProductListPrice(p.ProductId, GETDATE())
FROM Production.Product p;

You use an APPLY for a TVF

SELECT p.*, plp.*
FROM Production.Product p
OUTER APPLY dbo.ufnGetProductListPrice(p.ProductId, GETDATE()) plp;

An OUTER APPLY works like a LEFT JOIN, which means you get NULL if there are no rows.

1
Giuliano69 On

@Charlieface Thanks for your reply. One more note on the function ufnGetProductStandardCost

AdventureWorks is storing historical Retailprices and CostPrices, to let you found the price (list, cost) of a certain product at a certain time. This is why I'm interested in this part of the database

It even offers a "dual" function for Cost prices ufnGetProductStandardCost https://dataedo.com/samples/html/AdventureWorks/doc/AdventureWorks_2/functions/dbo_ufnGetProductStandardCost_117.html
with the same errors :-(

I think that the idea of ufnGetProductListPrice(ProductID, OrderDate), is to report a SINGLE price or Null if at that given time the price did not exist.
NOT a table or listrow.

Let's just recall the case I supposed before

  • search OrderDate 1/1/2022
  • one record with StartDate=1/1/2020 , EndDate=null
  • one record with StartDate=1/1/2021 , EndDate=null

One idea for such function would be to:

  • use a subquery to find the MAX() StardDate<@OrderDate for the given @ProductId
  • use such StartDate to get the correspondig ListPrice in the external query or give back NULL if we have NO StartDate ListPrice before @OrderDate (e.g @OrderDate=1/1/2019)

What do you think about this query ? Any improvement ?

CREATE FUNCTION dbo.ufnGetProductListPrice (
  @ProductID int,
  @OrderDate datetime
)
AS 
BEGIN

DECLARE @ListPrice money;

SELECT @ListPrice= plph.ListPrice
FROM Production.ProductListPriceHistory plph 
WHERE plph.ProductID = @ProductID 
  AND plhp.StartDate = (
        SELECT MAX(StardDate)
        FROM Production.ProductListPriceHistory plph1
        WHERE plph1.ProductID = @ProductID
            AND plhp1.StartDate <=  @OrderDate)         
RETURN @ListPrice;
END;