method index-add specialized on class hash-list-index - CL:PUSH vs. CL:PUSHNEW #3

Open
mon-key opened this Issue Jun 19, 2012 · 2 comments

Comments

Projects
None yet
2 participants

mon-key commented Jun 19, 2012

The method index-add specialized on class hash-list-index in bknr-datastore/src/indices/indices.lisp
uses CL:PUSH as follows:

(push object (gethash key hash-table))

I would think CL:PUSHNEW would be a better choice.

As it is, updating the slot-value of an indexed slot with a list containing duplicate keys causes the object to appear multiple times in the corresponding hash-value list associated with the duplicated key in slot-index-hash-table. Following is an example:

(defclass tt--indexed-record ()
  ((id-string 
    :initarg          :id-string
    :accessor         id-string
    :index-type       string-unique-index
    :index-reader     tt--indexed-record-with-id-string
    :index-values     tt--indexed-record-all-id-string
    :index-mapvalues  tt--indexed-record-map-id-string
    :index-subclasses t)
   (entity-name-coref
    :initarg         :entity-name-coref
    :initform        (list nil)
    :accessor        entity-name-coref
    :index-type      hash-list-index
    :index-initargs  (:test 'equal :index-nil t)
    :index-reader    tt--indexed-record-with-entity-name-coref
    :index-values    tt--indexed-record-all-entity-name-coref
    :index-mapvalues tt--indexed-record-map-entity-name-coref
    :index-subclasses t))
  (:metaclass bknr.indices:indexed-class))

(make-instance 'tt--indexed-record  :id-string "000042")
;=> #<TT--INDEXED-RECORD {CAB7131}>

(tt--indexed-record-with-entity-name-coref nil)
;=> (#<TT--INDEXED-RECORD {CAB7131}>), T

(setf (entity-name-coref (tt--indexed-record-with-id-string "000042"))
      (list "Jangles, Bo" "Gurdy, Hurdy"))
;=> ("Jangles, Bo" "Gurdy, Hurdy")

(tt--indexed-record-with-entity-name-coref "Jangles, Bo")
;=> (#<<TT--INDEXED-RECORD {CAB7131}>), T

(tt--indexed-record-with-entity-name-coref "Gurdy, Hurdy")
;=> (#<TT--INDEXED-RECORD {CAB7131}>), T

(setf (entity-name-coref (tt--indexed-record-with-id-string "000042"))
      (list "Gurdy, Hurdy"))
;=> ("Gurdy, Hurdy")

(tt--indexed-record-with-entity-name-coref "Jangles, Bo")
;=> NIL, NIL

(tt--indexed-record-with-entity-name-coref "Gurdy, Hurdy")
;=> (#<TT--INDEXED-RECORD {CAB7131}>), T

(setf (entity-name-coref (tt--indexed-record-with-id-string "000042"))
      (list NIL "Gurdy, Hurdy" NIL "Gurdy, Hurdy"))
;=> (NIL "Gurdy, Hurdy" NIL "Gurdy, Hurdy")

The following two return values are IMHO unnecessarily pathological:

(tt--indexed-record-with-entity-name-coref "Gurdy, Hurdy")
;=> (#<TT--INDEXED-RECORD {CAB7131}> #<TT--INDEXED-RECORD {CAB7131}>), T

(tt--indexed-record-with-entity-name-coref NIL)
;=> (#<TT--INDEXED-RECORD {CAB7131}> #<TT--INDEXED-RECORD {CAB7131}>), T
Owner

hanshuebner commented Jun 30, 2012

Sorry that it took me a while to look at this.

I do not think that hash-list-index should treat the list in the slot as a set - If it would, it should be named hash-set-index.

Changing the push to a pushnew would alter the performance profile of the index drastically, as insertions would become slow with large lists.

I would favor the addition of a hash-set-index if that is what you're really looking for.

mon-key commented Jul 12, 2012

"Sorry that it took me a while to look at this."

NP

"Changing the push to a pushnew would alter the performance profile of the index drastically, as insertions would become slow with large lists."

OK, makes sense i hadn't really considered just how much of a difference such a change might incur.

"I would favor the addition of a hash-set-index if that is what you're really looking for."

The addition of a HASH-SET-INDEX may indeed be what I am looking for. At the moment I'm working on a separate (albeit related) project. I would prefer not to pursue such an addition without first giving the matter further consideration - I'm sure neither of us want to engage the potential overhead of such an addition unless it is really wanted/needed. IOW, let me get back to you on that :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment