[codeface] Re: [PATCH 1/2] Add aggregate option to db query

  • From: Wolfgang Mauerer <wolfgang.mauerer@xxxxxxxxxxxxxxxxx>
  • To: <codeface@xxxxxxxxxxxxx>, "Joblin, Mitchell (ext)" <mitchell.joblin.ext@xxxxxxxxxxx>
  • Date: Tue, 20 Oct 2015 12:46:42 +0200

Hi Mitchell,

thanks for the patches; I have a few nitpicks on this one.

Am 15/10/2015 um 10:48 schrieb Mitchell Joblin:

- Retrieving the whole commit table is quite memory intensive
when one only wants the commit counts

Signed-off-by: Mitchell Joblin <mitchell.joblin.ext@xxxxxxxxxxx>
---
codeface/R/query.r | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/codeface/R/query.r b/codeface/R/query.r
index c8309e4..9142cb8 100644
--- a/codeface/R/query.r
+++ b/codeface/R/query.r
@@ -182,17 +182,29 @@ get.commits.by.ranges <- function(conf, subset=NULL,
FUN=NULL) {
}

get.commits.by.date.con <- function(con, pid, start.date, end.date,
- commit.date=TRUE) {
+ commit.date=TRUE,
+ commit.count=FALSE) {
Why a separate line for each optional argument in
the parameter list?
if (commit.date==TRUE) {
date.type <- "commitDate"
} else {
date.type <- "authorDate"
}

- query <- str_c("SELECT * FROM commit",
+ if (commit.count==TRUE) {
+ select <- "SELECT author, COUNT(*) as freq"

I _think_ there's a standard package that equips R with
a select language feature. While there is no direct
clash, how about using "query" instead of "select"?
This would also be consistent with other cases in query.r
where we construct the select statement dynamically.

Of course, we could also unify all such uses to select.stmt.

+ group.by <- " GROUP BY author"
+ } else {
+ select <- "SELECT *"
+ group.by <- NULL
+ }
+
+ query <- str_c(select,
+ " FROM commit",
" WHERE projectId=", pid,
" AND ", date.type, ">=", sq(start.date),
- " AND ", date.type, "<", sq(end.date))
+ " AND ", date.type, "<", sq(end.date),
+ group.by)
+

Reviewed-by: Wolfgang Mauerer <wolfgang.mauerer@xxxxxxxxxxxxxxxxx>

dat <- dbGetQuery(con, query)

return(dat)


Other related posts: