fukamachi / cl-dbi

Database independent interface for Common Lisp
202 stars 28 forks source link

Please Consider Converting `prepare-sql` from Variadic to take in a list #60

Closed kat-co closed 4 years ago

kat-co commented 4 years ago
(defgeneric prepare-sql (conn sql)
  (:documentation
   "Create a function that takes parameters, binds them into a query and returns SQL as a string.")
  (:method ((conn dbi-connection) (sql string))
    (labels ((param-to-sql (param)
               (typecase param
                 (string (concatenate 'string "'" (escape-sql conn param) "'"))
                 (null "NULL")
                 (t (princ-to-string param)))))
      (let ((sql-parts (split-sequence #\? sql)))
        (lambda (&rest params)
          (if params
              (with-output-to-string (out)
                (loop for (part . rest) on sql-parts
                      do
                         (let ((param (pop params)))
                           (write-sequence
                            (if rest
                                (concatenate 'string part (param-to-sql param))
                                part)
                            out))))
              sql))))))

https://github.com/fukamachi/cl-dbi/blob/master/src/driver.lisp#L415

I am running into a confirmed bug on SBCL wherein mid-iteration in the loop, the params list suddenly gets set to 0. The advice I've received from an SBCL developer is that passing large numbers of arguments within a &rest variable is not very portable Common Lisp because of call-arguments-limit.

For performance reasons, users (such as myself) may want many bind parameters to take advantage of so-called extended inserts, e.g.:

INSERT INTO tbl_name (a,b,c) VALUES(?,?,?),(?,?,?),(?,?,?),(?,?,?),...

When this is the case, cl-dbi is artificially coupling how many &rest parameters can be passed with how many extended inserts a DB can support thus potentially artificially limiting an application's performance. Further, it becomes more difficult to write portable code because of this implementation detail.

Please consider changing this lambda so that it takes in a list.

Thank you for the library!

fukamachi commented 4 years ago

Thank you for the suggestion. Sounds good to me. NOTE: Confirmed that prepare-sql is an internal method and only be used in MySQL driver.

kat-co commented 4 years ago

Thank you!