Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: pull: attr-option doesn't work inside map-spec #578

Closed
KsVlad opened this issue Nov 8, 2022 · 7 comments · Fixed by #595
Closed

[Bug]: pull: attr-option doesn't work inside map-spec #578

KsVlad opened this issue Nov 8, 2022 · 7 comments · Fixed by #595
Assignees
Labels
bug Something isn't working datomic compat

Comments

@KsVlad
Copy link

KsVlad commented Nov 8, 2022

What version of Datahike are you using?

0.6.1521

What version of Java are you using?

openjdk version "19.0.1" 2022-10-18

What operating system are you using?

Arch WSL (Windows 11)

What database EDN configuration are you using?

{:store {:backend :mem}}

Describe the bug

In pull pattern attr-option (:as, :limit) works:

(pull (db conn) '[(:attr :as "attr")] id)

but attr-option in map-spec throws with Expected (attr-name | limit-expr):

(pull (db conn) '[{(:attr :as "attr") [*]}] id)

See example in "How can the behaviour be reproduced?"

N.B. legacy-limit-expr in terms of Datomic works (e.g. (pull (db conn) '[{(limit :attr 1) [*]}] id)), so it's clear what exception is talking about.

What is the expected behaviour?

(pull (db conn) '[{(:attr :as "attr") [*]}] id)

Returns result with :attr substituted with "attr"

How can the behaviour be reproduced?

(def cfg {:store {:backend :mem}})
(def conn (d/connect cfg))

For schema:

(def schema [{:db/ident :name
              :db/valueType :db.type/string
              :db/cardinality :db.cardinality/one
              :db/unique :db.unique/identity}
             {:db/ident :friend
              :db/valueType :db.type/ref
              :db/cardinality :db.cardinality/one}])
(d/transact conn schema)

And seed data:

(d/transact conn [{:db/id -1
                   :name "Joe"}
                  {:name "Jane"
                   :friend -1}])

This pull works:

(d/pull (d/db conn) '[(:friend :as "body")] [:name "Jane"])
=> {"body" #:db{:id 3}}

This one doesn't:

(d/pull (d/db conn) '[{(:friend :as "body") [*]}] [:name "Jane"])
=> 
; Execution error (ExceptionInfo) at datalog.parser.pull/parse-map-spec-entry (pull.cljc:117).
; Expected (attr-name | limit-expr)

(Both works in Datomic dev-local)

@KsVlad KsVlad added bug Something isn't working triage labels Nov 8, 2022
@KsVlad KsVlad changed the title [Bug]: [Bug]: pull: attr-option doesn't work inside map-spec Nov 8, 2022
@jsmassa
Copy link
Collaborator

jsmassa commented Nov 8, 2022

Thank you for reporting this :)

It looks like our parser is not up-to-date with the Datomic pull grammar.
I can see this also has already been reported in replikativ/datalog-parser#25.

We should be able to fix this soon.

@jsmassa jsmassa self-assigned this Nov 8, 2022
@KsVlad
Copy link
Author

KsVlad commented Nov 9, 2022

Also, have discovered only now, default-expr is not supported in simple attr-expr

(d/pull (d/db conn) '[(:misssing :default "default")] [:name "Jane"])
>
; Execution error (ExceptionInfo) at datahike.db.utils/validate-attr-ident (utils.cljc:157).
; Bad entity attribute :misssing at (resolve-datom db 4 :misssing nil nil), not defined in current schema

Seems like a different kind of error.

Please tell, is it worth a separate issue? Or maybe it is somewhat intended difference from datomic

@KsVlad
Copy link
Author

KsVlad commented Nov 9, 2022

Parser knows about :default

(d/pull (d/db conn) '[(:friend :default "default")] [:name "Jane"])
>
{:friend #:db{:id 3}}

But seems like a strange feature to have for only present on entity attributes :)

@KsVlad
Copy link
Author

KsVlad commented Nov 9, 2022

Oops, my bad
Bad entity attribute :misssing at (resolve-datom db 4 :misssing nil nil), **not defined in current schema**

I will read messages properly. With :misssing property defined everything works fine.

Anyway, assuming the label "datomic compat", some validity of question remain, because in datomic :defaults works even on properties not in the schema.

Now I'm stop spamming here. Please, let me know if there is a need to make a proper issue for :default thing.

@jsmassa
Copy link
Collaborator

jsmassa commented Nov 9, 2022

Allowing :default values also on attributes not in the schema is indeed a separate issue that should be discussed.

We usually try to stay compatible with Datomic, so I guess we will want to support that sooner or later, but to me it seems like a strange feature to have, so that wouldn't be exactly a priority.

@jsmassa
Copy link
Collaborator

jsmassa commented Nov 9, 2022

Btw, replikativ/datalog-parser#27 will update the parser and as soon as this is included in datahike the options should work like in datomic again.

@KsVlad
Copy link
Author

KsVlad commented Nov 10, 2022

We usually try to stay compatible with Datomic, so I guess we will want to support that sooner or later, but to me it seems like a strange feature to have, so that wouldn't be exactly a priority.

Agree with "strange" here. Throw in the face if I misspell some attribute seems like more preferable behavior to me.
It turned out It's more than :default. Datomic pull silently omits :not-in-schema on simple (pull db [:not-in-schema] id)

So, here is an #579 . A good candidate for wontfix it is, but let it be for future reference :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working datomic compat
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants