ronantakizawa/codereview-bench huggingface dataset:
Repository: risingwavelabs/risingwave
File: src/ctl/src/cmd_impl/throttle.rs (Rust)

Code before:
// See the License for the specific language governing permissions and
// limitations under the License.

use risingwave_pb::common::PbThrottleType;
use risingwave_pb::meta::PbThrottleTarget;

use crate::ThrottleCommandArgs;
use crate::common::CtlContext;

pub async fn apply_throttle(
    context: &CtlContext,
    kind: PbThrottleTarget,
    params: ThrottleCommandArgs,
) -> anyhow::Result<()> {
    let meta_client = context.meta_client().await?;
    // Infer throttle type from target for backward compatibility.
    let throttle_type = match kind {
        PbThrottleTarget::Source => PbThrottleType::Source,
        PbThrottleTarget::Mv => PbThrottleType::Backfill,
        PbThrottleTarget::Sink => PbThrottleType::Sink,
        PbThrottleTarget::Table | PbThrottleTarget::Fragment | PbThrottleTarget::Unspecified => {
            // Default to Backfill for unspecified/unsupported combinations; user should use SQL for table throttling
            PbThrottleType::Backfill
        }
    };
    meta_client
        .apply_throttle(kind, throttle_type, params.id, params.rate)
        .await?;
    Ok(())
}


Review comment (suggestion):
This should be fixed, we should supply both throttle type and throttle target.

Code after:
// See the License for the specific language governing permissions and
// limitations under the License.

use risingwave_pb::common::PbThrottleType;
use risingwave_pb::meta::PbThrottleTarget;

use crate::common::CtlContext;
use crate::{ThrottleCommandArgs, ThrottleTypeArg};

pub async fn apply_throttle(
    context: &CtlContext,
    kind: PbThrottleTarget,
    params: ThrottleCommandArgs,
) -> anyhow::Result<()> {
    let meta_client = context.meta_client().await?;
    let throttle_type = match params.throttle_type {
        ThrottleTypeArg::Dml => PbThrottleType::Dml,
        ThrottleTypeArg::Backfill => PbThrottleType::Backfill,
        ThrottleTypeArg::Source => PbThrottleType::Source,
        ThrottleTypeArg::Sink => PbThrottleType::Sink,
    };

    meta_client
        .apply_throttle(kind, throttle_type, params.id, params.rate)
        .await?;
    Ok(())
}
Repository: risingwavelabs/risingwave
File: src/ctl/src/lib.rs (Rust)

Code before:
    Source(ThrottleCommandArgs),
    Mv(ThrottleCommandArgs),
    Sink(ThrottleCommandArgs),
}

#[derive(Clone, Debug, clap::ValueEnum)]
pub enum ThrottleTypeArg {
    Dml,
    Backfill,
    Source,
    Sink,
}

#[derive(Clone, Debug, Args)]
pub struct ThrottleCommandArgs {
    /// The ID of the object to throttle
    #[clap(long, required = true)]
    id: u32,
    /// The rate limit to apply
    #[clap(long)]
    rate: Option<u32>,
    /// The type of throttle to apply
    #[clap(long, value_enum, required = true)]
    throttle_type: ThrottleTypeArg,
}

#[derive(Subcommand, Clone, Debug)]
pub enum ProfileCommands {
    /// CPU profile
    Cpu {
        /// The time to active profiling for (in seconds)
        #[clap(short, long = "sleep")]
        sleep: u64,
    },
    /// Heap profile
    Heap {
        /// The output directory of the dumped file
        #[clap(long = "dir")]
        dir: Option<String>,
    },
}

/// Start `risectl` with the given options.
/// Cancel the operation when the given `shutdown` token triggers.
/// Log and abort the process if any error occurs.
///
/// Note: use [`start_fallible`] if you want to call functionalities of `risectl`
/// in an embedded manner.
pub async fn start(opts: CliOpts, shutdown: CancellationToken) {
    let context = CtlContext::default();


Review comment (suggestion):
also if using id, there is no need for ThrottleTypeArg. Id is unique in the cluster, using id can find the object and determine the throttle_type.

Code after:
#[derive(Subcommand, Clone, Debug)]
enum ThrottleCommands {
    Source(ThrottleCommandArgs),
    Mv(ThrottleCommandArgs),
    Sink(ThrottleCommandArgs),
}

#[derive(Clone, Debug, clap::ValueEnum)]
pub enum ThrottleTypeArg {
    Dml,
    Backfill,
    Source,
    Sink,
}

#[derive(Clone, Debug, Args)]
pub struct ThrottleCommandArgs {
    /// The ID of the object to throttle
    #[clap(long, required = true)]
    id: u32,
    /// The rate limit to apply
    #[clap(long)]
    rate: Option<u32>,
    /// The type of throttle to apply
    #[clap(long, value_enum, required = true)]
    throttle_type: ThrottleTypeArg,
}

#[derive(Subcommand, Clone, Debug)]
pub enum ProfileCommands {
    /// CPU profile
    Cpu {
        /// The time to active profiling for (in seconds)
        #[clap(short, long = "sleep")]
        sleep: u64,
        /// Target worker types. Repeatable. Defaults to frontend, compute-node, and compactor.
        #[clap(long = "worker-type", value_name = "TYPE")]
        worker_types: Vec<ProfileWorkerType>,
    },
    /// Heap profile
    Heap {
        /// The output directory of the dumped file
        #[clap(long = "dir")]
        dir: Option<String>,
        /// Target worker types. Repeatable. Defaults to frontend, compute-node, and compactor.
        #[clap(long = "worker-type", value_name = "TYPE")]
        worker_types: Vec<ProfileWorkerType>,
    },
}

/// Start `risectl` with the given options.
Repository: risingwavelabs/risingwave
File: src/frontend/src/datafusion/error.rs (Rust)

Code before:
use std::backtrace::Backtrace;
use std::fmt::Display;

use datafusion_common::DataFusionError;
use risingwave_common::error::BoxedError;

#[derive(Debug)]
struct ToDataFusionErrorWrapper(BoxedError, Option<Backtrace>);

impl Display for ToDataFusionErrorWrapper {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        write!(f, "{}", self.0)?;
        if let Some(bt) = std::error::request_ref::<Backtrace>(self.0.as_ref()) {
            write!(f, "{}{}", DataFusionError::BACK_TRACE_SEP, bt)?;
        } else if let Some(bt) = &self.1 {
            write!(f, "{}{}", DataFusionError::BACK_TRACE_SEP, bt)?;
        }
        Ok(())
    }
}

impl std::error::Error for ToDataFusionErrorWrapper {
    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
        self.0.source()
    }

    fn provide<'a>(&'a self, request: &mut std::error::Request<'a>) {
        if let Some(source) = self.source() {
            source.provide(request);
        }
    }
}

// DataFusionError doesn't use `provide` to print or provide backtrace, so we need to
// put backtrace in the Display impl above like other DataFusionErrors do.
pub fn to_datafusion_error(error: impl Into<BoxedError>) -> DataFusionError {
    let error = error.into();
    let mut bt = None;
    if std::error::request_ref::<Backtrace>(error.as_ref()).is_none() {
        bt = Some(Backtrace::capture());
    }
    DataFusionError::External(Box::new(ToDataFusionErrorWrapper(error, bt)))
}


Review comment (suggestion):
The public function `to_datafusion_error` is missing rustdoc comments. Since this is a new public API function being exported from the module, it should have documentation explaining its purpose, parameters, return value, and how it handles backtraces. This is also noted as incomplete in the PR's checklist ("I have written necessary rustdoc comments").

Code after:

impl std::error::Error for ToDataFusionErrorWrapper {
    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
        self.0.source()
    }

    fn provide<'a>(&'a self, request: &mut std::error::Request<'a>) {
        if let Some(bt) = &self.1 {
            request.provide_ref(bt);
        }
        if let Some(source) = self.source() {
            source.provide(request);
        }
    }
}

/// Converts a boxed error into a `DataFusionError`, capturing a backtrace if one is not already
/// present in the error.
pub fn to_datafusion_error(error: impl Into<BoxedError>) -> DataFusionError {
    let error = error.into();
    let mut bt = None;
    if std::error::request_ref::<Backtrace>(error.as_ref()).is_none() {
        bt = Some(Backtrace::capture());
    }
    DataFusionError::External(Box::new(ToDataFusionErrorWrapper(error, bt)))
}
Repository: risingwavelabs/risingwave
File: scripts/check/check-trailing-spaces.sh (Shell)

Code before:
    echo "  -s, --skip-dir <dir>   Skip a directory (can be specified multiple times)."
    echo "                          Example: $self --skip-dir target --skip-dir src/tests"
    echo "  -h, --help             Show this help message and exit."
}

fix=false
skip_dirs=()
while [ $# -gt 0 ]; do
    case $1 in
    -f | --fix)
        fix=true
        ;;
    -s | --skip-dir)
        shift
        if [ $# -eq 0 ]; then
            echo -e "${RED}${BOLD}$self: missing argument for --skip-dir${NONE}"
            print_help
            exit 1
        fi
        skip_dirs+=("$1")
        ;;
    -h | --help)
        print_help
        exit 0
        ;;
    *)
        echo -e "${RED}${BOLD}$self: invalid option \`$1\`\n${NONE}"
        print_help
        exit 1
        ;;
    esac
    shift
done

temp_file=$(mktemp)

echo -ne "${BLUE}"
# Build git pathspec exclusions. `--` separates revs/options from pathspecs.
# Default: search repo root, but skip `.cargo/` and `src/tests/regress/`.
git_grep_pathspec=(-- ':' ':!.cargo' ':!src/tests/regress')
if [ ${#skip_dirs[@]} -gt 0 ]; then
    for d in "${skip_dirs[@]}"; do
        # Normalize leading "./" to keep the pathspec consistent.
        d="${d#./}"
        git_grep_pathspec+=(":!${d}")
    done
fi

git grep -nIP --untracked '[[:space:]]+$' "${git_grep_pathspec[@]}" | tee "$temp_file" || true
echo -ne "${NONE}"


Review comment (suggestion):
Missing quotes around variable expansion. The variables $temp_file should be quoted as "$temp_file" to handle filenames with spaces correctly and follow bash best practices (especially since the script uses 'set -u' which requires careful variable handling).

Code after:
    echo "  -h, --help             Show this help message and exit."
}

fix=false
while [ $# -gt 0 ]; do
    case $1 in
    -f | --fix)
        fix=true
        ;;
    -h | --help)
        print_help
        exit 0
        ;;
    *)
        echo -e "${RED}${BOLD}$self: invalid option \`$1\`\n${NONE}"
        print_help
        exit 1
        ;;
    esac
    shift
done

temp_file=$(mktemp)

echo -ne "${BLUE}"
git grep -nIP --untracked '[[:space:]]+$' -- ':!src/tests/regress/data' | tee $temp_file || true
echo -ne "${NONE}"

bad_files=$(cat $temp_file | cut -f1 -d ':' | sort -u)
rm $temp_file

if [ ! -z "$bad_files" ]; then
    if [[ $fix == true ]]; then
        for file in $bad_files; do
            sed -i '' -e's/[[:space:]]*$//' "$file"
        done

        echo
        echo -e "${GREEN}${BOLD}All trailing spaces listed above have been cleaned.${NONE}"
        exit 0
    else
        echo
        echo -e "${RED}${BOLD}Please clean all the trailing spaces listed above.${NONE}"
        echo -e "${BOLD}You can run '$self --fix' for convenience.${NONE}"
        exit 1
    fi
else
    echo -e "${GREEN}${BOLD}No trailing spaces found.${NONE}"
    exit 0
fi
Repository: risingwavelabs/risingwave
File: scripts/check/check-trailing-spaces.sh (Shell)

Code before:

fix=false
skip_dirs=()
while [ $# -gt 0 ]; do
    case $1 in
    -f | --fix)
        fix=true
        ;;
    -s | --skip-dir)
        shift
        if [ $# -eq 0 ]; then
            echo -e "${RED}${BOLD}$self: missing argument for --skip-dir${NONE}"
            print_help
            exit 1
        fi
        skip_dirs+=("$1")
        ;;
    -h | --help)
        print_help
        exit 0
        ;;
    *)
        echo -e "${RED}${BOLD}$self: invalid option \`$1\`\n${NONE}"
        print_help
        exit 1
        ;;
    esac
    shift
done

temp_file=$(mktemp)

echo -ne "${BLUE}"
# Build git pathspec exclusions. `--` separates revs/options from pathspecs.
# Default: search repo root, but skip `.cargo/` and `src/tests/regress/`.
git_grep_pathspec=(-- ':' ':!.cargo' ':!src/tests/regress')
if [ ${#skip_dirs[@]} -gt 0 ]; then
    for d in "${skip_dirs[@]}"; do
        # Normalize leading "./" to keep the pathspec consistent.
        d="${d#./}"
        git_grep_pathspec+=(":!${d}")
    done
fi

git grep -nIP --untracked '[[:space:]]+$' "${git_grep_pathspec[@]}" | tee "$temp_file" || true
echo -ne "${NONE}"

bad_files=$(cat "$temp_file" | cut -f1 -d ':' | sort -u)
rm "$temp_file"

# Portable in-place sed (GNU vs BSD/macOS).

Review comment (suggestion):
Missing quotes around variable expansion. The variables $temp_file and $bad_files should be quoted to handle filenames with spaces correctly and follow bash best practices.

Code after:
while [ $# -gt 0 ]; do
    case $1 in
    -f | --fix)
        fix=true
        ;;
    -h | --help)
        print_help
        exit 0
        ;;
    *)
        echo -e "${RED}${BOLD}$self: invalid option \`$1\`\n${NONE}"
        print_help
        exit 1
        ;;
    esac
    shift
done

temp_file=$(mktemp)

echo -ne "${BLUE}"
git grep -nIP --untracked '[[:space:]]+$' -- ':!src/tests/regress/data' | tee $temp_file || true
echo -ne "${NONE}"

bad_files=$(cat $temp_file | cut -f1 -d ':' | sort -u)
rm $temp_file

if [ ! -z "$bad_files" ]; then
    if [[ $fix == true ]]; then
        for file in $bad_files; do
            sed -i '' -e's/[[:space:]]*$//' "$file"
        done

        echo
        echo -e "${GREEN}${BOLD}All trailing spaces listed above have been cleaned.${NONE}"
        exit 0
    else
        echo
        echo -e "${RED}${BOLD}Please clean all the trailing spaces listed above.${NONE}"
        echo -e "${BOLD}You can run '$self --fix' for convenience.${NONE}"
        exit 1
    fi
else
    echo -e "${GREEN}${BOLD}No trailing spaces found.${NONE}"
    exit 0
fi
Repository: risingwavelabs/risingwave
File: scripts/check/check-trailing-spaces.sh (Shell)

Code before:
skip_dirs=()
while [ $# -gt 0 ]; do
    case $1 in
    -f | --fix)
        fix=true
        ;;
    -s | --skip-dir)
        shift
        if [ $# -eq 0 ]; then
            echo -e "${RED}${BOLD}$self: missing argument for --skip-dir${NONE}"
            print_help
            exit 1
        fi
        skip_dirs+=("$1")
        ;;
    -h | --help)
        print_help
        exit 0
        ;;
    *)
        echo -e "${RED}${BOLD}$self: invalid option \`$1\`\n${NONE}"
        print_help
        exit 1
        ;;
    esac
    shift
done

temp_file=$(mktemp)

echo -ne "${BLUE}"
# Build git pathspec exclusions. `--` separates revs/options from pathspecs.
# Default: search repo root, but skip `.cargo/` and `src/tests/regress/`.
git_grep_pathspec=(-- ':' ':!.cargo' ':!src/tests/regress')
if [ ${#skip_dirs[@]} -gt 0 ]; then
    for d in "${skip_dirs[@]}"; do
        # Normalize leading "./" to keep the pathspec consistent.
        d="${d#./}"
        git_grep_pathspec+=(":!${d}")
    done
fi

git grep -nIP --untracked '[[:space:]]+$' "${git_grep_pathspec[@]}" | tee "$temp_file" || true
echo -ne "${NONE}"

bad_files=$(cat "$temp_file" | cut -f1 -d ':' | sort -u)
rm "$temp_file"

# Portable in-place sed (GNU vs BSD/macOS).
sed_inplace() {
    local expr=$1

Review comment (suggestion):
The condition '[ ! -z "$bad_files" ]' is not best practice in bash. It should be '[ -n "$bad_files" ]' for checking if a variable is non-empty. While functionally equivalent, the -n operator is the preferred and more readable way to check for non-empty strings.
```suggestion
if [ -n "$bad_files" ]; then
```

Code after:
    -f | --fix)
        fix=true
        ;;
    -h | --help)
        print_help
        exit 0
        ;;
    *)
        echo -e "${RED}${BOLD}$self: invalid option \`$1\`\n${NONE}"
        print_help
        exit 1
        ;;
    esac
    shift
done

temp_file=$(mktemp)

echo -ne "${BLUE}"
git grep -nIP --untracked '[[:space:]]+$' -- ':!src/tests/regress/data' | tee $temp_file || true
echo -ne "${NONE}"

bad_files=$(cat $temp_file | cut -f1 -d ':' | sort -u)
rm $temp_file

if [ ! -z "$bad_files" ]; then
    if [[ $fix == true ]]; then
        for file in $bad_files; do
            sed -i '' -e's/[[:space:]]*$//' "$file"
        done

        echo
        echo -e "${GREEN}${BOLD}All trailing spaces listed above have been cleaned.${NONE}"
        exit 0
    else
        echo
        echo -e "${RED}${BOLD}Please clean all the trailing spaces listed above.${NONE}"
        echo -e "${BOLD}You can run '$self --fix' for convenience.${NONE}"
        exit 1
    fi
else
    echo -e "${GREEN}${BOLD}No trailing spaces found.${NONE}"
    exit 0
fi
Repository: risingwavelabs/risingwave
File: scripts/check/check-trailing-spaces.sh (Shell)

Code before:
        fix=true
        ;;
    -s | --skip-dir)
        shift
        if [ $# -eq 0 ]; then
            echo -e "${RED}${BOLD}$self: missing argument for --skip-dir${NONE}"
            print_help
            exit 1
        fi
        skip_dirs+=("$1")
        ;;
    -h | --help)
        print_help
        exit 0
        ;;
    *)
        echo -e "${RED}${BOLD}$self: invalid option \`$1\`\n${NONE}"
        print_help
        exit 1
        ;;
    esac
    shift
done

temp_file=$(mktemp)

echo -ne "${BLUE}"
# Build git pathspec exclusions. `--` separates revs/options from pathspecs.
# Default: search repo root, but skip `.cargo/` and `src/tests/regress/`.
git_grep_pathspec=(-- ':' ':!.cargo' ':!src/tests/regress')
if [ ${#skip_dirs[@]} -gt 0 ]; then
    for d in "${skip_dirs[@]}"; do
        # Normalize leading "./" to keep the pathspec consistent.
        d="${d#./}"
        git_grep_pathspec+=(":!${d}")
    done
fi

git grep -nIP --untracked '[[:space:]]+$' "${git_grep_pathspec[@]}" | tee "$temp_file" || true
echo -ne "${NONE}"

bad_files=$(cat "$temp_file" | cut -f1 -d ':' | sort -u)
rm "$temp_file"

# Portable in-place sed (GNU vs BSD/macOS).
sed_inplace() {
    local expr=$1
    local file=$2
    if sed --version >/dev/null 2>&1; then
        # GNU sed
        sed -i -e "${expr}" "${file}"

Review comment (suggestion):
The for loop iterates over unquoted $bad_files, which will break if any filename contains spaces. The variable should be quoted, or the loop should use a while read loop like the original implementation to properly handle filenames with special characters.

Code after:
        print_help
        exit 0
        ;;
    *)
        echo -e "${RED}${BOLD}$self: invalid option \`$1\`\n${NONE}"
        print_help
        exit 1
        ;;
    esac
    shift
done

temp_file=$(mktemp)

echo -ne "${BLUE}"
git grep -nIP --untracked '[[:space:]]+$' -- ':!src/tests/regress/data' | tee $temp_file || true
echo -ne "${NONE}"

bad_files=$(cat $temp_file | cut -f1 -d ':' | sort -u)
rm $temp_file

if [ ! -z "$bad_files" ]; then
    if [[ $fix == true ]]; then
        for file in $bad_files; do
            sed -i '' -e's/[[:space:]]*$//' "$file"
        done

        echo
        echo -e "${GREEN}${BOLD}All trailing spaces listed above have been cleaned.${NONE}"
        exit 0
    else
        echo
        echo -e "${RED}${BOLD}Please clean all the trailing spaces listed above.${NONE}"
        echo -e "${BOLD}You can run '$self --fix' for convenience.${NONE}"
        exit 1
    fi
else
    echo -e "${GREEN}${BOLD}No trailing spaces found.${NONE}"
    exit 0
fi
Repository: risingwavelabs/risingwave
File: src/frontend/src/datafusion/iceberg_executor.rs (Rust)

Code before:
    let mut target_columns = Vec::with_capacity(batch.num_columns());
    for (column, target_field) in batch.columns().iter().zip_eq_fast(target_schema.fields()) {
        if column.data_type() == target_field.data_type() {
            target_columns.push(column.clone());
        } else {
            let casted_column = datafusion::arrow::compute::cast(column, target_field.data_type())?;
            target_columns.push(casted_column);
        }
    }

    let res = RecordBatch::try_new_with_options(
        target_schema.clone(),
        target_columns,
        &RecordBatchOptions::new().with_row_count(Some(batch.num_rows())),
    )?;
    Ok(res)
}

fn calculate_statistics<'a>(
    tasks: impl Iterator<Item = &'a FileScanTask>,
    schema: &Schema,
) -> Statistics {
    let mut total_rows: usize = 0;
    let mut total_bytes: usize = 0;
    for task in tasks {
        total_rows += task.record_count.unwrap() as usize;
        total_bytes += task.file_size_in_bytes as usize;
    }
    Statistics {
        num_rows: Precision::Exact(total_rows),
        total_byte_size: Precision::Exact(total_bytes),
        column_statistics: Statistics::unknown_column(schema),
    }
}


Review comment (security):
Currently we didn't support predicate pushdown to the file scan task, but after https://github.com/risingwavelabs/risingwave/pull/24161 it should be feasible to support, so `unwrap` here could be a potential vulnerability. We'd better to check the record count if it is absent.

Code after:
    let mut target_columns = Vec::with_capacity(batch.num_columns());
    for (column, target_field) in batch.columns().iter().zip_eq_fast(target_schema.fields()) {
        if column.data_type() == target_field.data_type() {
            target_columns.push(column.clone());
        } else {
            let casted_column = datafusion::arrow::compute::cast(column, target_field.data_type())?;
            target_columns.push(casted_column);
        }
    }

    let res = RecordBatch::try_new_with_options(
        target_schema.clone(),
        target_columns,
        &RecordBatchOptions::new().with_row_count(Some(batch.num_rows())),
    )?;
    Ok(res)
}

fn calculate_statistics<'a>(
    tasks: impl Iterator<Item = &'a FileScanTask>,
    schema: &Schema,
) -> Statistics {
    let mut total_rows: Option<usize> = Some(0);
    let mut total_bytes: usize = 0;
    for task in tasks {
        match (task.record_count, total_rows) {
            (Some(count), Some(ref mut total_rows)) => *total_rows += count as usize,
            (None, _) => total_rows = None,
            _ => {}
        };
        total_bytes += task.file_size_in_bytes as usize;
    }

    let num_rows = match total_rows {
        Some(rows) => Precision::Exact(rows),
        None => Precision::Absent,
    };
    Statistics {
        num_rows,
        total_byte_size: Precision::Exact(total_bytes),
        column_statistics: Statistics::unknown_column(schema),
    }
}