ronantakizawa/codereview-bench huggingface dataset:
Repository: microsoft/garnet
File: libs/server/ArgSlice/ArgSliceVector.cs (C#)

Code before:

            items.Add((insertLoc, sb.Length, HasNamespace: true));
            return true;
        }

        /// <summary>
        /// Clear ArgSliceVector
        /// </summary>
        public void Clear()
        {
            items.Clear();
            bufferManager.Reset();
        }

        public IEnumerator<SpanByte> GetEnumerator()
        {
            var full = bufferManager.ViewFullArgSlice();

            foreach (var (offset, length, hasNamespace) in items)
            {
                var span = full.ReadOnlySpan.Slice(offset, length);
                var ret = SpanByte.FromPinnedSpan(span);

                if (hasNamespace)
                {
                    ret.MarkNamespace();
                }

                yield return ret;
            }
        }

        IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
    }
}

Review comment (suggestion):
Per the semantics of [ScratchBufferBuilder](https://github.com/microsoft/garnet/blob/main/libs/server/ArgSlice/ScratchBufferBuilder.cs), once you start returning ArgSlices to callers, you need to "seal" the data structure (before enumeration) to prevent any more allocations, in order to ensure validity of the ArgSlices returned by enumeration. It may be useful to enforce it (i.e., sealing) for the `ArgSliceVector` data structure.

The other option, as discussed offline, is to use [ScratchBufferAllocator](https://github.com/microsoft/garnet/blob/main/libs/server/ArgSlice/ScratchBufferAllocator.cs), which does not have such a restriction.

Code after:
            var sb = argSlice.SpanByte;

            sb.MarkNamespace();
            sb.SetNamespaceInPayload((byte)ns);
            item.CopyTo(sb.AsSpan());

            items.Add((insertLoc, sb.Length, HasNamespace: true));
            return true;
        }

        /// <summary>
        /// Clear ArgSliceVector
        /// </summary>
        public void Clear()
        {
            Debug.Assert(!enumerating, "Cannot modify while enumerating");

            items.Clear();
            bufferManager.Reset();
        }

        public IEnumerator<SpanByte> GetEnumerator()
        {
            Debug.Assert(!enumerating, "Concurrent enumeration is not allwed");

            var full = bufferManager.ViewFullArgSlice();

            enumerating = true;
            try
            {
                foreach (var (offset, length, hasNamespace) in items)
                {
                    var span = full.ReadOnlySpan.Slice(offset, length);
                    var ret = SpanByte.FromPinnedSpan(span);

                    if (hasNamespace)
                    {
                        ret.MarkNamespace();
                    }

                    yield return ret;
                }
            }
            finally
            {
                enumerating = false;
            }
        }

        IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
    }
Repository: microsoft/gctoolkit
File: api/src/test/java/com/microsoft/gctoolkit/time/DateTimeStampTest.java (Java)

Code before:
    }

    @Test
    void compareToTransitivity() {
        DateTimeStamp stamp1 = new DateTimeStamp("2021-09-01T11:12:13.111-0100", 100);
        DateTimeStamp stamp2 = new DateTimeStamp((String)null, 200);
        DateTimeStamp stamp3 = new DateTimeStamp("2021-08-31T11:12:13.111-0100", 300);
        assertTrue(stamp1.compareTo(stamp2) < 0);
        assertTrue(stamp2.compareTo(stamp3) < 0);
        assertTrue(stamp1.compareTo(stamp3) < 0);
    }

    @Test
    void compareToTransitivityWithEqualTimeStamps() {
        DateTimeStamp stamp1 = new DateTimeStamp("2021-09-01T11:12:13.111-0100", 100);
        DateTimeStamp stamp2 = new DateTimeStamp((String)null, 100);
        DateTimeStamp stamp3 = new DateTimeStamp("2021-08-31T11:12:13.111-0100", 100);
        int comp1To2 = stamp1.compareTo(stamp2);
        int comp2To3 = stamp2.compareTo(stamp3);
        assertEquals(comp1To2, comp2To3);
        int comp1To3 = stamp1.compareTo(stamp3);
        assertEquals(comp1To2, comp1To3, "compareTo() is not transitive");
    }

    @Test
    void workWithMalformedDateTimeStamps() {
        final DateTimeStamp onlyDate = new DateTimeStamp("2021-09-01T11:12:13.111-0100");
        final DateTimeStamp onlyTime = new DateTimeStamp(1.0D);
        assertThrows(IllegalStateException.class, () -> onlyDate.after(onlyTime));
    }

}

Review comment (suggestion):
```suggestion
    void malformedDateTimeStampThrowsException() {
```

Code after:
    }

    @Test
    void compareToTransitivity() {
        DateTimeStamp stamp1 = new DateTimeStamp("2021-09-01T11:12:13.111-0100", 100);
        DateTimeStamp stamp2 = new DateTimeStamp((String)null, 200);
        DateTimeStamp stamp3 = new DateTimeStamp("2021-08-31T11:12:13.111-0100", 300);
        assertTrue(stamp1.compareTo(stamp2) < 0);
        assertTrue(stamp2.compareTo(stamp3) < 0);
        assertTrue(stamp1.compareTo(stamp3) < 0);
    }

    @Test
    void compareToTransitivityWithEqualTimeStamps() {
        DateTimeStamp stamp1 = new DateTimeStamp("2021-09-01T11:12:13.111-0100", 100);
        DateTimeStamp stamp2 = new DateTimeStamp((String)null, 100);
        DateTimeStamp stamp3 = new DateTimeStamp("2021-08-31T11:12:13.111-0100", 100);
        int comp1To2 = stamp1.compareTo(stamp2);
        int comp2To3 = stamp2.compareTo(stamp3);
        assertEquals(comp1To2, comp2To3);
        int comp1To3 = stamp1.compareTo(stamp3);
        assertEquals(comp1To2, comp1To3, "compareTo() is not transitive");
    }

    @Test
    void malformedDateTimeStampThrowsException() {
        final DateTimeStamp onlyDate = new DateTimeStamp("2021-09-01T11:12:13.111-0100");
        final DateTimeStamp onlyTime = new DateTimeStamp(1.0D);
        assertThrows(IllegalStateException.class, () -> onlyDate.after(onlyTime));
    }

}
Repository: microsoft/gctoolkit
File: parser/src/test/java/com/microsoft/gctoolkit/parser/unittests/UnifiedG1GCParserTest.java (Java)

Code before:
        for (String name : details) {
            try {
                Path path = new TestLogFile("g1gc/" + name).getFile().toPath();
                TestResults testResults = testUnifiedG1GCSingleFile(path);
                analyzeResults(name, testResults, detailsNumberOfDifferentCollectors[i], detailsCounts[i++]);
            } catch (IOException ioe) {
                fail(ioe.getMessage());
            }
        }
    }

    private static final String[] details = {
            "jvm-gc-10Nov2022.log",
            "details_reference.log",
            "jdk11_details.log.zip"
    };

    private static final int[] detailsNumberOfDifferentCollectors = {
            9,
            12,
            11
    };
    private static final int[][] detailsCounts = {
            //  0,   1,   2, 3,  4,   5, 6,   7,   8,   9, 10, 11,  12,  13,  14,  15,  16,  17,  18,  19
            {   4,   0,   2, 0,  0,   2, 0,   2,   2,   2,  0,  0,   2,   2,   0,   0,   0,   0,   2,   0},
            {1130, 141, 146, 0,  0, 146, 0, 146,   0, 146,  1,  0, 146, 146,   0,   0,  16, 146, 146,   0},
            {9969,  40, 194, 0, 42, 198, 0, 156, 152, 152,  0,  0, 194, 194,   0,   0,   0,   0, 152,   0}
    };
}


Review comment (bug):
These changes caught a bug in the G1 test, where the line

```
[226.179s][info ][gc,start      ] GC(1198) Pause Full (Diagnostic Command)
```

Was not being identified correctly. `Diagnostic Command` was not in the GCCauses map and it was identified as a `System.gc()`. The fix now picks this up as Full GC type. The result is the the counts now differ after these changes.  

Code after:
        for (String name : details) {
            try {
                Path path = new TestLogFile("g1gc/" + name).getFile().toPath();
                TestResults testResults = testUnifiedG1GCSingleFile(path);
                analyzeResults(name, testResults, detailsNumberOfDifferentCollectors[i], detailsCounts[i++]);
            } catch (IOException ioe) {
                fail(ioe.getMessage());
            }
        }
    }

    private static final String[] details = {
            "jvm-gc-10Nov2022.log",
            "details_reference.log",
            "jdk11_details.log.zip"
    };

    private static final int[] detailsNumberOfDifferentCollectors = {
            9,
            12,
            11
    };
    private static final int[][] detailsCounts = {
            //  0,   1,   2, 3,  4,   5, 6,   7,   8,   9, 10, 11,  12,  13,  14,  15,  16,  17,  18,  19
            {   4,   0,   2, 0,  0,   2, 0,   2,   2,   2,  0,  0,   2,   2,   0,   0,   0,   0,   2,   0},
            {1130, 141, 146, 0,  1, 146, 0, 146,   0, 146,  0,  0, 146, 146,   0,   0,  16, 146, 146,   0},
            {9969,  40, 194, 0, 42, 198, 0, 156, 152, 152,  0,  0, 194, 194,   0,   0,   0,   0, 152,   0}
    };
}
Repository: microsoft/graphrag
File: graphrag/index/utils/tokens.py (Python)

Code before:
# Copyright (c) 2024 Microsoft Corporation.
# Licensed under the MIT License

"""Utilities for working with tokens."""

import tiktoken


def num_tokens_from_string(
    string: str, model: str | None = None, encoding_name: str | None = None
) -> int:
    """Return the number of tokens in a text string."""
    if model is not None:
        encoding = tiktoken.encoding_for_model(model)
    elif encoding_name is not None:
        encoding = tiktoken.get_encoding(encoding_name)
    else:
        encoding = tiktoken.get_encoding("cl100k_base")
    return len(encoding.encode(string))


def string_from_tokens(
    tokens: list[int], model: str | None = None, encoding_name: str | None = None
) -> str:
    """Return a text string from a list of tokens."""
    if model is not None:
        encoding = tiktoken.encoding_for_model(model)
    elif encoding_name is not None:
        encoding = tiktoken.get_encoding(encoding_name)
    else:
        msg = "Either model or encoding_name must be specified."
        raise ValueError(msg)
    return encoding.decode(tokens)


Review comment (suggestion):
collapse this whole elif and else clause into

`encoding = tiktoken.get_encoding(encoding_name or DEFAULT_ENCODING_NAME)` and set the default at the module level

Code after:
# Copyright (c) 2024 Microsoft Corporation.
# Licensed under the MIT License

"""Utilities for working with tokens."""

import tiktoken

DEFAULT_ENCODING_NAME = "cl100k_base"


def num_tokens_from_string(
    string: str, model: str | None = None, encoding_name: str | None = None
) -> int:
    """Return the number of tokens in a text string."""
    if model is not None:
        encoding = tiktoken.encoding_for_model(model)
    else:
        encoding = tiktoken.get_encoding(encoding_name or DEFAULT_ENCODING_NAME)
    return len(encoding.encode(string))


def string_from_tokens(
    tokens: list[int], model: str | None = None, encoding_name: str | None = None
) -> str:
    """Return a text string from a list of tokens."""
    if model is not None:
        encoding = tiktoken.encoding_for_model(model)
    elif encoding_name is not None:
        encoding = tiktoken.get_encoding(encoding_name)
    else:
        msg = "Either model or encoding_name must be specified."
        raise ValueError(msg)
    return encoding.decode(tokens)
Repository: microsoft/graphrag
File: graphrag/index/cli.py (Python)

Code before:
        info(
            f"Logging not enabled for config {_redact(default_config.model_dump())}",
            True,
        )

    if skip_validations:
        validate_config_names(progress_reporter, default_config)

    info(f"Starting pipeline run for: {run_id}, {dryrun=}", verbose)
    info(
        f"Using default configuration: {_redact(default_config.model_dump())}",
        verbose,
    )

    if dryrun:
        info("Dry run complete, exiting...", True)
        sys.exit(0)

    pipeline_emit = emit.split(",") if emit else None

    _register_signal_handlers(progress_reporter)

    outputs = asyncio.run(
        build_index(
            default_config,
            run_id,
            memprofile,
            progress_reporter,
            pipeline_emit,
        )
    )
    encountered_errors = any(
        output.errors and len(output.errors) > 0 for output in outputs
    )

    progress_reporter.stop()
    if encountered_errors:
        error(
            "Errors occurred during the pipeline run, see logs for more details.", True
        )
    else:
        success("All workflows completed successfully.", True)

    sys.exit(1 if encountered_errors else 0)


def _initialize_project_at(path: str, reporter: ProgressReporter) -> None:
    """Initialize the project at the given path."""
    reporter.info(f"Initializing project at {path}")
    root = Path(path)
    if not root.exists():

Review comment (suggestion):
This seems good for now. Once implementation starts, you may find it cleaner to fold the logic into the existing `build_index(...)` function and add a new parameter. But this will be a good placeholder for now.

Code after:
    else:
        info(
            f"Logging not enabled for config {_redact(default_config.model_dump())}",
            True,
        )

    if skip_validations:
        validate_config_names(progress_reporter, default_config)

    info(f"Starting pipeline run for: {run_id}, {dryrun=}", verbose)
    info(
        f"Using default configuration: {_redact(default_config.model_dump())}",
        verbose,
    )

    if dryrun:
        info("Dry run complete, exiting...", True)
        sys.exit(0)

    pipeline_emit = emit.split(",") if emit else None

    _register_signal_handlers(progress_reporter)

    if update_index_id:
        outputs = asyncio.run(
            update_index(
                default_config,
                memprofile,
                update_index_id,
                progress_reporter,
                pipeline_emit,
            )
        )
    else:
        outputs = asyncio.run(
            build_index(
                default_config,
                run_id,
                memprofile,
                progress_reporter,
                pipeline_emit,
            )
        )
    encountered_errors = any(
        output.errors and len(output.errors) > 0 for output in outputs
    )

    progress_reporter.stop()
    if encountered_errors:
        error(
            "Errors occurred during the pipeline run, see logs for more details.", True
Repository: microsoft/graphrag
File: graphrag/config/load_config.py (Python)

Code before:

    # If user specified a config file path then it is required
    if config_filepath:
        config_path = (root / config_filepath).resolve()
        if not config_path.exists():
            msg = f"Specified Config file not found: {config_path}"
            raise FileNotFoundError(msg)

    # Else optional resolve the config path from the root directory if it exists
    config_path = search_for_config_in_root_dir(root)
    if config_path:
        config = load_config_from_file(config_path)
    else:
        config = create_graphrag_config(root_dir=str(root))

    if run_id:
        config.storage.base_dir = str(
            resolve_timestamp_path(config.storage.base_dir, run_id)
        )
        config.reporting.base_dir = str(
            resolve_timestamp_path(config.reporting.base_dir, run_id)
        )
    else:
        config.storage.base_dir = str(resolve_timestamp_path(config.storage.base_dir))
        config.reporting.base_dir = str(
            resolve_timestamp_path(config.reporting.base_dir)
        )

    if config.storage.type == StorageType.file:
        config.storage.base_dir = str((root / config.storage.base_dir).resolve())
    if config.reporting.type == ReportingType.file:
        config.reporting.base_dir = str((root / config.reporting.base_dir).resolve())

    return config


Review comment (refactor):
Wonder if we can simplify the logic in the load_config method by translating some of the responsability to resolve_timestamp_path.
Make it more generic, something like resolve_path that receives the path, the root, the type, the id, etc. And allows for nulls and such.
Also, we can use switch case when possible, it resolves faster than if else.

Code after:

    # If user specified a config file path then it is required
    if config_filepath:
        config_path = (root / config_filepath).resolve()
        if not config_path.exists():
            msg = f"Specified Config file not found: {config_path}"
            raise FileNotFoundError(msg)

    # Else optional resolve the config path from the root directory if it exists
    config_path = search_for_config_in_root_dir(root)
    if config_path:
        config = load_config_from_file(config_path)
    else:
        config = create_graphrag_config(root_dir=str(root))

    config.storage.base_dir = str(
        resolve_path(
            config.storage.base_dir,
            root if config.storage.type == StorageType.file else None,
            run_id,
        )
    )
    config.reporting.base_dir = str(
        resolve_path(
            config.reporting.base_dir,
            root if config.reporting.type == ReportingType.file else None,
            run_id,
        )
    )

    return config
Repository: microsoft/graphrag
File: graphrag/index/workflows/v1/subflows/create_base_documents.py (Python)

Code before:
    )

    docs_with_text_units = aggregate_df(
        joined,
        groupby=["id"],
        aggregations=[
            {
                "column": "chunk_id",
                "operation": "array_agg",
                "to": "text_units",
            }
        ],
    )

    rejoined = docs_with_text_units.merge(
        source,
        left_on="id",
        right_on="id",
        how="right",
    )
    rejoined.rename(columns={"text": "raw_content"}, inplace=True)
    rejoined["id"] = rejoined["id"].astype(str)

    # attribute columns are converted to strings and then collapsed into a single json object
    if document_attribute_columns is not None and len(document_attribute_columns) > 0:
        for column in document_attribute_columns:
            rejoined[column] = rejoined[column].astype(str)
        rejoined["attributes"] = rejoined[document_attribute_columns].apply(
            lambda row: ({**row}),
            axis=1,
        )
        rejoined.drop(columns=document_attribute_columns, inplace=True)
        rejoined.reset_index()

    return create_verb_result(
        cast(
            Table,
            rejoined,
        )
    )


Review comment (suggestion):
I think that 
```py
rejoined = rejoined.astype({column: str for column in document_attribute_columns})
```
 would do the trick... the idea would be that the astype is executed just once on the dataframe instead of `len(document_attribute_columns)` times 

Code after:
    )

    docs_with_text_units = aggregate_df(
        joined,
        groupby=["id"],
        aggregations=[
            {
                "column": "chunk_id",
                "operation": "array_agg",
                "to": "text_units",
            }
        ],
    )

    rejoined = docs_with_text_units.merge(
        source,
        on="id",
        how="right",
    )
    rejoined.rename(columns={"text": "raw_content"}, inplace=True)
    rejoined["id"] = rejoined["id"].astype(str)

    # attribute columns are converted to strings and then collapsed into a single json object
    if document_attribute_columns:
        for column in document_attribute_columns:
            rejoined[column] = rejoined[column].astype(str)
        rejoined["attributes"] = rejoined[document_attribute_columns].apply(
            lambda row: {**row},
            axis=1,
        )
        rejoined.drop(columns=document_attribute_columns, inplace=True)
        rejoined.reset_index()

    return create_verb_result(
        cast(
            Table,
            rejoined,
        )
    )
Repository: microsoft/graphrag
File: graphrag/index/workflows/v1/subflows/create_base_documents.py (Python)

Code before:
        how="inner",
    )

    docs_with_text_units = aggregate_df(
        joined,
        groupby=["id"],
        aggregations=[
            {
                "column": "chunk_id",
                "operation": "array_agg",
                "to": "text_units",
            }
        ],
    )

    rejoined = docs_with_text_units.merge(
        source,
        left_on="id",
        right_on="id",
        how="right",
    )
    rejoined.rename(columns={"text": "raw_content"}, inplace=True)
    rejoined["id"] = rejoined["id"].astype(str)

    # attribute columns are converted to strings and then collapsed into a single json object
    if document_attribute_columns is not None and len(document_attribute_columns) > 0:
        for column in document_attribute_columns:
            rejoined[column] = rejoined[column].astype(str)
        rejoined["attributes"] = rejoined[document_attribute_columns].apply(
            lambda row: ({**row}),
            axis=1,
        )
        rejoined.drop(columns=document_attribute_columns, inplace=True)
        rejoined.reset_index()

    return create_verb_result(
        cast(
            Table,
            rejoined,
        )
    )


Review comment (suggestion):
this condition could be replaced by `if document_attribute_columns:` (empty lists in python are False, and None is also False)

Code after:
        how="inner",
    )

    docs_with_text_units = aggregate_df(
        joined,
        groupby=["id"],
        aggregations=[
            {
                "column": "chunk_id",
                "operation": "array_agg",
                "to": "text_units",
            }
        ],
    )

    rejoined = docs_with_text_units.merge(
        source,
        on="id",
        how="right",
    )
    rejoined.rename(columns={"text": "raw_content"}, inplace=True)
    rejoined["id"] = rejoined["id"].astype(str)

    # attribute columns are converted to strings and then collapsed into a single json object
    if document_attribute_columns:
        for column in document_attribute_columns:
            rejoined[column] = rejoined[column].astype(str)
        rejoined["attributes"] = rejoined[document_attribute_columns].apply(
            lambda row: {**row},
            axis=1,
        )
        rejoined.drop(columns=document_attribute_columns, inplace=True)
        rejoined.reset_index()

    return create_verb_result(
        cast(
            Table,
            rejoined,
        )
    )