ronantakizawa/codereview-bench huggingface dataset:
Repository: pingcap/tidb
File: pkg/expression/fts_helper.go (Go)

Code before:
		}
	}
	for _, item := range patternGroup.Must {
		f, err := rewriteFunc(item)
		if err != nil {
			return expr, false, err
		}
		searchFuncs = append(searchFuncs, f)
	}
	for _, item := range patternGroup.MustNot {
		f, err := rewriteFunc(item)
		if err != nil {
			return expr, false, err
		}
		// For NOT conditions, we wrap the function with a unary NOT operator.
		nf := NewFunctionInternal(bctx, ast.UnaryNot, types.NewFieldType(mysql.TypeDouble), f)
		searchFuncs = append(searchFuncs, nf)
	}
	for _, item := range patternGroup.Should {
		f, err := rewriteFunc(item)
		if err != nil {
			return expr, false, err
		}
		searchFuncs = append(searchFuncs, f)
	}
	if len(patternGroup.Should) > 0 {
		// If there are multiple SHOULD conditions, we combine them with OR operator.
		startIdx := len(patternGroup.Must) + len(patternGroup.MustNot)
		endIdx := startIdx + len(patternGroup.Should)
		orShould := ComposeDNFCondition(bctx, searchFuncs[startIdx:endIdx]...)
		searchFuncs[startIdx] = orShould
		searchFuncs = searchFuncs[:startIdx+1]
	}
	final := ComposeCNFCondition(bctx, searchFuncs...)
	return final, true, nil
}


Review comment (suggestion):
When all slices in patternGroup are empty (e.g., when modifier is not BOOLEAN MODE), searchFuncs will be empty, and ComposeCNFCondition will be called with an empty slice, which returns nil. This could lead to unexpected behavior in the caller. The function should validate that at least one search function was generated, or handle the empty case explicitly.
```suggestion
	}
	if len(searchFuncs) == 0 {
		// No search functions were generated; do not rewrite the original expression.
		return expr, false, nil
	}
```

Code after:
		searchFuncs = append(searchFuncs, f)
	}
	for _, item := range patternGroup.MustNot {
		f, err := rewriteFunc(item)
		if err != nil {
			return expr, err
		}
		// For NOT conditions, we wrap the function with a unary NOT operator.
		nf := NewFunctionInternal(bctx, ast.UnaryNot, types.NewFieldType(mysql.TypeDouble), f)
		searchFuncs = append(searchFuncs, nf)
	}
	for _, item := range patternGroup.Should {
		f, err := rewriteFunc(item)
		if err != nil {
			return expr, err
		}
		searchFuncs = append(searchFuncs, f)
	}
	if len(patternGroup.Should) > 0 {
		// If there are multiple SHOULD conditions, we combine them with OR operator.
		startIdx := len(patternGroup.Must) + len(patternGroup.MustNot)
		endIdx := startIdx + len(patternGroup.Should)
		orShould := ComposeDNFCondition(bctx, searchFuncs[startIdx:endIdx]...)
		searchFuncs[startIdx] = orShould
		searchFuncs = searchFuncs[:startIdx+1]
	}
	final := ComposeCNFCondition(bctx, searchFuncs...)
	return final, nil
}
Repository: pingcap/tidb
File: pkg/lightning/backend/local/localhelper_test.go (Go)

Code before:
		wg.Add(1)
		go func(storeID uint64) {
			defer wg.Done()
			start := time.Now()
			var gotTokens int
			for {
				n := rand.Intn(50)
				if limiter.WaitN(ctx, storeID, n) != nil {
					break
				}
				gotTokens += n
			}
			elapsed := time.Since(start)
			maxTokens := 120 + int(float64(elapsed)/float64(time.Second)*100)
			// In theory, gotTokens should be less than or equal to maxTokens.
			// But we allow a little of error to avoid the test being flaky.
			require.LessOrEqual(t, gotTokens, maxTokens+1)
		}(uint64(i))
	}
	wg.Wait()
}

type splitAndScatterCounterClient struct {
	split.SplitClient
	splitCount atomic.Int32
}

func (c *splitAndScatterCounterClient) SplitKeysAndScatter(_ context.Context, _ [][]byte) ([]*split.RegionInfo, error) {
	c.splitCount.Inc()
	return []*split.RegionInfo{
		{
			Region: &metapb.Region{Id: 1},
		},
	}, nil
}

func (*splitAndScatterCounterClient) WaitRegionsScattered(context.Context, []*split.RegionInfo) (int, error) {
	return 0, nil
}

type splitAndScatterErrClient struct {
	split.SplitClient
	splitCount atomic.Int32
	failAt     int32
}

func (c *splitAndScatterErrClient) SplitKeysAndScatter(_ context.Context, _ [][]byte) ([]*split.RegionInfo, error) {
	cnt := c.splitCount.Inc()
	if cnt == c.failAt {
		return nil, errors.New("mock split error")
	}

Review comment (suggestion):
maybe generate mock impl for `split.SplitClient` to make adding test easier, see example in `gen_mock` of Makefile

Code after:
	limiter = newStoreWriteLimiter(100)
	start := time.Now()
	// 120 is the initial burst, 150 is the number of new tokens.
	err = limiter.WaitN(context.Background(), 1, 120+120)
	require.NoError(t, err)
	require.Greater(t, time.Since(start), time.Second)

	// Test WaitN with different store id.
	limiter = newStoreWriteLimiter(100)
	var wg sync.WaitGroup
	ctx, cancel := context.WithTimeout(context.Background(), time.Second*2)
	defer cancel()
	for i := 0; i < 10; i++ {
		wg.Add(1)
		go func(storeID uint64) {
			defer wg.Done()
			start := time.Now()
			var gotTokens int
			for {
				n := rand.Intn(50)
				if limiter.WaitN(ctx, storeID, n) != nil {
					break
				}
				gotTokens += n
			}
			elapsed := time.Since(start)
			maxTokens := 120 + int(float64(elapsed)/float64(time.Second)*100)
			// In theory, gotTokens should be less than or equal to maxTokens.
			// But we allow a little of error to avoid the test being flaky.
			require.LessOrEqual(t, gotTokens, maxTokens+1)
		}(uint64(i))
	}
	wg.Wait()
}

func TestSplitAndScatterRegionInBatchesTwoLevel(t *testing.T) {
	makeSplitKeys := func(n int) [][]byte {
		keys := make([][]byte, n)
		for i := 0; i < n; i++ {
			keys[i] = []byte{byte(i >> 8), byte(i)}
		}
		return keys
	}

	t.Run("large split keys trigger coarse and fine layers", func(t *testing.T) {
		splitCli := &testSplitClient{}
		local := &Backend{splitCli: splitCli}

		err := local.splitAndScatterRegionInBatches(context.Background(), makeSplitKeys(121), 50, 0)
		require.NoError(t, err)
		// 121 keys => coarse pass(11 keys, 1 batch) + fine pass(121 keys, 3 batches) = 4 calls.
Repository: pingcap/tidb
File: pkg/sessionctx/stmtctx/stmtctx.go (Go)

Code before:
	} else {
		sc.ExtraWarnHandler = contextutil.NewStaticWarnHandler(0)
	}
	return true
}

// CheckMViewUpdatable checks whether a DML operation on a materialized view or materialized view
// log table should be rejected. It returns an error if the table is an MV or MV log and the
// current statement is not an internal maintenance operation.
//
// This guard is called at two layers for defense-in-depth:
//   - Planner layer: provides early rejection during plan building, including at PREPARE time, and
//     keeps consistency with how View and Sequence non-updatability is checked in the same paths.
//   - Executor build layer: the essential correctness check. When a prepared statement is created
//     in a maintenance context but executed after leaving that context, the planner does not
//     re-run — only the executor build phase catches this.
func (sc *StatementContext) CheckMViewUpdatable(tableInfo *model.TableInfo, tableName, op string) error {
	if tableInfo == nil {
		return nil
	}

	if tableName == "" {
		tableName = tableInfo.Name.O
	}

	if sc.InMaterializedViewMaintenance {
		return nil
	}

	if tableInfo.MaterializedView == nil && tableInfo.MaterializedViewLog == nil {
		return nil
	}

	return plannererrors.ErrNonUpdatableTable.GenWithStackByArgs(tableName, op)
}

// CtxID returns the context id of the statement
func (sc *StatementContext) CtxID() uint64 {
	return sc.ctxID
}

// TimeZone returns the timezone of the type context
func (sc *StatementContext) TimeZone() *time.Location {
	intest.AssertNotNil(sc)
	if sc == nil {
		return time.UTC
	}

	return sc.typeCtx.Location()
}


Review comment (suggestion):
I think we can move it to the first line to reduce unnecessary CPU cost (maybit only a bit) in most cases.

Code after:
	return true
}

// CtxID returns the context id of the statement
func (sc *StatementContext) CtxID() uint64 {
	return sc.ctxID
}

// TimeZone returns the timezone of the type context
func (sc *StatementContext) TimeZone() *time.Location {
	intest.AssertNotNil(sc)
	if sc == nil {
		return time.UTC
	}

	return sc.typeCtx.Location()
}

// SetTimeZone sets the timezone
func (sc *StatementContext) SetTimeZone(tz *time.Location) {
	intest.AssertNotNil(tz)
	sc.typeCtx = sc.typeCtx.WithLocation(tz)
}

// TypeCtx returns the type context
func (sc *StatementContext) TypeCtx() types.Context {
	return sc.typeCtx
}

// ErrCtx returns the error context
func (sc *StatementContext) ErrCtx() errctx.Context {
	return sc.errCtx
}

// SetErrLevels sets the error levels for statement
// The argument otherLevels is used to set the error levels except truncate
func (sc *StatementContext) SetErrLevels(otherLevels errctx.LevelMap) {
	sc.errCtx = newErrCtx(sc.typeCtx, otherLevels, sc)
}

// ErrLevels returns the current `errctx.LevelMap`
func (sc *StatementContext) ErrLevels() errctx.LevelMap {
	return sc.errCtx.LevelMap()
}

// ErrGroupLevel returns the error level for the given error group
func (sc *StatementContext) ErrGroupLevel(group errctx.ErrGroup) errctx.Level {
	return sc.errCtx.LevelForGroup(group)
}

// TypeFlags returns the type flags
Repository: pingcap/tidb
File: pkg/planner/core/util.go (Go)

Code before:

	encodedIdxVals, err := codec.EncodeKey(sc.TimeZone(), nil, idxVals...)
	err = sc.HandleError(err)
	if err != nil {
		return nil, err
	}
	return encodedIdxVals, nil
}

// CheckMViewUpdatable checks whether a DML operation on a materialized view or materialized view
// log table should be rejected. It returns an error if the table is an MV or MV log and the
// current session is not in internal maintenance mode.
func CheckMViewUpdatable(sv *variable.SessionVars, tableInfo *model.TableInfo, aliasName, op string) error {
	if tableInfo.MaterializedView == nil && tableInfo.MaterializedViewLog == nil {
		return nil
	}

	if sv.InMaterializedViewMaintenance {
		// All MV maintenance work currently uses internal sessions (restricted SQL).
		intest.Assert(sv.InRestrictedSQL)
		return nil
	}

	if aliasName == "" {
		aliasName = tableInfo.Name.O
	}

	return plannererrors.ErrNonUpdatableTable.GenWithStackByArgs(aliasName, op)
}


Review comment (suggestion):
For production we could also add a check to make it more robust
```
if !sv.InRestrictedSQL {
    return err
}
```

Code after:
	encodedIdxVals, err := codec.EncodeKey(sc.TimeZone(), nil, idxVals...)
	err = sc.HandleError(err)
	if err != nil {
		return nil, err
	}
	return encodedIdxVals, nil
}

// CheckMViewUpdatable checks whether a DML operation on a materialized view or materialized view
// log table should be rejected. It returns an error if the table is an MV or MV log and the current
// session is not in internal maintenance mode.
func CheckMViewUpdatable(
	sv *variable.SessionVars, tableInfo *model.TableInfo, aliasName, op string,
) error {
	if tableInfo.MaterializedView == nil && tableInfo.MaterializedViewLog == nil {
		return nil
	}

	if sv.InMaterializedViewMaintenance {
		// All MV maintenance work should uses internal sessions (restricted SQL).
		if !sv.InRestrictedSQL {
			return plannererrors.ErrInternal.GenWithStack(
				"materialized view maintenance should only run in restricted SQL mode",
			)
		}
		return nil
	}

	if aliasName == "" {
		aliasName = tableInfo.Name.O
	}

	return plannererrors.ErrNonUpdatableTable.GenWithStackByArgs(aliasName, op)
}
Repository: pingcap/tidb
File: pkg/objstore/objectio/writer_test.go (Go)

Code before:

	// sync decode
	synctest.Test(t, func(t *testing.T) {
		pipeReader, pipeWriter := io.Pipe()

		prevRoutineCnt := runtime.NumGoroutine()
		config := compressedio.DecompressConfig{ZStdDecodeConcurrency: 1}
		r, err := compressedio.NewReader(compressedio.Zstd, config, pipeReader)
		synctest.Wait()

		require.NoError(t, err)
		currRoutineCnt := runtime.NumGoroutine()
		require.Equal(t, prevRoutineCnt, currRoutineCnt)

		// because this is "sync" decode, the Write will block and cause a deadlock. We
		// must use another goroutine. This is another sign that the decode is sync and
		// previous synctest is async.
		go func() {
			_, err := pipeWriter.Write(compressedData)
			require.NoError(t, err)
			err = pipeWriter.Close()
			require.NoError(t, err)
		}()

		allData, err := io.ReadAll(r)
		require.NoError(t, err)
		require.Equal(t, "data", string(allData))
	})
}


Review comment (bug):
The goroutine spawned here can fail with require.NoError assertions, which will cause a panic in a non-test goroutine. In Go testing, require functions (unlike assert) will call t.FailNow() which can cause issues when called from non-test goroutines. Consider either using assert functions or ensuring that any test failures are properly communicated back to the test goroutine. A safer pattern would be to use a channel or wait group to ensure the goroutine completes before the test ends, or use testify's assert functions instead of require.

Code after:
		require.Equal(t, "data", string(allData))
	})

	// sync decode
	synctest.Test(t, func(t *testing.T) {
		pipeReader, pipeWriter := io.Pipe()
		defer pipeReader.Close()

		prevRoutineCnt := runtime.NumGoroutine()
		config := compressedio.DecompressConfig{ZStdDecodeConcurrency: 1}
		r, err := compressedio.NewReader(compressedio.Zstd, config, pipeReader)
		synctest.Wait()

		require.NoError(t, err)
		currRoutineCnt := runtime.NumGoroutine()
		require.Equal(t, prevRoutineCnt, currRoutineCnt)

		// because this is "sync" decode, the Write will block and cause a deadlock. We
		// must use another goroutine. This is another sign that the decode is sync and
		// previous synctest is async.
		go func() {
			_, err := pipeWriter.Write(compressedData)
			assert.NoError(t, err)
			err = pipeWriter.Close()
			assert.NoError(t, err)
		}()

		allData, err := io.ReadAll(r)
		require.NoError(t, err)
		require.Equal(t, "data", string(allData))
	})
}
Repository: pingcap/tidb
File: pkg/objstore/objectio/writer_test.go (Go)

Code before:

	// sync decode
	synctest.Test(t, func(t *testing.T) {
		pipeReader, pipeWriter := io.Pipe()

		prevRoutineCnt := runtime.NumGoroutine()
		config := compressedio.DecompressConfig{ZStdDecodeConcurrency: 1}
		r, err := compressedio.NewReader(compressedio.Zstd, config, pipeReader)
		synctest.Wait()

		require.NoError(t, err)
		currRoutineCnt := runtime.NumGoroutine()
		require.Equal(t, prevRoutineCnt, currRoutineCnt)

		// because this is "sync" decode, the Write will block and cause a deadlock. We
		// must use another goroutine. This is another sign that the decode is sync and
		// previous synctest is async.
		go func() {
			_, err := pipeWriter.Write(compressedData)
			require.NoError(t, err)
			err = pipeWriter.Close()
			require.NoError(t, err)
		}()

		allData, err := io.ReadAll(r)
		require.NoError(t, err)
		require.Equal(t, "data", string(allData))
	})
}


Review comment (suggestion):
There's no synchronization to ensure the goroutine completes before the test ends. If io.ReadAll at line 249 returns quickly (e.g., due to an error or small data), the test may exit before the goroutine has finished writing and closing the pipe. This could lead to test flakiness or resource leaks. Consider using a sync.WaitGroup or channel to ensure the goroutine completes before the test exits, especially since this test is specifically designed to fix flakiness issues.

Code after:
		require.Equal(t, "data", string(allData))
	})

	// sync decode
	synctest.Test(t, func(t *testing.T) {
		pipeReader, pipeWriter := io.Pipe()
		defer pipeReader.Close()

		prevRoutineCnt := runtime.NumGoroutine()
		config := compressedio.DecompressConfig{ZStdDecodeConcurrency: 1}
		r, err := compressedio.NewReader(compressedio.Zstd, config, pipeReader)
		synctest.Wait()

		require.NoError(t, err)
		currRoutineCnt := runtime.NumGoroutine()
		require.Equal(t, prevRoutineCnt, currRoutineCnt)

		// because this is "sync" decode, the Write will block and cause a deadlock. We
		// must use another goroutine. This is another sign that the decode is sync and
		// previous synctest is async.
		go func() {
			_, err := pipeWriter.Write(compressedData)
			assert.NoError(t, err)
			err = pipeWriter.Close()
			assert.NoError(t, err)
		}()

		allData, err := io.ReadAll(r)
		require.NoError(t, err)
		require.Equal(t, "data", string(allData))
	})
}
Repository: pixijs/pixijs
File: tests/assets/TextFormat.test.ts (TypeScript)

Code before:
import { TextFormat } from '../../src/scene/text/bitmap/asset/textFormat';

const rawFontString = `
info face="sans-serif" size=36 bold=0 italic=0 charset="" unicode=1 stretchH=100 smooth=1 aa=1 padding=1,1,1,1 spacing=1,1
common lineHeight=36 base=27 scaleW=256 scaleH=256 pages=1 packed=0
page id=0 file="sans-serif.png"
chars count=2
char id=32 x=0 y=0 width=0 height=0 xoffset=0 yoffset=0 xadvance=12 page=0 chnl=15
char id=33 x=244 y=107 width=8 height=30 xoffset=4 yoffset=0 xadvance=15 page=0 chnl=15
`;

describe('TextFormat', () =>
{
    it.only('should parse text chars, if no letter or char property is present ', async () =>
    {
        const parsedText = TextFormat.parse(rawFontString);

        expect(parsedText.chars['!']).toEqual({
            id: 33,
            page: 0,
            x: 244,
            y: 107,
            width: 8,
            height: 30,
            xOffset: 4,
            yOffset: 0,
            xAdvance: 15,
            kerning: {}
        });
    });
});


Review comment (suggestion):
```suggestion
    it('should parse text chars, if no letter or char property is present ', async () =>
```

Code after:
import { TextFormat } from '../../src/scene/text/bitmap/asset/textFormat';

const rawFontString = `
info face="sans-serif" size=36 bold=0 italic=0 charset="" unicode=1 stretchH=100 smooth=1 aa=1 padding=1,1,1,1 spacing=1,1
common lineHeight=36 base=27 scaleW=256 scaleH=256 pages=1 packed=0
page id=0 file="sans-serif.png"
chars count=2
char id=32 x=0 y=0 width=0 height=0 xoffset=0 yoffset=0 xadvance=12 page=0 chnl=15
char id=33 x=244 y=107 width=8 height=30 xoffset=4 yoffset=0 xadvance=15 page=0 chnl=15
`;

describe('TextFormat', () =>
{
    it('should parse text chars, if no letter or char property is present ', async () =>
    {
        const parsedText = TextFormat.parse(rawFontString);

        expect(parsedText.chars['!']).toEqual({
            id: 33,
            page: 0,
            x: 244,
            y: 107,
            width: 8,
            height: 30,
            xOffset: 4,
            yOffset: 0,
            xAdvance: 15,
            kerning: {}
        });
    });
});
Repository: pixijs/pixijs
File: tests/mixin-get-child-by-name/getChildByName.tests.ts (TypeScript)

Code before:
import { Container } from '../../src/scene/container/Container';
// import '../../src/scene/container/container-mixins/findMixin';

describe('DisplayObject#name', () =>
{
    it('should contain property', () =>
    {
        const obj = new Container();

        expect(obj.label).toBeDefined();
        expect(obj.label).toBeNull();
    });
});

describe('Container#getChildByName', () =>
{
    it('should exist', () =>
    {
        const parent = new Container();

        expect(parent.getChildByName).toBeDefined();
        expect(parent.getChildByName).toBeInstanceOf(Function);
    });

    it('should correctly find a child by its name', () =>
    {
        const obj = new Container();
        const parent = new Container();


Review comment (suggestion):
```suggestion
describe('Container#name', () =>
```

Code after:
import { Container } from '../../src/scene/container/Container';
// import '../../src/scene/container/container-mixins/findMixin';

describe('Container#name', () =>
{
    it('should contain property', () =>
    {
        const obj = new Container();

        expect(obj.label).toBeDefined();
        expect(obj.label).toBeNull();
    });
});

describe('Container#getChildByName', () =>
{
    it('should exist', () =>
    {
        const parent = new Container();

        expect(parent.getChildByName).toBeDefined();
        expect(parent.getChildByName).toBeInstanceOf(Function);
    });

    it('should correctly find a child by its name', () =>
    {
        const obj = new Container();
        const parent = new Container();
Repository: pixijs/pixijs
File: tests/mixin-get-global-position/getGlobalPosition.tests.ts (TypeScript)

Code before:
import { Container } from '../../src/scene/container/Container';

describe('DisplayObject#getGlobalPosition', () =>
{
    it('should exist', () =>
    {
        const obj = new Container();

        expect(obj.getGlobalPosition).toBeDefined();
        expect(obj.getGlobalPosition).toBeInstanceOf(Function);
    });

    it.skip('should return correct global coordinates of a displayObject, without depending on its pivot', () =>
    {
        const parent = new Container();
        const container = new Container();

        parent.addChild(container);

        parent.position.set(100, 100);
        parent.rotation = Math.PI;
        parent.scale.set(2, 2);
        container.position.set(10, -30);
        container.pivot.set(1000, 1000);

        let globalPoint = container.getGlobalPosition();

        expect(globalPoint.x).toEqual(80);

Review comment (suggestion):
```suggestion
describe('Container#getGlobalPosition', () =>
```

Code after:
import { Container } from '../../src/scene/container/Container';

describe('Container#getGlobalPosition', () =>
{
    it('should exist', () =>
    {
        const obj = new Container();

        expect(obj.getGlobalPosition).toBeDefined();
        expect(obj.getGlobalPosition).toBeInstanceOf(Function);
    });

    it('should return correct global coordinates of a container, without depending on its pivot', () =>
    {
        const parent = new Container();
        const container = new Container();

        parent.addChild(container);

        parent.position.set(100, 100);
        parent.rotation = Math.PI;
        parent.scale.set(2, 2);
        container.position.set(10, -30);
        container.pivot.set(1000, 1000);

        let globalPoint = container.getGlobalPosition();

        expect(globalPoint.x).toEqual(80);