ronantakizawa/codereview-bench huggingface dataset:
Repository: Cinnamon/kotaemon
File: libs/kotaemon/kotaemon/loaders/azureai_document_intelligence_loader.py (Python)

Code before:
            table_metadata = {
                "type": "table",
                "page_label": page_number,
                "table_origin": text_content[offset : offset + length],
            }
            table_metadata.update(metadata)

            tables.append(
                Document(
                    text=text_content[offset : offset + length],
                    metadata=table_metadata,
                )
            )
            removed_spans += table_desc["spans"]

        removed_spans = sorted(removed_spans, key=lambda x: x["offset"], reverse=True)
        for span in removed_spans:
            text_content = (
                text_content[: span["offset"]]
                + text_content[span["offset"] + span["length"] :]
            )

        # save the text content into markdown format
        if self.cache_dir is not None:
            print(file_name)
            with open(self.cache_dir / f"{file_name}.md", "w") as f:
                f.write(text_content)
        return [Document(content=text_content, metadata=metadata)] + figures + tables


Review comment (performance):
Since self.cache_dir can be string, which will not work with `/` operator, should use `Path(self.cache_dir)` instead.

Code after:
                "type": "table",
                "page_label": page_number,
                "table_origin": text_content[offset : offset + length],
            }
            table_metadata.update(metadata)

            tables.append(
                Document(
                    text=text_content[offset : offset + length],
                    metadata=table_metadata,
                )
            )
            removed_spans += table_desc["spans"]
        # save the text content into markdown format
        if self.cache_dir is not None:
            with open(Path(self.cache_dir) / f"{file_name.stem}.md", "w") as f:
                f.write(text_content)

        removed_spans = sorted(removed_spans, key=lambda x: x["offset"], reverse=True)
        for span in removed_spans:
            text_content = (
                text_content[: span["offset"]]
                + text_content[span["offset"] + span["length"] :]
            )

        return [Document(content=text_content, metadata=metadata)] + figures + tables
Repository: Cinnamon/kotaemon
File: libs/kotaemon/kotaemon/loaders/azureai_document_intelligence_loader.py (Python)

Code before:
            table_metadata = {
                "type": "table",
                "page_label": page_number,
                "table_origin": text_content[offset : offset + length],
            }
            table_metadata.update(metadata)

            tables.append(
                Document(
                    text=text_content[offset : offset + length],
                    metadata=table_metadata,
                )
            )
            removed_spans += table_desc["spans"]

        removed_spans = sorted(removed_spans, key=lambda x: x["offset"], reverse=True)
        for span in removed_spans:
            text_content = (
                text_content[: span["offset"]]
                + text_content[span["offset"] + span["length"] :]
            )

        # save the text content into markdown format
        if self.cache_dir is not None:
            with open(Path(self.cache_dir) / f"{file_name.stem}.md", "w") as f:
                f.write(text_content)
        return [Document(content=text_content, metadata=metadata)] + figures + tables


Review comment (refactor):
@jacky0218 I think you should perform the full Markdown save before this chunk

```
        for span in removed_spans:
            text_content = (
                text_content[: span["offset"]]
                + text_content[span["offset"] + span["length"] :]
            )
```

Since this chunk will remove the tables and figures to avoid duplicates in our database.

Code after:
            table_metadata = {
                "type": "table",
                "page_label": page_number,
                "table_origin": text_content[offset : offset + length],
            }
            table_metadata.update(metadata)

            tables.append(
                Document(
                    text=text_content[offset : offset + length],
                    metadata=table_metadata,
                )
            )
            removed_spans += table_desc["spans"]
        # save the text content into markdown format
        if self.cache_dir is not None:
            with open(Path(self.cache_dir) / f"{file_name.stem}.md", "w") as f:
                f.write(text_content)

        removed_spans = sorted(removed_spans, key=lambda x: x["offset"], reverse=True)
        for span in removed_spans:
            text_content = (
                text_content[: span["offset"]]
                + text_content[span["offset"] + span["length"] :]
            )

        return [Document(content=text_content, metadata=metadata)] + figures + tables
Repository: Cinnamon/kotaemon
File: scripts/run_macos.sh (Shell)

Code before:

# Main script execution

# move two levels up from the dir where this script resides
cd "$(
    cd -- "$(dirname "$0")" >/dev/null 2>&1
    pwd -P
)" && cd ..

install_dir="$(pwd)/install_dir"
conda_root="${install_dir}/conda"
env_dir="${install_dir}/env"
python_version="3.10"

check_path_for_spaces

read -p "Do you want to install Miniconda (recommended) y/n: " install_conda_choice
# Convert user input to lowercase
if [[ "$install_conda_choice" == "y" || "$install_conda_choice" == "Y" ]]; then
    install_miniconda
    print_highlight "Creating conda environment"
    create_conda_env "$python_version"
    activate_conda_env
else
    echo "miniconda installation skipped."
fi


print_highlight "Installing requirements"
install_dependencies

print_highlight "Setting up a local model"
setup_local_model

print_highlight "Launching Kotaemon in your browser, please wait..."
launch_ui

if [[ "$install_conda_choice" == "y" || "$install_conda_choice" == "Y" ]]; then
    deactivate_conda_env
fi

read -p "Press enter to continue"


Review comment (security):
We initially created `scripts/run_*` as installers for non-dev end users. They may not be familiar with Python and conda, and to avoid permission issues, we managed conda ourselves without caring if it already existed system-wide. This way, we didn't require user confirmation to install it. Additionally, miniconda is relatively small, so it didn't add much overhead to the application.

Code after:

install_dir="$(pwd)/install_dir"
conda_root="${install_dir}/conda"
env_dir="${install_dir}/env"
python_version="3.10"

pdf_js_version="4.0.379"
pdf_js_dist_name="pdfjs-${pdf_js_version}-dist"
pdf_js_dist_url="https://github.com/mozilla/pdf.js/releases/download/v${pdf_js_version}/${pdf_js_dist_name}.zip"
target_pdf_js_dir="$(pwd)/libs/ktem/ktem/assets/prebuilt/${pdf_js_dist_name}"

check_path_for_spaces

print_highlight "Setting up Miniconda"
install_miniconda

print_highlight "Creating conda environment"
create_conda_env "$python_version"
activate_conda_env

print_highlight "Installing requirements"
install_dependencies

print_highlight "Downloading and unzipping PDF.js"
download_and_unzip $pdf_js_dist_url $target_pdf_js_dir

print_highlight "Setting up a local model"
setup_local_model

print_highlight "Launching Kotaemon in your browser, please wait..."
launch_ui $target_pdf_js_dir

deactivate_conda_env

read -p "Press enter to continue"
Repository: ClickHouse/ClickHouse
File: base/base/wide_integer_impl.h (C/C++)

Code before:
#include <tuple>
#include <limits>


// NOLINTBEGIN(*)

/// Use same extended double for all platforms
#if (LDBL_MANT_DIG == 64)
#define CONSTEXPR_FROM_DOUBLE constexpr
using FromDoubleIntermediateType = long double;
#else
#include <boost/math/special_functions/fpclassify.hpp>
#include <boost/multiprecision/cpp_bin_float.hpp>
/// `wide_integer_from_builtin` can't be constexpr with non-literal `cpp_bin_float_double_extended`
#define CONSTEXPR_FROM_DOUBLE
using FromDoubleIntermediateType = boost::multiprecision::cpp_bin_float_double_extended;
#endif

namespace CityHash_v1_0_2 { struct uint128; }

namespace wide
{

constexpr bool supportsBitInt256()
{
#if defined(__x86_64__) && defined(__clang__) && (__clang_major__ > 14 || (__clang_major__ == 14 && __clang_minor__ >= 0))
    return true;
#else
    return false;
#endif
}

#if defined(__x86_64__) && defined(__clang__) && (__clang_major__ > 14 || (__clang_major__ == 14 && __clang_minor__ >= 0))
#    pragma clang diagnostic push
#    pragma clang diagnostic ignored "-Wbit-int-extension"
using BitInt256 = signed _BitInt(256);
using BitUInt256 = unsigned _BitInt(256);
#    pragma clang diagnostic pop

struct Error {};

template <typename Signed>
struct ConstructBitInt256
{
    using Type = Error;
};

template <>
struct ConstructBitInt256<signed>
{
    using Type = BitInt256;

Review comment (suggestion):
Which simply means "yes", because we only use clang and we use minimum clang-19 as of today.

`#if defined(__x86_64__)` will be enough.

Code after:
#include <tuple>
#include <limits>


// NOLINTBEGIN(*)

/// Use same extended double for all platforms
#if (LDBL_MANT_DIG == 64)
#define CONSTEXPR_FROM_DOUBLE constexpr
using FromDoubleIntermediateType = long double;
#else
#include <boost/math/special_functions/fpclassify.hpp>
#include <boost/multiprecision/cpp_bin_float.hpp>
/// `wide_integer_from_builtin` can't be constexpr with non-literal `cpp_bin_float_double_extended`
#define CONSTEXPR_FROM_DOUBLE
using FromDoubleIntermediateType = boost::multiprecision::cpp_bin_float_double_extended;
#endif

namespace CityHash_v1_0_2 { struct uint128; }

namespace wide
{

constexpr bool supportsBitInt256()
{
#if defined(__x86_64__)
    return true;
#else
    return false;
#endif
}

#if defined(__x86_64__)
#    pragma clang diagnostic push
#    pragma clang diagnostic ignored "-Wbit-int-extension"
using BitInt256 = signed _BitInt(256);
using BitUInt256 = unsigned _BitInt(256);
#    pragma clang diagnostic pop

struct Error {};

template <typename Signed>
struct ConstructBitInt256
{
    using Type = Error;
};

template <>
struct ConstructBitInt256<signed>
{
    using Type = BitInt256;
Repository: ClickHouse/ClickHouse
File: tests/queries/0_stateless/01043_geo_distance.sql (SQL)

Code before:
SET geo_distance_returns_float64_on_float64_arguments = 0;

SELECT greatCircleDistance(0., 0., 0., 1.);
SELECT greatCircleDistance(0., 89., 0, 90.);

SELECT geoDistance(0., 0., 0., 1.);
SELECT geoDistance(0., 89., 0., 90.);

SELECT greatCircleDistance(0., 0., 90., 0.);
SELECT greatCircleDistance(0., 0., 0., 90.);

SELECT geoDistance(0., 0., 90., 0.);
SELECT geoDistance(0., 0., 0., 90.);

SET geo_distance_returns_float64_on_float64_arguments = 1;

SELECT greatCircleDistance(0., 0., 0., 1.);
SELECT greatCircleDistance(0., 89., 0, 90.);

SELECT geoDistance(0., 0., 0., 1.);
SELECT geoDistance(0., 89., 0., 90.);

SELECT round(greatCircleDistance(0., 0., 90., 0.), 8);
SELECT round(greatCircleDistance(0., 0., 0., 90.), 8);

SELECT round(geoDistance(0., 0., 90., 0.), 8);
SELECT round(geoDistance(0., 0., 0., 90.), 8);


Review comment (suggestion):
Couldn't we just adjust the expected results and be done (i.e. not `round` the result)?

(same in the other test below)

Code after:
SET geo_distance_returns_float64_on_float64_arguments = 0;

SELECT greatCircleDistance(0., 0., 0., 1.);
SELECT greatCircleDistance(0., 89., 0, 90.);

SELECT geoDistance(0., 0., 0., 1.);
SELECT geoDistance(0., 89., 0., 90.);

SELECT greatCircleDistance(0., 0., 90., 0.);
SELECT greatCircleDistance(0., 0., 0., 90.);

SELECT geoDistance(0., 0., 90., 0.);
SELECT geoDistance(0., 0., 0., 90.);

SET geo_distance_returns_float64_on_float64_arguments = 1;

SELECT greatCircleDistance(0., 0., 0., 1.);
SELECT greatCircleDistance(0., 89., 0, 90.);

SELECT geoDistance(0., 0., 0., 1.);
SELECT geoDistance(0., 89., 0., 90.);

SELECT greatCircleDistance(0., 0., 90., 0.);
SELECT greatCircleDistance(0., 0., 0., 90.);

SELECT geoDistance(0., 0., 90., 0.);
SELECT geoDistance(0., 0., 0., 90.);
Repository: ClickHouse/ClickHouse
File: contrib/libllvmlibc-cmake/llvmlibc_dispatch.cpp (C++)

Code before:
// Runtime dispatch for llvm-libc math functions (x86_64 only).
// Selects between x86-64-v2 (SSE4.2) and x86-64-v3 (AVX/FMA) variants based on CPU capabilities.

#if defined(__x86_64__)

#include <Common/CPUID.h>

namespace {
    // 0 = unknown, 1 = no FMA, 2 = has FMA
    int g_fma_status = 0;

    __attribute__((always_inline))
    inline bool cpu_has_fma()
    {
        int status = g_fma_status;
        if (__builtin_expect(status != 0, 1))
            return status == 2;

        bool has_fma = DB::CPU::haveFMA();
        g_fma_status = has_fma ? 2 : 1;
        return has_fma;
    }
}


#define DISPATCH_1(ret, name, T1) \
    namespace __llvm_libc__x86_64_v3 { ret name(T1); } \
    namespace __llvm_libc__x86_64_v2 { ret name(T1); } \
    extern "C" ret name(T1 a1) { \
        return cpu_has_fma() ? __llvm_libc__x86_64_v3::name(a1) \
                             : __llvm_libc__x86_64_v2::name(a1); \
    }

#define DISPATCH_2(ret, name, T1, T2) \
    namespace __llvm_libc__x86_64_v3 { ret name(T1, T2); } \
    namespace __llvm_libc__x86_64_v2 { ret name(T1, T2); } \
    extern "C" ret name(T1 a1, T2 a2) { \
        return cpu_has_fma() ? __llvm_libc__x86_64_v3::name(a1, a2) \
                             : __llvm_libc__x86_64_v2::name(a1, a2); \
    }


Review comment (suggestion):
Not sure but could this be simplified with `g_fma_status` as a `static` global variable? Something like:

```cpp

bool cpu_has_fma()
{
    static const bool has_fma = DB::CPU::haveFMA();
    return has_fma;
}
```

Code after:
// Runtime dispatch for llvm-libc math functions (x86_64 only).
// Selects between x86-64-v2 (SSE4.2) and x86-64-v3 (AVX/FMA) variants based on CPU capabilities.

#if defined(__x86_64__)

#include <Common/CPUID.h>

namespace {
    __attribute__((always_inline))
    inline bool cpu_has_fma()
    {
        static const bool has_fma = DB::CPU::haveFMA();
        return has_fma;
    }
}


#define DISPATCH_1(ret, name, T1) \
    namespace __llvm_libc__x86_64_v3 { ret name(T1); } \
    namespace __llvm_libc__x86_64_v2 { ret name(T1); } \
    extern "C" ret name(T1 a1) { \
        return cpu_has_fma() ? __llvm_libc__x86_64_v3::name(a1) \
                             : __llvm_libc__x86_64_v2::name(a1); \
    }

#define DISPATCH_2(ret, name, T1, T2) \
    namespace __llvm_libc__x86_64_v3 { ret name(T1, T2); } \
    namespace __llvm_libc__x86_64_v2 { ret name(T1, T2); } \
    extern "C" ret name(T1 a1, T2 a2) { \
        return cpu_has_fma() ? __llvm_libc__x86_64_v3::name(a1, a2) \
                             : __llvm_libc__x86_64_v2::name(a1, a2); \
    }

#define DISPATCH_3(ret, name, T1, T2, T3) \
    namespace __llvm_libc__x86_64_v3 { ret name(T1, T2, T3); } \
    namespace __llvm_libc__x86_64_v2 { ret name(T1, T2, T3); } \
    extern "C" ret name(T1 a1, T2 a2, T3 a3) { \
        return cpu_has_fma() ? __llvm_libc__x86_64_v3::name(a1, a2, a3) \
                             : __llvm_libc__x86_64_v2::name(a1, a2, a3); \
    }
Repository: ClickHouse/ClickHouse
File: src/Storages/ObjectStorage/IObjectIterator.cpp (C++)

Code before:
    {
        auto result = pending_objects_info.front();
        pending_objects_info.pop();
        return result;
    }
    auto last_object_info = iterator->next(id);
    if (!last_object_info)
        return {};


    auto splitter = FormatFactory::instance().getSplitter(format);
    if (splitter)
    {
        auto buffer = createReadBuffer(last_object_info->relative_path_with_metadata, object_storage, getContext(), log);
        size_t bucket_size = getContext()->getSettingsRef()[Setting::cluster_table_function_buckets_batch_size];
        auto file_bucket_info = splitter->splitToBuckets(bucket_size, *buffer, format_settings);
        for (const auto & file_bucket : file_bucket_info)
        {
            auto copy_object_info = *last_object_info;
            copy_object_info.file_bucket_info = file_bucket;
            pending_objects_info.push(std::make_shared<ObjectInfo>(copy_object_info));
        }
    }

    if (pending_objects_info.empty())
        return {};
    auto result = pending_objects_info.front();
    pending_objects_info.pop();
    return result;
}

String ObjectInfo::getIdentifier() const
{
    String result = getPath();
    if (file_bucket_info)
        result += file_bucket_info->getIdentifier();
    return result;
}

}


Review comment (performance):
Why can we have pending_objecs_info empty here? 
I have two answers: splitter is null and buffer is empty.
1) I think, that we should fallback and process the whole file if splitter is null, shouldn't we.
2) If buffer is empty, I think we can use query_settings.skip_empty_files to understand if it is ok for us.
Why shouldn't we take a next file/batch for the iteration? 

Code after:
    {
        auto last_object_info = iterator->next(id);
        if (!last_object_info)
            return {};

        auto splitter = FormatFactory::instance().getSplitter(format);
        if (splitter)
        {
            auto buffer = createReadBuffer(last_object_info->relative_path_with_metadata, object_storage, getContext(), log);
            size_t bucket_size = getContext()->getSettingsRef()[Setting::cluster_table_function_buckets_batch_size];
            auto file_bucket_info = splitter->splitToBuckets(bucket_size, *buffer, format_settings);
            for (const auto & file_bucket : file_bucket_info)
            {
                auto copy_object_info = *last_object_info;
                copy_object_info.file_bucket_info = file_bucket;
                pending_objects_info.push(std::make_shared<ObjectInfo>(copy_object_info));
            }
        }
    }

    auto result = pending_objects_info.front();
    pending_objects_info.pop();
    return result;
}

String ObjectInfo::getIdentifier() const
{
    String result = getPath();
    if (file_bucket_info)
        result += file_bucket_info->getIdentifier();
    return result;
}

}
Repository: ClickHouse/ClickHouse
File: src/Databases/IDatabase.h (C/C++)

Code before:

namespace DB
{


struct Settings;
struct ConstraintsDescription;
struct IndicesDescription;
struct StorageInMemoryMetadata;
struct StorageID;
class ASTCreateQuery;
struct AlterCommand;
class AlterCommands;
class SettingsChanges;
using DictionariesWithID = std::vector<std::pair<String, UUID>>;
struct ParsedTablesMetadata;
struct QualifiedTableName;
class IRestoreCoordination;

struct LightWeightTableDetails
{
    String name;
};

class IDatabaseTablesIterator
{
public:
    virtual void next() = 0;
    virtual bool isValid() const = 0;

    virtual const String & name() const = 0;

    /// This method can return nullptr if it's Lazy database
    /// (a database with support for lazy tables loading
    /// - it maintains a list of tables but tables are loaded lazily).
    virtual const StoragePtr & table() const = 0;

    explicit IDatabaseTablesIterator(const String & database_name_) : database_name(database_name_) { }
    explicit IDatabaseTablesIterator(String && database_name_) : database_name(std::move(database_name_)) { }

    virtual ~IDatabaseTablesIterator() = default;

    virtual UUID uuid() const { return UUIDHelpers::Nil; }

    const String & databaseName() const { assert(!database_name.empty()); return database_name; }

protected:
    String database_name;
};

/// Copies list of tables and iterates through such snapshot.

Review comment (suggestion):
The struct `LightWeightTableDetails` lacks documentation explaining its purpose and relationship to the heavier table details. Add a comment describing when this lightweight version should be used versus full table objects.

Code after:


namespace DB
{


struct Settings;
struct ConstraintsDescription;
struct IndicesDescription;
struct StorageInMemoryMetadata;
struct StorageID;
class ASTCreateQuery;
struct AlterCommand;
class AlterCommands;
class SettingsChanges;
using DictionariesWithID = std::vector<std::pair<String, UUID>>;
struct ParsedTablesMetadata;
struct QualifiedTableName;
class IRestoreCoordination;

/// This structure is returned when getLightweightTablesIterator is called
/// It contains basic details of the table, currently only the table name
struct LightWeightTableDetails
{
    String name;
};

class IDatabaseTablesIterator
{
public:
    virtual void next() = 0;
    virtual bool isValid() const = 0;

    virtual const String & name() const = 0;

    /// This method can return nullptr if it's Lazy database
    /// (a database with support for lazy tables loading
    /// - it maintains a list of tables but tables are loaded lazily).
    virtual const StoragePtr & table() const = 0;

    explicit IDatabaseTablesIterator(const String & database_name_) : database_name(database_name_) { }
    explicit IDatabaseTablesIterator(String && database_name_) : database_name(std::move(database_name_)) { }

    virtual ~IDatabaseTablesIterator() = default;

    virtual UUID uuid() const { return UUIDHelpers::Nil; }

    const String & databaseName() const { assert(!database_name.empty()); return database_name; }

protected:
    String database_name;
Repository: ClickHouse/ClickHouse
File: tests/queries/0_stateless/03803_insert_on_connection_drop.sh (Shell)

Code before:
    curl -sS -d@- "$CLICKHOUSE_URL"

(
i=1
while true; do
    ts=$(date +%s)
    echo "$i,hello-$i,$ts,3" || exit 0
    ((i++))
done
) | curl -sS -N --no-buffer \
    -T - \
    -X POST \
    -H "Content-Type: text/csv" \
    -H "Transfer-Encoding: chunked" \
    "$CLICKHOUSE_INSERT_URL" 2>&1 &

PIPELINE_PID=$!

sleep 8

kill -9 $PIPELINE_PID 2>/dev/null

wait $PIPELINE_PID 2>/dev/null


sleep 1

$CLICKHOUSE_CLIENT -q "SYSTEM FLUSH LOGS query_log, part_log;"


parts_count=$(${CLICKHOUSE_CLIENT} --query "
SELECT count(*) 
FROM system.part_log 
WHERE table = '${CLICKHOUSE_TABLE}' 
  AND event_type = 'NewPart'
  AND query_id = (
        SELECT argMax(query_id, event_time) 
        FROM system.query_log 
        WHERE query LIKE CONCAT('%INSERT INTO ', '${CLICKHOUSE_TABLE}', '%') 
          AND current_database = currentDatabase()
    )
")

echo "Number of parts created: ${parts_count}"

echo "DROP TABLE IF EXISTS ${CLICKHOUSE_TABLE}" | \
    curl -sS -d@- "$CLICKHOUSE_URL"



Review comment (bug):
On the internal testing this test fails due to difference in result from the reference -- it gets 0 parts instead of 1 part. Let's try to increase the duration of sleep. The test suit that fails is related to s3 in asan build, so this is likely a latency issue.

Code after:
    curl -sS -d@- "$CLICKHOUSE_URL"

(
i=1
while true; do
    ts=$(date +%s)
    echo "$i,hello-$i,$ts,3" || exit 0
    ((i++))
done
) | curl -sS -N --no-buffer \
    -T - \
    -X POST \
    -H "Content-Type: text/csv" \
    -H "Transfer-Encoding: chunked" \
    "$CLICKHOUSE_INSERT_URL" 2>&1 &

PIPELINE_PID=$!

sleep 15

kill -9 $PIPELINE_PID 2>/dev/null

wait $PIPELINE_PID 2>/dev/null


sleep 5

$CLICKHOUSE_CLIENT -q "SYSTEM FLUSH LOGS query_log, part_log;"


parts_count=$(${CLICKHOUSE_CLIENT} --query "
SELECT count(*) 
FROM system.part_log 
WHERE table = '${CLICKHOUSE_TABLE}' 
  AND event_type = 'NewPart'
  AND query_id = (
        SELECT argMax(query_id, event_time) 
        FROM system.query_log 
        WHERE query LIKE CONCAT('%INSERT INTO ', '${CLICKHOUSE_TABLE}', '%') 
          AND current_database = currentDatabase()
    )
")

echo "Number of parts created: ${parts_count}"

echo "DROP TABLE IF EXISTS ${CLICKHOUSE_TABLE}" | \
    curl -sS -d@- "$CLICKHOUSE_URL"