core.logic

Rel relation PersistentHashSet becomes LazySeq after issuing a retraction

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Patch:
    Code

Description

The first retraction of facts from a relation transforms the internal PersistentHashSet -set var+atom, which guarantees uniqueness, into a LazySeq which allows duplicate facts to be added. Once the first retraction occurs, a LazySeq persists for the rest of the life of the relation. Only the value of the primary -set var+atom is affected, not the -index var+atom values.

This issue is not an external correctness issue but does affect performance of subsequent duplicate fact additions which grow the size relation.

Preparation:

user=> (defrel foo x y)
#<user$eval4287$fn__4288 user$eval4287$fn__4288@1a9d489b>
user=> (facts foo [[:joe :man][:jane :woman][:sue :boy]])
nil
user=> foo_2-set
#<Atom@52aaf223: #{[:joe :man] [:jane :woman] [:sue :boy]}>
user=> (class @foo_2-set)
clojure.lang.PersistentHashSet
user=> (retraction foo :jane :woman)
nil

Without patch applied:

user=> foo_2-set
#<Atom@52aaf223: ([:joe :man] [:sue :boy])>
user=> (class @foo_2-set)
clojure.lang.LazySeq
user=> (facts foo [[:joe :man][:jane :woman][:sue :boy]])
nil
user=> foo_2-set
#<Atom@52aaf223: ([:sue :boy] [:jane :woman] [:joe :man] [:joe :man] [:sue :boy])>
user=> (class @foo_2-set)
clojure.lang.LazySeq

With patch applied:

user=> foo_2-set
#<Atom@31eb9b15: #{[:joe :man] [:sue :boy]}>
user=> (class @foo_2-set)
clojure.lang.PersistentHashSet
user=> (facts foo [[:joe :man][:jane :woman][:sue :boy]])
nil
user=> foo_2-set
#<Atom@31eb9b15: #{[:joe :man] [:jane :woman] [:sue :boy]}>
user=> (class @foo_2-set)
clojure.lang.PersistentHashSet

I've filed this as a Minor issue as it does not affect core.logic correctness and degraded performance can be avoided by not re-asserting duplicate facts.

I will also issue a GitHub pull request which can be used or ignored at your convenience.

Activity

Hide
David Nolen added a comment -
Show
David Nolen added a comment - fixed, http://github.com/clojure/core.logic/commit/9bc6eb42be28bfd2b493657344f6eea8f5ed657c. pushing out a 0.8.0-alpha3 release as well.
Hide
Aaron Brooks added a comment -

My apologies for the extra run-around here. I've attached an -002 version of the patch created with git-format-patch which should be amenable to git-am.

Show
Aaron Brooks added a comment - My apologies for the extra run-around here. I've attached an -002 version of the patch created with git-format-patch which should be amenable to git-am.
Hide
David Nolen added a comment -

Yes the patch is not properly formatted and we're not supposed to take pull requests. The patch is missing attribution info. I normally apply patches with "git am foo.patch". I used the following guide for figuring out how to generate patches that can be applied with "git am", http://ariejan.net/2009/10/26/how-to-create-and-apply-a-patch-with-git.

Show
David Nolen added a comment - Yes the patch is not properly formatted and we're not supposed to take pull requests. The patch is missing attribution info. I normally apply patches with "git am foo.patch". I used the following guide for figuring out how to generate patches that can be applied with "git am", http://ariejan.net/2009/10/26/how-to-create-and-apply-a-patch-with-git.
Hide
Aaron Brooks added a comment -

The attached patch should work fine with git-apply (in either "cat foo.patch |git apply" or "git apply foo.patch" form). I made the GitHub pull request as I figured that would be the easiest path to pull the changes in.

Show
Aaron Brooks added a comment - The attached patch should work fine with git-apply (in either "cat foo.patch |git apply" or "git apply foo.patch" form). I made the GitHub pull request as I figured that would be the easiest path to pull the changes in.
Hide
David Nolen added a comment -

I tried to apply the patch but I'm getting a "Patch format detection failed".

Show
David Nolen added a comment - I tried to apply the patch but I'm getting a "Patch format detection failed".
Hide
David Nolen added a comment -

Thanks for the information. Will apply to master. I actually run tests with "mvn test", I've updated :source-paths so other people can run the tests with lein.

Show
David Nolen added a comment - Thanks for the information. Will apply to master. I actually run tests with "mvn test", I've updated :source-paths so other people can run the tests with lein.
Hide
Aaron Brooks added a comment -

I forgot to mention that I needed to add "src/test/clojure" to the project.clj :source-paths vector to get lein1/2's test command to run the tests. Is that expected? Is there another way to run the tests that I'm missing? I'm happy to file a separate ticket/patch to address this if project.clj needs to be modified.

Show
Aaron Brooks added a comment - I forgot to mention that I needed to add "src/test/clojure" to the project.clj :source-paths vector to get lein1/2's test command to run the tests. Is that expected? Is there another way to run the tests that I'm missing? I'm happy to file a separate ticket/patch to address this if project.clj needs to be modified.
Hide
Aaron Brooks added a comment -

(cf. here and GitHub I'll keep this thread on JIRA.) Yes, this is targeted for master. I don't know if this warrants a release unto itself.

Show
Aaron Brooks added a comment - (cf. here and GitHub I'll keep this thread on JIRA.) Yes, this is targeted for master. I don't know if this warrants a release unto itself.
Hide
David Nolen added a comment -

Thanks! Is this meant to be applied to master?

Show
David Nolen added a comment - Thanks! Is this meant to be applied to master?

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: