ronantakizawa/codereview-bench huggingface dataset:
Repository: remix-run/react-router
File: packages/react-router/lib/dom/ssr/routeModules.ts (TypeScript)

Code before:
  meta: MetaDescriptor[];
  error?: unknown;
}

export type MetaMatches<
  MatchLoaders extends Record<
    string,
    LoaderFunction | ClientLoaderFunction | unknown
  > = Record<string, unknown>,
> = Array<
  {
    [K in keyof MatchLoaders]: MetaMatch<
      Exclude<K, number | symbol>,
      MatchLoaders[K]
    >;
  }[keyof MatchLoaders]
>;

export interface MetaArgs<
  Loader extends LoaderFunction | ClientLoaderFunction | unknown = unknown,
  MatchLoaders extends Record<
    string,
    LoaderFunction | ClientLoaderFunction | unknown
  > = Record<string, unknown>,
> {
  /** @deprecated Use `loaderData` instead */
  data:
    | (Loader extends LoaderFunction | ClientLoaderFunction
        ? SerializeFrom<Loader>
        : unknown)
    | undefined;
  loaderData:
    | (Loader extends LoaderFunction | ClientLoaderFunction
    ? SerializeFrom<Loader>
    : unknown)
    | undefined;
  params: Params;
  location: Location;
  matches: MetaMatches<MatchLoaders>;
  error?: unknown;
}

/**
 * A function that returns an array of data objects to use for rendering
 * metadata HTML tags in a route. These tags are not rendered on descendant
 * routes in the route hierarchy. In other words, they will only be rendered on
 * the route in which they are exported.
 *
 * @param Loader - The type of the current route's loader function
 * @param MatchLoaders - Mapping from a parent route's filepath to its loader
 * function type

Review comment (suggestion):
```suggestion
  /** @deprecated Use `MetaArgs.loaderData` instead */
```

Code after:
  meta: MetaDescriptor[];
  error?: unknown;
}

export type MetaMatches<
  MatchLoaders extends Record<
    string,
    LoaderFunction | ClientLoaderFunction | unknown
  > = Record<string, unknown>,
> = Array<
  {
    [K in keyof MatchLoaders]: MetaMatch<
      Exclude<K, number | symbol>,
      MatchLoaders[K]
    >;
  }[keyof MatchLoaders]
>;

export interface MetaArgs<
  Loader extends LoaderFunction | ClientLoaderFunction | unknown = unknown,
  MatchLoaders extends Record<
    string,
    LoaderFunction | ClientLoaderFunction | unknown
  > = Record<string, unknown>,
> {
  /** @deprecated Use `MetaArgs.loaderData` instead */
  data:
    | (Loader extends LoaderFunction | ClientLoaderFunction
        ? SerializeFrom<Loader>
        : unknown)
    | undefined;
  loaderData:
    | (Loader extends LoaderFunction | ClientLoaderFunction
    ? SerializeFrom<Loader>
    : unknown)
    | undefined;
  params: Params;
  location: Location;
  matches: MetaMatches<MatchLoaders>;
  error?: unknown;
}

/**
 * A function that returns an array of data objects to use for rendering
 * metadata HTML tags in a route. These tags are not rendered on descendant
 * routes in the route hierarchy. In other words, they will only be rendered on
 * the route in which they are exported.
 *
 * @param Loader - The type of the current route's loader function
 * @param MatchLoaders - Mapping from a parent route's filepath to its loader
 * function type
Repository: remix-run/react-router
File: playground/rsc-vite-framework/start-vite-middleware.js (JavaScript)

Code before:
import { createRequestListener } from "@mjackson/node-fetch-server";
import express from "express";

const viteDevServer =
  process.env.NODE_ENV === "production"
    ? undefined
    : await import("vite").then(({ createServer }) =>
        createServer({
          server: {
            middlewareMode: true,
          },
        }),
      );

const requestListener = viteDevServer
  ? async (req, res) => {
      // In dev mode, ensure we load a fresh request handler every request
      const rscEntry = await viteDevServer.environments.rsc.runner.import(
        "virtual:react-router/unstable_rsc/rsc-entry",
      );
      return createRequestListener(rscEntry.default)(req, res);
    }
  : // In production, get the static request handler from the build output
    createRequestListener((await import("./build/server/index.js")).default);

const app = express();

if (viteDevServer) {
  app.use(viteDevServer.middlewares);
} else {
  app.use(
    "/assets",
    express.static("build/client/assets", { immutable: true, maxAge: "1y" }),
  );
  app.use(express.static("build/client"));
}

app.all("*", requestListener);

const port = process.env.PORT || 3000;
app.listen(port);
console.log(`Server listening on port ${port} (http://localhost:${port})`);


Review comment (suggestion):
During dev, currently this does nothing since rsc plugin already includes the same handler in `viteDevServer.middlewares`. I noticed that it isn't the case in non-rsc framework mode. Should rsc plugin align with it?

https://github.com/remix-run/react-router/blob/e495e0eef712794a422882eaa4d70ed2a1a22844/packages/react-router-dev/vite/plugin.ts#L1623-L1626

(Btw, I noticed there's nothing specific about react router for this custom server setup and I'm thinking to add a same example on rsc plugin https://github.com/vitejs/vite-plugin-react/pull/817)

Code after:
import { createRequestListener } from "@mjackson/node-fetch-server";
import express from "express";

const viteDevServer =
  process.env.NODE_ENV === "production"
    ? undefined
    : await import("vite").then(({ createServer }) =>
        createServer({
          server: {
            middlewareMode: true,
          },
        }),
      );

const app = express();

if (viteDevServer) {
  app.use(viteDevServer.middlewares);
} else {
  app.use(
    "/assets",
    express.static("build/client/assets", { immutable: true, maxAge: "1y" }),
  );
  app.use(express.static("build/client"));
  app.all(
    "*",
    createRequestListener((await import("./build/server/index.js")).default),
  );
}

const port = process.env.PORT || 3000;
app.listen(port);
console.log(`Server listening on port ${port} (http://localhost:${port})`);
Repository: risingwavelabs/risingwave
File: src/expr/impl/src/scalar/random.rs (Rust)

Code before:
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
//     http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

use rand::Rng;
use risingwave_common::types::F64;
use risingwave_expr::function;

/// Generates a random float between 0 and 1.
///
/// ```slt
/// query I
/// SELECT random();
/// ----
/// 0.123456789
/// ```
#[function("random() -> float8", volatile)]
fn random() -> F64 {
    rand::rng().random::<F64>()
}


Review comment (suggestion):
Use random_range instead?

https://docs.rs/rand/latest/rand/fn.random_range.html

I don't think `random` will have its range constrainted to [0, 1].

Code after:
// Copyright 2025 RisingWave Labs
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
//     http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

use rand::random_range;
use risingwave_common::types::F64;
use risingwave_expr::function;

/// Generates a random float between 0 and 1 inclusive.
#[function("random() -> float8", volatile)]
fn random() -> F64 {
    let val: f64 = random_range(0.0..=1.0);
    F64::from(val)
}
Repository: risingwavelabs/risingwave
File: src/expr/impl/src/scalar/arithmetic_op.rs (Rust)

Code before:
/// query R
/// SELECT gamma('1'::float8);
/// ----
/// 1
///
/// query R
/// SELECT gamma('2'::float8);
/// ----
/// 1
///
/// query R
/// SELECT gamma('3'::float8);
/// ----
/// 2
///
/// query R
/// SELECT gamma('4'::float8);
/// ----
/// 6
///
/// query R
/// SELECT gamma('5'::float8);
/// ----
/// 24
///
/// statement error
/// SELECT gamma('-1'::float8);
///
/// statement error
/// SELECT gamma('-1000.5'::float8);
///
/// statement error
/// SELECT gamma('0'::float8);
///
/// statement error
/// SELECT gamma('1000'::float8);
/// ```
#[function("gamma(float8) -> float8")]
pub fn gamma_f64(input: F64) -> Result<F64> {
    let mut result = input;
    if input.is_nan() {
        return Ok(result);
    } else if input.is_infinite() {
        if input.is_negative() {
            return Err(ExprError::InvalidParam {
                name: "gamma",
                reason: "value out of range: overflow".into(),
            });
        }
    } else {
        result = F64::from(Gamma::gamma(input.0));

Review comment (bug):
We can include part of the error message to test if it matches.

```suggestion
/// statement error underflow
```

Code after:
#[function("lgamma(float8) -> float8")]
pub fn lgamma_f64(input: F64) -> Result<F64> {
    let (result, _sign) = input.ln_gamma();
    if result.is_infinite() && input.is_finite() {
        return Err(ExprError::NumericOverflow);
    }
    Ok(F64::from(result))
}

#[cfg(test)]
mod tests {
    use std::str::FromStr;

    use num_traits::Float;
    use risingwave_common::types::test_utils::IntervalTestExt;
    use risingwave_common::types::{F32, Int256, Int256Ref, Scalar};

    use super::*;

    #[test]
    fn test() {
        assert_eq!(
            general_add::<_, _, Decimal>(Decimal::from_str("1").unwrap(), 1i32).unwrap(),
            Decimal::from_str("2").unwrap()
        );
    }

    #[test]
    fn test_arithmetic() {
        assert_eq!(
            general_add::<Decimal, i32, Decimal>(dec("1.0"), 1).unwrap(),
            dec("2.0")
        );
        assert_eq!(
            general_sub::<Decimal, i32, Decimal>(dec("1.0"), 2).unwrap(),
            dec("-1.0")
        );
        assert_eq!(
            general_mul::<Decimal, i32, Decimal>(dec("1.0"), 2).unwrap(),
            dec("2.0")
        );
        assert_eq!(
            general_div::<Decimal, i32, Decimal>(dec("2.0"), 2).unwrap(),
            dec("1.0")
        );
        assert_eq!(
            general_mod::<Decimal, i32, Decimal>(dec("2.0"), 2).unwrap(),
            dec("0")
        );
        assert_eq!(general_neg::<Decimal>(dec("1.0")).unwrap(), dec("-1.0"));
        assert_eq!(general_add::<i16, i32, i32>(1i16, 1i32).unwrap(), 2i32);
Repository: risingwavelabs/risingwave
File: src/connector/src/source/pulsar/mod.rs (Rust)

Code before:

    const SOURCE_NAME: &'static str = PULSAR_CONNECTOR;
}

impl crate::source::UnknownFields for PulsarProperties {
    fn unknown_fields(&self) -> HashMap<String, String> {
        self.unknown_fields.clone()
    }
}

impl EnforceSecret for PulsarProperties {
    fn enforce_secret<'a>(prop_iter: impl Iterator<Item = &'a str>) -> Result<(), ConnectorError> {
        for prop in prop_iter {
            PulsarCommon::enforce_one(prop)?;
        }
        Ok(())
    }
}

impl EnforceSecret for PulsarConsumerOptions {}

#[derive(Clone, Debug, Deserialize, WithOptions)]
#[serde_as]
pub struct PulsarConsumerOptions {
    #[serde(rename = "pulsar.read_compacted")]
    #[serde_as(as = "Option<DisplayFromStr>")]
    pub read_compacted: Option<bool>,
}

#[derive(Clone, Debug, Deserialize, WithOptions)]
#[serde_as]
pub struct PulsarProperties {
    #[serde(rename = "scan.startup.mode", alias = "pulsar.scan.startup.mode")]
    pub scan_startup_mode: Option<String>,

    #[serde(
        rename = "scan.startup.timestamp.millis",
        alias = "pulsar.time.offset",
        alias = "scan.startup.timestamp_millis"
    )]
    pub time_offset: Option<String>,

    #[serde(flatten)]
    pub common: PulsarCommon,

    #[serde(flatten)]
    pub oauth: Option<PulsarOauthCommon>,

    #[serde(flatten)]
    pub aws_auth_props: AwsAuthProps,


Review comment (suggestion):
The `read_compacted` field uses `DisplayFromStr` for deserialization, but booleans should deserialize naturally without string conversion. Remove the `#[serde_as(as = \"Option<DisplayFromStr>\")]` attribute to allow direct boolean deserialization.
```suggestion

```

Code after:
    type SplitEnumerator = PulsarSplitEnumerator;
    type SplitReader = PulsarSplitReader;

    const SOURCE_NAME: &'static str = PULSAR_CONNECTOR;
}

impl crate::source::UnknownFields for PulsarProperties {
    fn unknown_fields(&self) -> HashMap<String, String> {
        self.unknown_fields.clone()
    }
}

impl EnforceSecret for PulsarProperties {
    fn enforce_secret<'a>(prop_iter: impl Iterator<Item = &'a str>) -> Result<(), ConnectorError> {
        for prop in prop_iter {
            PulsarCommon::enforce_one(prop)?;
        }
        Ok(())
    }
}

impl EnforceSecret for PulsarConsumerOptions {}

#[derive(Clone, Debug, Deserialize, WithOptions)]
#[serde_as]
pub struct PulsarConsumerOptions {
    #[serde(
        rename = "pulsar.read_compacted",
        default,
        deserialize_with = "deserialize_optional_bool_from_string"
    )]
    pub read_compacted: Option<bool>,
}

#[derive(Clone, Debug, Deserialize, WithOptions)]
#[serde_as]
pub struct PulsarProperties {
    #[serde(rename = "scan.startup.mode", alias = "pulsar.scan.startup.mode")]
    pub scan_startup_mode: Option<String>,

    #[serde(
        rename = "scan.startup.timestamp.millis",
        alias = "pulsar.time.offset",
        alias = "scan.startup.timestamp_millis"
    )]
    pub time_offset: Option<String>,

    #[serde(flatten)]
    pub common: PulsarCommon,

    #[serde(flatten)]
Repository: risingwavelabs/risingwave
File: src/common/src/gap_fill.rs (Rust)

Code before:
            Some(ScalarImpl::Float32((v2 - v1) / steps as f32))
        }
        (ScalarRefImpl::Float64(v1), ScalarRefImpl::Float64(v2)) => {
            Some(ScalarImpl::Float64((v2 - v1) / steps as f64))
        }
        (ScalarRefImpl::Decimal(v1), ScalarRefImpl::Decimal(v2)) => {
            Some(ScalarImpl::Decimal((v2 - v1) / Decimal::from(steps)))
        }
        _ => None,
    }
}

/// Mutates the `current` datum by adding the value of `step` to it.
///
/// This function is used during the interpolation process in gap filling,
/// where it incrementally updates the datum to generate intermediate values
/// between known data points.
pub fn apply_interpolation_step(current: &mut Datum, step: &ScalarImpl) {
    if let Some(curr) = current.as_mut() {
        match (curr, step) {
            (ScalarImpl::Int16(v1), ScalarImpl::Int16(v2)) => *v1 += v2,
            (ScalarImpl::Int32(v1), ScalarImpl::Int32(v2)) => *v1 += v2,
            (ScalarImpl::Int64(v1), ScalarImpl::Int64(v2)) => *v1 += v2,
            (ScalarImpl::Float32(v1), ScalarImpl::Float32(v2)) => *v1 += v2,
            (ScalarImpl::Float64(v1), ScalarImpl::Float64(v2)) => *v1 += v2,
            (ScalarImpl::Decimal(v1), ScalarImpl::Decimal(v2)) => *v1 = *v1 + *v2,
            _ => (),
        }
    }
}


Review comment (bug):
Incorrect dereference of `v2` in pattern matching. When matching `&ScalarImpl` with pattern `ScalarImpl::Decimal(v2)`, the value `v2` is already extracted as `Decimal` (which implements `Copy`), not as `&Decimal`. The expression should be `*v1 = *v1 + v2` without dereferencing `v2`.
```suggestion
            (ScalarImpl::Decimal(v1), ScalarImpl::Decimal(v2)) => *v1 = *v1 + v2,
```

Code after:
            Some(ScalarImpl::Float32((v2 - v1) / steps as f32))
        }
        (ScalarRefImpl::Float64(v1), ScalarRefImpl::Float64(v2)) => {
            Some(ScalarImpl::Float64((v2 - v1) / steps as f64))
        }
        (ScalarRefImpl::Decimal(v1), ScalarRefImpl::Decimal(v2)) => {
            Some(ScalarImpl::Decimal((v2 - v1) / Decimal::from(steps)))
        }
        _ => None,
    }
}

/// Mutates the `current` datum by adding the value of `step` to it.
///
/// This function is used during the interpolation process in gap filling,
/// where it incrementally updates the datum to generate intermediate values
/// between known data points.
pub fn apply_interpolation_step(current: &mut Datum, step: &ScalarImpl) {
    if let Some(curr) = current.as_mut() {
        match (curr, step) {
            (ScalarImpl::Int16(v1), &ScalarImpl::Int16(v2)) => *v1 += v2,
            (ScalarImpl::Int32(v1), &ScalarImpl::Int32(v2)) => *v1 += v2,
            (ScalarImpl::Int64(v1), &ScalarImpl::Int64(v2)) => *v1 += v2,
            (ScalarImpl::Float32(v1), &ScalarImpl::Float32(v2)) => *v1 += v2,
            (ScalarImpl::Float64(v1), &ScalarImpl::Float64(v2)) => *v1 += v2,
            (ScalarImpl::Decimal(v1), &ScalarImpl::Decimal(v2)) => *v1 = *v1 + v2,
            _ => (),
        }
    }
}
Repository: risingwavelabs/risingwave
File: src/frontend/src/handler/create_source/validate.rs (Rust)

Code before:
        }
        if !props.contains_key("publication.name") {
            // Default publication name is "rw_publication"
            props.insert("publication.name".into(), "rw_publication".into());
        }
        if !props.contains_key("publication.create.enable") {
            // Default auto create publication if doesn't exist
            props.insert("publication.create.enable".into(), "true".into());
        }
    }

    if connector == SQL_SERVER_CDC_CONNECTOR && !props.contains_key("schema.name") {
        // Default schema name is "dbo"
        props.insert("schema.name".into(), "dbo".into());
    }

    // Validate cdc.source.wait.streaming.start.timeout for all CDC connectors
    if (connector == MYSQL_CDC_CONNECTOR
        || connector == POSTGRES_CDC_CONNECTOR
        || connector == CITUS_CDC_CONNECTOR
        || connector == MONGODB_CDC_CONNECTOR
        || connector == SQL_SERVER_CDC_CONNECTOR)
        && let Some(timeout_value) = props.get("cdc.source.wait.streaming.start.timeout")
        && timeout_value.parse::<u32>().is_err()
    {
        return Err(RwError::from(ProtocolError(format!(
            "Invalid 'cdc.source.wait.streaming.start.timeout' value: '{}'. Expected a positive integer, not a string",
            timeout_value
        ))));
    }

    Ok(())
}


Review comment (suggestion):
nits: should be `InvalidConfigValue` or `InvalidParameterValue` than `ProtocolError`

Code after:
        }
        if !props.contains_key("publication.name") {
            // Default publication name is "rw_publication"
            props.insert("publication.name".into(), "rw_publication".into());
        }
        if !props.contains_key("publication.create.enable") {
            // Default auto create publication if doesn't exist
            props.insert("publication.create.enable".into(), "true".into());
        }
    }

    if connector == SQL_SERVER_CDC_CONNECTOR && !props.contains_key("schema.name") {
        // Default schema name is "dbo"
        props.insert("schema.name".into(), "dbo".into());
    }

    // Validate cdc.source.wait.streaming.start.timeout for all CDC connectors
    if (connector == MYSQL_CDC_CONNECTOR
        || connector == POSTGRES_CDC_CONNECTOR
        || connector == CITUS_CDC_CONNECTOR
        || connector == MONGODB_CDC_CONNECTOR
        || connector == SQL_SERVER_CDC_CONNECTOR)
        && let Some(timeout_value) = props.get("cdc.source.wait.streaming.start.timeout")
        && timeout_value.parse::<u32>().is_err()
    {
        return Err(ErrorCode::InvalidConfigValue {
            config_entry: "cdc.source.wait.streaming.start.timeout".to_owned(),
            config_value: timeout_value.to_owned(),
        }
        .into());
    }

    Ok(())
}
Repository: risingwavelabs/risingwave
File: src/meta/src/rpc/ddl_controller.rs (Rust)

Code before:
            NonZeroUsize::new(6).unwrap(),
            Some(NonZeroUsize::new(10).unwrap()),
            &DefaultParallelism::Full,
            "default",
        )
        .unwrap();
        assert_eq!(result.get(), 6);
    }

    #[test]
    fn zero_available_returns_unavailable_error() {
        let result = DdlController::resolve_stream_parallelism_inner(
            None,
            NonZeroUsize::new(4).unwrap(),
            None,
            &DefaultParallelism::Full,
            "default",
        );
        assert!(matches!(
            result,
            Err(ref e) if matches!(e.inner(), MetaErrorInner::Unavailable(_))
        ));
    }

    #[test]
    fn specified_over_max_is_rejected() {
        let result = DdlController::resolve_stream_parallelism_inner(
            Some(NonZeroUsize::new(8).unwrap()),
            NonZeroUsize::new(4).unwrap(),
            Some(NonZeroUsize::new(10).unwrap()),
            &DefaultParallelism::Full,
            "default",
        );
        assert!(matches!(
            result,
            Err(ref e) if matches!(e.inner(), MetaErrorInner::InvalidParameter(_))
        ));
    }
}


Review comment (suggestion):
[nitpick] Test name `specified_over_max_is_rejected` could be clearer. Consider renaming to `test_specified_over_max_returns_error` to more precisely describe the behavior and follow common test naming conventions (starting with `test_`).
```suggestion
    fn test_specified_over_max_returns_error() {
```

Code after:
            NonZeroUsize::new(6).unwrap(),
            Some(NonZeroUsize::new(10).unwrap()),
            &DefaultParallelism::Full,
            "default",
        )
        .unwrap();
        assert_eq!(result.get(), 6);
    }

    #[test]
    fn test_no_available_slots_returns_error() {
        let result = DdlController::resolve_stream_parallelism_inner(
            None,
            NonZeroUsize::new(4).unwrap(),
            None,
            &DefaultParallelism::Full,
            "default",
        );
        assert!(matches!(
            result,
            Err(ref e) if matches!(e.inner(), MetaErrorInner::Unavailable(_))
        ));
    }

    #[test]
    fn test_specified_over_max_returns_error() {
        let result = DdlController::resolve_stream_parallelism_inner(
            Some(NonZeroUsize::new(8).unwrap()),
            NonZeroUsize::new(4).unwrap(),
            Some(NonZeroUsize::new(10).unwrap()),
            &DefaultParallelism::Full,
            "default",
        );
        assert!(matches!(
            result,
            Err(ref e) if matches!(e.inner(), MetaErrorInner::InvalidParameter(_))
        ));
    }
}