From 8ef799740c18f2a98ab8458a592e8a4ef0b0ef53 Mon Sep 17 00:00:00 2001
From: Jake Wheat <jakewheatmail@gmail.com>
Date: Sat, 14 Dec 2013 11:41:58 +0200
Subject: [PATCH] refactor the combo query expression parsing and the optional
 parts of a select query expr

---
 Language/SQL/SimpleSQL/Parser.lhs | 76 ++++++++++++++++---------------
 Language/SQL/SimpleSQL/Syntax.lhs |  7 +++
 Tests.lhs                         |  4 ++
 3 files changed, 50 insertions(+), 37 deletions(-)

diff --git a/Language/SQL/SimpleSQL/Parser.lhs b/Language/SQL/SimpleSQL/Parser.lhs
index 82bf160..8140c34 100644
--- a/Language/SQL/SimpleSQL/Parser.lhs
+++ b/Language/SQL/SimpleSQL/Parser.lhs
@@ -144,15 +144,14 @@ aggregate([all|distinct] args [order by orderitems])
 >            ,Distinct <$ keyword "distinct"]
 
 parse a window call as a suffix of a regular function call
-
 this looks like this:
-
 functioncall(args) over ([partition by ids] [order by orderitems])
 
 No support for explicit frames yet.
 
 The convention in this file is that the 'Suffix', erm, suffix on
-parser names means that they have been left factored.
+parser names means that they have been left factored. These are almost
+always used with the optionSuffix combinator.
 
 > windowSuffix :: ScalarExpr -> P ScalarExpr
 > windowSuffix (App f es) =
@@ -489,7 +488,7 @@ join tref
 
 
 > from :: P [TableRef]
-> from = option [] (try (keyword_ "from") *> commaSep1 tref)
+> from = try (keyword_ "from") *> commaSep1 tref
 >   where
 >     tref = choice [try (JoinQueryExpr <$> parens queryExpr)
 >                   ,JoinParens <$> parens tref
@@ -537,19 +536,19 @@ pretty trivial.
 Here is a helper for parsing a few parts of the query expr (currently
 where, having, limit, offset).
 
-> optionalScalarExpr :: String -> P (Maybe ScalarExpr)
-> optionalScalarExpr k = optionMaybe (try (keyword_ k) *> scalarExpr)
+> keywordScalarExpr :: String -> P ScalarExpr
+> keywordScalarExpr k = try (keyword_ k) *> scalarExpr
 
-> swhere :: P (Maybe ScalarExpr)
-> swhere = optionalScalarExpr "where"
+> swhere :: P ScalarExpr
+> swhere = keywordScalarExpr "where"
 
 > sgroupBy :: P [ScalarExpr]
-> sgroupBy = option [] (try (keyword_ "group")
->                       *> keyword_ "by"
->                       *> commaSep1 scalarExpr)
+> sgroupBy = try (keyword_ "group")
+>            *> keyword_ "by"
+>            *> commaSep1 scalarExpr
 
-> having :: P (Maybe ScalarExpr)
-> having = optionalScalarExpr "having"
+> having :: P ScalarExpr
+> having = keywordScalarExpr "having"
 
 > orderBy :: P [(ScalarExpr,Direction)]
 > orderBy = try (keyword_ "order") *> keyword_ "by" *> commaSep1 ob
@@ -558,11 +557,11 @@ where, having, limit, offset).
 >              <*> option Asc (choice [Asc <$ keyword_ "asc"
 >                                     ,Desc <$ keyword_ "desc"])
 
-> limit :: P (Maybe ScalarExpr)
-> limit = optionalScalarExpr "limit"
+> limit :: P ScalarExpr
+> limit = keywordScalarExpr "limit"
 
-> offset :: P (Maybe ScalarExpr)
-> offset = optionalScalarExpr "offset"
+> offset :: P ScalarExpr
+> offset = keywordScalarExpr "offset"
 
 == common table expressions
 
@@ -581,34 +580,33 @@ and union, etc..
 
 > queryExpr :: P QueryExpr
 > queryExpr =
->   choice [select >>= queryExprSuffix, with]
+>   choice [with
+>          ,select >>= optionSuffix queryExprSuffix]
 >   where
 >     select = try (keyword_ "select") >>
 >         Select
 >         <$> (fromMaybe All <$> duplicates)
 >         <*> selectList
->         <*> from
->         <*> swhere
->         <*> sgroupBy
->         <*> having
+>         <*> option [] from
+>         <*> optionMaybe swhere
+>         <*> option [] sgroupBy
+>         <*> optionMaybe having
 >         <*> option [] orderBy
->         <*> limit
->         <*> offset
+>         <*> optionMaybe limit
+>         <*> optionMaybe offset
 
 > queryExprSuffix :: QueryExpr -> P QueryExpr
 > queryExprSuffix qe =
->     choice [(CombineQueryExpr qe
->              <$> try (choice
->                       [Union <$ keyword_ "union"
->                       ,Intersect <$ keyword_ "intersect"
->                       ,Except <$ keyword_ "except"])
->              <*> (fromMaybe All <$> duplicates)
->              <*> option Respectively
->                         (try (Corresponding
->                               <$ keyword_ "corresponding"))
->              <*> queryExpr)
->             >>= queryExprSuffix
->            ,return qe]
+>     (CombineQueryExpr qe
+>      <$> try (choice
+>               [Union <$ keyword_ "union"
+>               ,Intersect <$ keyword_ "intersect"
+>               ,Except <$ keyword_ "except"])
+>      <*> (fromMaybe All <$> duplicates)
+>      <*> option Respectively
+>                 (try (Corresponding <$ keyword_ "corresponding"))
+>      <*> queryExpr)
+>     >>= optionSuffix queryExprSuffix
 
 wrapper for query expr which ignores optional trailing semicolon.
 
@@ -632,7 +630,11 @@ trailing semicolon is optional.
 
 = lexing parsers
 
-The lexing is a bit 'virtual', in the usual parsec style.
+The lexing is a bit 'virtual', in the usual parsec style. The
+convention in this file is to put all the parsers which access
+characters directly or indirectly here (i.e. ones which use char,
+string, digit, etc.), except for the parsers which only indirectly
+access them via these functions, if you follow?
 
 > symbol :: String -> P String
 > symbol s = string s
diff --git a/Language/SQL/SimpleSQL/Syntax.lhs b/Language/SQL/SimpleSQL/Syntax.lhs
index 38044cf..b429d26 100644
--- a/Language/SQL/SimpleSQL/Syntax.lhs
+++ b/Language/SQL/SimpleSQL/Syntax.lhs
@@ -80,6 +80,10 @@
 >     | With [(String,QueryExpr)] QueryExpr
 >       deriving (Eq,Show)
 
+TODO: add queryexpr parens to deal with e.g.
+(select 1 union select 2) union select 3
+I'm not sure if this is valid syntax or not
+
 > data Duplicates = Distinct | All deriving (Eq,Show)
 > data Direction = Asc | Desc deriving (Eq,Show)
 > data CombineOp = Union | Except | Intersect deriving (Eq,Show)
@@ -104,6 +108,9 @@
 >               | JoinQueryExpr QueryExpr
 >                 deriving (Eq,Show)
 
+TODO: add function table ref
+
+
 > data JoinType = Inner | JLeft | JRight | Full | Cross
 >                 deriving (Eq,Show)
 
diff --git a/Tests.lhs b/Tests.lhs
index cdba94b..7913680 100644
--- a/Tests.lhs
+++ b/Tests.lhs
@@ -380,6 +380,10 @@
 >     ,("select a from t union distinct corresponding \
 >       \select b from u"
 >      ,CombineQueryExpr ms1 Union Distinct Corresponding ms2)
+>     ,("select a from t union select a from t union select a from t"
+>      -- is this the correct associativity? 
+>      ,CombineQueryExpr ms1 Union All Respectively
+>        (CombineQueryExpr ms1 Union All Respectively ms1))
 >     ]
 >   where
 >     ms1 = makeSelect