perf: Use map for InPredicate when reading dataobj - #18325
Conversation
| return found */ | ||
|
|
||
| value := row.Values[columnIndex] | ||
| if value.Type() != p.Column.ColumnInfo().Type { |
There was a problem hiding this comment.
this could fail for Nil values?
| case datasetmd.VALUE_TYPE_BYTE_ARRAY: | ||
| _, ok = p.ValuesMap[value.ByteArray()] | ||
| return ok |
There was a problem hiding this comment.
This should panic at runtime; Go maps don't support being keyed with a slice. This compiles because the slice is being casted to an interface{} / any, but it'll fail at runtime when the runtime tries to hash the slice value for populating the entry in the map.
There was a problem hiding this comment.
Good spot - fixed using your suggestion
rfratto
left a comment
rfratto
left a comment
There was a problem hiding this comment.
LGTM, two small things that need to be addressed but this is good to merge!
Does Ashwanth's comment about nulls need to be addressed still?
| Values: NewInt64ValueSet([]Value{ | ||
| Int64Value(150), | ||
| Int64Value(600), | ||
| }), // 2 values in range. ~200 matching rows |
There was a problem hiding this comment.
Is this comment correct? the test name is that "all values are outside range"
| for i := range p.Values { | ||
| vals[i] = arrowconv.FromScalar(p.Values[i], mustConvertType(p.Values[i].DataType())) | ||
| value := arrowconv.FromScalar(p.Values[i], mustConvertType(p.Values[i].DataType())) | ||
| values = append(values, value) |
There was a problem hiding this comment.
This will break now; the length of the array is already p.Values, so appending to it will have the length be p.Values*2 after the end of this loop. (I guess there's no test for this path?)
| for i := range p.Values { | ||
| vals[i] = arrowconv.FromScalar(p.Values[i], mustConvertType(p.Values[i].DataType())) | ||
| value := arrowconv.FromScalar(p.Values[i], mustConvertType(p.Values[i].DataType())) | ||
| values = append(values, value) |
There was a problem hiding this comment.
Same comment as above: this should be either assigning directly to values[i] or changing values to have a length of zero with a capacity of len(p.Values).
3 participants
What this PR does / why we need it:
When we use a large list of stream IDs, dataobjs spend a lot of time running CompareValues on each element of the Values list. Here I'm replacing it with a map of the underlying value (hence, it only supports int64, uint64 and byte arrays) and simply skipping the check if its not a relevant type.
For this benchmark, its 30% faster but when querying with many stream IDs I've seen my queries get >10x faster.