revisit the QueryResult interface

[travisturner]

We may be able to simplify the QueryResult interface.

// QueryResult represents one of the results in the response.
type QueryResult interface {
	Type() uint32
	Bitmap() BitmapResult
	CountItems() []CountResultItem
	Count() int64
	Sum() int64
	Changed() bool
}

Because each function is specific to the type, it's effectively an empty interface. And every implementation is left with unused methods:

func (BitmapResult) Type() uint32                  { return QueryResultTypeBitmap }
func (b BitmapResult) Bitmap() BitmapResult        { return b }
func (BitmapResult) CountItems() []CountResultItem { return nil }
func (BitmapResult) Count() int64                  { return 0 }
func (BitmapResult) Value() int64                  { return 0 }
func (BitmapResult) Changed() bool                 { return false }

I could see it being something like this instead:

type QueryResult interface{}

type BitmapResult struct {
    QueryResult

    Attributes map[string]interface{} `json:"attrs"`
    Bits       []uint64               `json:"bits"`
    Keys       []string               `json:"keys"`
}

In this case the QueryResult inside the BitmapResult is not really necessary because everything implements the empty interface, but if for some reason QueryResult needs to have a function defined, this would ensure BitmapResult implements the interface.

This is not at all a high priority, but may be worth revisiting at some point.

[yuce]

This would require the user to cast a QueryResult to something else, e.g., BitmapResult before accessing its fields, wouldn't it ?

[travisturner]

Yes, but because each interface method is specific to the type, we're effectively doing that anyway. For example, we're only using Bitmap() on a BitmapResult. I guess what I'm suggesting is that if every implementation of an interface only uses 1 or 2 of the 5 methods, then that's not a very good use of interfaces. Also, one of the methods of this interface is Type which means we're still doing a type switch somewhere.

Again, this is not high priority because it works, but I wanted to make a note that I think that it could be simplified.

[yuce]

We don't do a type check, but we use the Type field of the protobuf result to create the correct type in the newQueryResultFromInternal function. I like how the empty interface makes our code shorter, but that makes the user code longer.

I think we will be able to change our our implementation in the future without breaking user code when we need to.

[yuce]

Can we close this?

[travisturner]

I believe this still applies. It has been iceboxed because it's not a priority, but I think it's still worth reconsidering the way this is handled at some point.