Projection: <field>: <1 or true> and <field>: <0 or false>
#377
Conversation
| @@ -19,5 +19,15 @@ import "github.com/FerretDB/FerretDB/internal/types" | |||
| // ProjectDocuments modifies given documents in places according to the given projection. | |||
| func ProjectDocuments(docs []*types.Document, projection *types.Document) error { | |||
| // TODO | |||
|
|
|||
| projectionMap := projection.Map() | |||
There was a problem hiding this comment.
it can be done w/o creation of new variable..
| @@ -100,9 +100,6 @@ func (s *storage) MsgCount(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg, e | |||
| resDocs = append(resDocs, doc) | |||
| } | |||
|
|
|||
| if err = common.SortDocuments(resDocs, sort); err != nil { | |||
| return nil, err | |||
| } | |||
There was a problem hiding this comment.
If count has limit (i.e. "no more than", then sort makes sense..)
There was a problem hiding this comment.
I'd better won't change it here.
There was a problem hiding this comment.
No, sorting is really not required. But that's not for that PR
| @@ -99,7 +99,6 @@ func (s *storage) MsgCount(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg, e | |||
|
|
|||
| resDocs = append(resDocs, doc) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Let's keep a blank line there to separate that block
| @@ -100,9 +100,6 @@ func (s *storage) MsgCount(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg, e | |||
| resDocs = append(resDocs, doc) | |||
| } | |||
|
|
|||
| if err = common.SortDocuments(resDocs, sort); err != nil { | |||
| return nil, err | |||
| } | |||
There was a problem hiding this comment.
No, sorting is really not required. But that's not for that PR
| @@ -45,7 +45,7 @@ func (s *storage) MsgCount(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg, e | |||
| "let", | |||
| } | |||
| if err := common.Unimplemented(document, unimplementedFields...); err != nil { | |||
| return nil, err | |||
There was a problem hiding this comment.
No, Unimplemented returns protocol error that we should return as-is to the client
| projectionMap := projection.Map() | ||
| for i := 0; i < len(docs); i++ { | ||
| for k := range docs[i].Map() { | ||
| if _, ok := projectionMap[k]; !ok { |
There was a problem hiding this comment.
We should check the value too. For example, 0 or false should remove the field.
Or the plan is to do that in a different PR?
There was a problem hiding this comment.
Then I'd add new tests for it too.
There was a problem hiding this comment.
I'd rather add tests and add code
There was a problem hiding this comment.
In this PR, or in different one?
seeforschauer
changed the title
<field>: <1 or true> and <field>: <0 or false>
seeforschauer
changed the title
<field>: <1 or true> and <field>: <0 or false><field>: <1 or true> and <field>: <0 or false>
| projectionMap := projection.Map() | ||
| for i := 0; i < len(docs); i++ { | ||
| for k := range docs[i].Map() { | ||
| if k == "_id" { |
There was a problem hiding this comment.
_id handing is more complicated than that
|
|
||
| v, ok := projectionMap[k] | ||
| if !ok { | ||
| docs[i].Remove(k) |
There was a problem hiding this comment.
Do we have a dance test for that? It is not clear from documentation how MongoDB behaves there
There was a problem hiding this comment.
no, dance has no tests for it at all,
can it be merged to not to stop Dmitry?
| docs[i].Remove(k) | ||
| continue | ||
| } | ||
| continue |
There was a problem hiding this comment.
We don't need both those continue statements
| case int32, int64, float32, float64: | ||
| if compareScalars(v, int32(0)) == equal { | ||
| docs[i].Remove(k) | ||
| continue |
dance FerretDB/dance#83