Allow using client indices for *string columns
#387
Conversation
cache/cache.go
Outdated
| @@ -1251,7 +1251,7 @@ func valueFromIndex(info *mapper.Info, columnKeys []model.ColumnKey) (interface{ | |||
| } | |||
|
|
|||
| func valueFromColumnKey(info *mapper.Info, columnKey model.ColumnKey) (interface{}, error) { | |||
| val, err := info.FieldByColumn(columnKey.Column) | |||
| val, err := info.DerefFieldByColumn(columnKey.Column) | |||
There was a problem hiding this comment.
I think the logic applies the same on val either returned from FieldByColumn or valueFromMap so i would just probably apply the logic on val at the end of this method.
|
The README can probably take an update too |
npinaeva
force-pushed
the
pointer-column
branch
from
58643c1
to
24d10ff
Compare
README.md
Outdated
| @@ -120,6 +120,26 @@ can now be improved with: | |||
| // quick indexed result | |||
| ovn.Where(lb).List(ctx, &results) | |||
|
|
|||
| Client indexes support using pointer Columns, for example if a column "optional_string" has Model type `*string`, | |||
There was a problem hiding this comment.
I will probably write this from the functional perspective rather than the implementation perspective. Something like
Unlike schema indexes, client indexes can be used with optional columns and rows with no value are indexed as well
You can probably join this sentence, with the other one above that also starts with Unlike schema indexes so that differences are highlighted together.
Other than that small mention I don't much else is needed. The example doesn't really add much value I think.
There was a problem hiding this comment.
not sure if I got your suggestion right, lmk if I should change it more.
cache/cache.go
Outdated
| v := reflect.ValueOf(val) | ||
| if v.Kind() == reflect.Ptr && !v.IsNil() { | ||
| v = v.Elem() | ||
| } | ||
| return v.Interface(), err |
There was a problem hiding this comment.
does this work?
// if the value is a non-nil pointer of an optional, dereference
v := reflect.ValueOf(val)
if v.Kind() == reflect.Ptr && !v.IsNil() {
val = v.Elem().Interface()
}
return val, err
cache/cache_test.go
Outdated
|
|
||
| var nilString *string | ||
|
|
||
| func TestRowCacheCreateReferenceClientIndex(t *testing.T) { |
There was a problem hiding this comment.
What does Reference stand for here? Maybe TestRowCacheCreateOptionalColumnClientIndex?
cache/cache_test.go
Outdated
| err = json.Unmarshal(getTestSchema(""), &schema) | ||
| require.Nil(t, err) | ||
| testData := Data{ | ||
| "Open_vSwitch": map[string]model.Model{"bar": &testModel{Datapath: getStringPtr("bar")}}} |
There was a problem hiding this comment.
nit: closing brace on a new line
npinaeva
force-pushed
the
pointer-column
branch
from
24d10ff
to
f4ace68
Compare
let me do an ovn-k pr with that |
Looks green enough :) ovn-org/ovn-kubernetes#4619 |
README.md
Outdated
| Unlike schema indexes, client indexes can be used with optional columns and rows with no value are indexed as well. | ||
| For example, if a column "optional_string" has Model type `*string`, it can be used in a client index. | ||
| The indexing will happen based on dereferenced value, but `nil` will be considered a valid value. |
There was a problem hiding this comment.
So above we have the phrase
...the columns that compose the index. Unlike schema indexes, a key within a column can be addressed if the column
type is a map.
I was thinking on something like:
...the columns that compose the index. However, unlike schema indexes, client indexes:
* can be used with columns that are optional, where no-value columns are indexed as well.
* can be used with columns that are maps, where specific map keys can be indexed (see example below).
This way we have the two main differences together. Let me know what you think.
There was a problem hiding this comment.
I like this description! I just missed that we already have a similar sentence in the doc.
This allows using optional pointer fields (e.g. *string) as client indices. Signed-off-by: Nadia Pinaeva <[email protected]>
npinaeva
force-pushed
the
pointer-column
branch
from
f4ace68
to
7f3049e
Compare
When getting cache values, dereference pointers.
This allows using pointer fields (e.g. *string) as client indices.
Addresses #386