fix(sampling): align resize-factor behavior and replace reservoir panics with errors#120
fix(sampling): align resize-factor behavior and replace reservoir panics with errors#120Fengzdadi wants to merge 2 commits intoapache:mainfrom
Conversation
| minLgArrItems = 4 | ||
| ) | ||
|
|
||
| func resizeFactorLg(rf ResizeFactor) (int, error) { |
There was a problem hiding this comment.
You can define ResizeFactor using iota. so, verbose case syntax doesn't need.
|
|
||
| ceilingLgK, _ := internal.ExactLog2(common.CeilingPowerOf2(k)) | ||
| initialLgSize := startingSubMultiple( | ||
| ceilingLgK, int(math.Log2(float64(options.resizeFactor))), minLgArrItems, |
There was a problem hiding this comment.
using ResizeFactor itself is ok , right?
| if targetCap <= cap(s.data) { | ||
| return nil | ||
| } | ||
| newData := make([]T, len(s.data), targetCap) |
There was a problem hiding this comment.
L162 ~ L164 is too expensive. that is why i use Grow
|
|
||
| // forceIncrementItemsSeen adds delta to the items seen count. | ||
| func (s *ReservoirItemsSketch[T]) forceIncrementItemsSeen(delta int64) { | ||
| func (s *ReservoirItemsSketch[T]) forceIncrementItemsSeen(delta int64) error { |
There was a problem hiding this comment.
If you add delta to s.n first, it already change state. so guarding not to above maxItemsSeen is correct.
There was a problem hiding this comment.
You are right. I was wrong. chaning state first is correct. sketch's state is gone wrong already.
| assert.Equal(t, ResizeX8, sketch.rf) | ||
| } | ||
|
|
||
| func TestReservoirItemsSketchUpdateReturnsErrorAtMaxItemsSeen(t *testing.T) { |
There was a problem hiding this comment.
I think this case is included already defined test function as nest case.
|
Some issues have been resolved, and I will close this PR. |
|
closed: #139 |
2 participants
ResizeFactorhandling with Java semantics (lg-based growth) inReservoirItemsSketchandVarOptItemsSketch.ReservoirItemsSketch.EstimateSubsetSumwith Java by returning stream-level estimates andTotalSketchWeight = N.panicpaths with returnederrorin sketch/union update flows.go test ./...passes.