unix-timestamp->honeysql implementation for h2 is incorrect for microseconds

[jeff303]

The current implementation has:

(defmethod sql.qp/unix-timestamp->honeysql [:h2 :millisecond] [_ _ expr]
  (add-to-1970 expr "millisecond"))

(defmethod sql.qp/unix-timestamp->honeysql [:h2 :millisecond] [_ _ expr]
  (add-to-1970 expr "microsecond"))

The 2nd param to the last one should clearly be :microseconds, and the first should be :milliseconds (plural). Unclear why current test suite doesn't fail from this.

[flamber]

H2 understands this magically - at least based on @dpsutton's description in #14024

[jeff303]

H2 understands this magically - at least based on @dpsutton's description in #14024

Yeah it may understand that keyword, but this is all Clojure stuff before we even call it. In fact the second defmethod is completely overwriting the first (currently) because it declares the exact same dispatch vals. And in addition, a dispatch val of :millisecond (as opposed to :milliseconds) is not used anywhere else for the same multimethod, which is very suspicious.

[dpsutton]

His point is that these two implementations clobber each other. it's the duplicate [:h2 :millisecond]. And certainly one of those should be [:h2 :microseconds]. Good catch @jeff-bruemmer

[dpsutton]

@jeff303 are you gonna do a quick patch or want me to?

[jeff303]

@jeff303 are you gonna do a quick patch or want me to?

Go ahead if you're interested. I just happened to notice while knee deep in something else and wanted to capture it.