ronantakizawa/codereview-bench huggingface dataset:
Repository: imartinez/privateGPT
File: scripts/utils.py (Python)

Code before:
    path = "local_data"
    print(f"Wiping {path}...")
    all_files = os.listdir(path)

    files_to_remove = [file for file in all_files if file != ".gitignore"]
    for file_name in files_to_remove:
        file_path = os.path.join(path, file_name)
        try:
            if os.path.isfile(file_path):
                os.remove(file_path)
            elif os.path.isdir(file_path):
                shutil.rmtree(file_path)
        except PermissionError:
            print(
                f"PermissionError: Unable to remove {file_path}. It is in use by another process."
            )
            continue


if __name__ == "__main__":
    commands = {
        "wipe": wipe,
    }

    parser = argparse.ArgumentParser()
    parser.add_argument("mode", help="select a mode to run", choices=["wipe"])
    args = parser.parse_args()
    commands[args.mode.lower()]()


Review comment (suggestion):
Potential improvement to reduce need for code change when adding new commands
```suggestion
    parser.add_argument("mode", help="select a mode to run", choices=list(commands.keys()))
```

Code after:
    path = "local_data"
    print(f"Wiping {path}...")
    all_files = os.listdir(path)

    files_to_remove = [file for file in all_files if file != ".gitignore"]
    for file_name in files_to_remove:
        file_path = os.path.join(path, file_name)
        try:
            if os.path.isfile(file_path):
                os.remove(file_path)
            elif os.path.isdir(file_path):
                shutil.rmtree(file_path)
            print(f" - Deleted {file_path}")
        except PermissionError:
            print(
                f"PermissionError: Unable to remove {file_path}. It is in use by another process."
            )
            continue


if __name__ == "__main__":
    commands = {
        "wipe": wipe,
    }

    parser = argparse.ArgumentParser()
    parser.add_argument(
        "mode", help="select a mode to run", choices=list(commands.keys())
    )
    args = parser.parse_args()
    commands[args.mode.lower()]()
Repository: imartinez/privateGPT
File: scripts/utils.py (Python)

Code before:
import argparse
import os
import shutil


def wipe():
    path = "local_data"
    print(f"Wiping {path}...")
    all_files = os.listdir(path)

    files_to_remove = [file for file in all_files if file != ".gitignore"]
    for file_name in files_to_remove:
        file_path = os.path.join(path, file_name)
        try:
            if os.path.isfile(file_path):
                os.remove(file_path)
            elif os.path.isdir(file_path):
                shutil.rmtree(file_path)
        except PermissionError:
            print(
                f"PermissionError: Unable to remove {file_path}. It is in use by another process."
            )
            continue


if __name__ == "__main__":
    commands = {
        "wipe": wipe,
    }

    parser = argparse.ArgumentParser()
    parser.add_argument("mode", help="select a mode to run", choices=["wipe"])
    args = parser.parse_args()
    commands[args.mode.lower()]()


Review comment (security):
What do you think of making it more verbose ?

```suggestion
            elif os.path.isdir(file_path):
                shutil.rmtree(file_path)
            print(f" - Deleted {file_path}")
        except PermissionError:
```

Code after:
import argparse
import os
import shutil


def wipe():
    path = "local_data"
    print(f"Wiping {path}...")
    all_files = os.listdir(path)

    files_to_remove = [file for file in all_files if file != ".gitignore"]
    for file_name in files_to_remove:
        file_path = os.path.join(path, file_name)
        try:
            if os.path.isfile(file_path):
                os.remove(file_path)
            elif os.path.isdir(file_path):
                shutil.rmtree(file_path)
            print(f" - Deleted {file_path}")
        except PermissionError:
            print(
                f"PermissionError: Unable to remove {file_path}. It is in use by another process."
            )
            continue


if __name__ == "__main__":
    commands = {
        "wipe": wipe,
    }

    parser = argparse.ArgumentParser()
    parser.add_argument(
        "mode", help="select a mode to run", choices=list(commands.keys())
    )
    args = parser.parse_args()
    commands[args.mode.lower()]()
Repository: imartinez/privateGPT
File: private_gpt/components/node_store/node_store_component.py (Python)

Code before:
                try:
                    from llama_index.core.storage.docstore.postgres_docstore import (
                        PostgresDocumentStore,
                    )
                    from llama_index.core.storage.index_store.postgres_index_store import (
                        PostgresIndexStore,
                    )
                except ImportError:
                    raise ImportError(
                        "Postgres dependencies not found, install with `poetry install --extras storage-postgres`"
                    ) from None

                if settings.postgres is None:
                    raise ValueError("Postgres index/doc store settings not found.")

                self.index_store = PostgresIndexStore.from_params(
                    **settings.postgres.model_dump(exclude_none=True)
                )
                self.doc_store = PostgresDocumentStore.from_params(
                    **settings.postgres.model_dump(exclude_none=True)
                )

            case _:
                # Should be unreachable
                # The settings validator should have caught this
                raise ValueError(f"Database {settings.nodestore.database} not supported")


Review comment (suggestion):
This will fix the current test error:
Change
```
                raise ValueError(f"Database {settings.nodestore.database} not supported")
```
to
```
raise ValueError(
                    f"Database {settings.nodestore.database} not supported"
                )
````

Code after:
                try:
                    from llama_index.core.storage.docstore.postgres_docstore import (
                        PostgresDocumentStore,
                    )
                    from llama_index.core.storage.index_store.postgres_index_store import (
                        PostgresIndexStore,
                    )
                except ImportError:
                    raise ImportError(
                        "Postgres dependencies not found, install with `poetry install --extras storage-nodestore-postgres`"
                    ) from None

                if settings.postgres is None:
                    raise ValueError("Postgres index/doc store settings not found.")

                self.index_store = PostgresIndexStore.from_params(
                    **settings.postgres.model_dump(exclude_none=True)
                )
                self.doc_store = PostgresDocumentStore.from_params(
                    **settings.postgres.model_dump(exclude_none=True)
                )

            case _:
                # Should be unreachable
                # The settings validator should have caught this
                raise ValueError(
                    f"Database {settings.nodestore.database} not supported"
                )
Repository: influxdata/influxdb
File: cmd/influx_inspect/export/export.go (Go)

Code before:
		cmd.startTime = math.MinInt64
	}
	if end != "" {
		e, err := time.Parse(time.RFC3339, end)
		if err != nil {
			return err
		}
		cmd.endTime = e.UnixNano()
	} else {
		// set end time to max if it is not set.
		cmd.endTime = math.MaxInt64
	}

	if err := cmd.validate(); err != nil {
		return err
	}

	return cmd.export()
}

func (cmd *Command) validate() error {
	if cmd.retentionPolicy != "" && cmd.database == "" {
		return fmt.Errorf("must specify a db")
	}
	if cmd.tsmFile != "" && (cmd.database == "" || cmd.retentionPolicy == "") {
		return fmt.Errorf("must specify a db and retention policy")
	}
	if cmd.startTime != 0 && cmd.endTime != 0 && cmd.endTime < cmd.startTime {
		return fmt.Errorf("end time before start time")
	}
	return nil
}

func (cmd *Command) export() error {
	if cmd.tsmFile != "" {
		key := cmd.database + string(os.PathSeparator) + cmd.retentionPolicy
		cmd.manifest[key] = struct{}{}
		cmd.tsmFiles[key] = []string{cmd.tsmFile}
		return cmd.write()
	}

	if err := cmd.walkTSMFiles(); err != nil {
		return err
	}
	if err := cmd.walkWALFiles(); err != nil {
		return err
	}

	return cmd.write()
}


Review comment (suggestion):
Can you indicate the flags in this error? `-database` and `-retention` 

Code after:
		cmd.startTime = math.MinInt64
	}
	if end != "" {
		e, err := time.Parse(time.RFC3339, end)
		if err != nil {
			return err
		}
		cmd.endTime = e.UnixNano()
	} else {
		// set end time to max if it is not set.
		cmd.endTime = math.MaxInt64
	}

	if err := cmd.validate(); err != nil {
		return err
	}

	return cmd.export()
}

func (cmd *Command) validate() error {
	if cmd.retentionPolicy != "" && cmd.database == "" {
		return fmt.Errorf("must specify a db (-database)")
	}
	if cmd.tsmFile != "" && (cmd.database == "" || cmd.retentionPolicy == "") {
		return fmt.Errorf("must specify a db (-database) and retention policy (-retention)")
	}
	if cmd.startTime != 0 && cmd.endTime != 0 && cmd.endTime < cmd.startTime {
		return fmt.Errorf("end time before start time")
	}
	return nil
}

func (cmd *Command) export() error {
	if cmd.tsmFile != "" {
		key := cmd.database + string(os.PathSeparator) + cmd.retentionPolicy
		cmd.manifest[key] = struct{}{}
		cmd.tsmFiles[key] = []string{cmd.tsmFile}
		return cmd.write()
	}

	if err := cmd.walkTSMFiles(); err != nil {
		return err
	}
	if err := cmd.walkWALFiles(); err != nil {
		return err
	}

	return cmd.write()
}
Repository: influxdata/influxdb
File: influxdb3/src/commands/show.rs (Rust)

Code before:
use clap::{Parser, ValueEnum};
use secrecy::{ExposeSecret, Secret};
use std::error::Error;
use url::Url;

#[derive(Debug, Parser)]
pub struct Config {
    #[clap(subcommand)]
    cmd: SubCommand,
}

#[derive(Debug, Parser)]
pub enum SubCommand {
    /// View all active databases
    Databases(DatabaseConfig),
}

#[derive(Debug, Parser)]
pub struct DatabaseConfig {
    /// The host URL of the running InfluxDB 3 Core server
    #[clap(
        short = 'H',
        long = "host",
        env = "INFLUXDB3_HOST_URL",
        default_value = "http://127.0.0.1:8181"
    )]
    host_url: Url,

    /// The token for authentication with the InfluxDB 3 Core server
    #[clap(long = "token", env = "INFLUXDB3_AUTH_TOKEN")]
    auth_token: Option<Secret<String>>,

    /// The format in which to output the list of databases
    #[clap(value_enum, long = "format", default_value = "pretty")]
    output_format: Format,
}

#[derive(Debug, ValueEnum, Clone)]
#[clap(rename_all = "snake_case")]

Review comment (suggestion):
Looking further down the PR, it looks like active means all databases not marked as deleted. Should the language be more specific to say that? If so, same change for other spots where "active" is used.

Code after:
use clap::{Parser, ValueEnum};
use secrecy::{ExposeSecret, Secret};
use std::error::Error;
use url::Url;

#[derive(Debug, Parser)]
pub struct Config {
    #[clap(subcommand)]
    cmd: SubCommand,
}

#[derive(Debug, Parser)]
pub enum SubCommand {
    /// List databases
    Databases(DatabaseConfig),
}

#[derive(Debug, Parser)]
pub struct DatabaseConfig {
    /// The host URL of the running InfluxDB 3 Core server
    #[clap(
        short = 'H',
        long = "host",
        env = "INFLUXDB3_HOST_URL",
        default_value = "http://127.0.0.1:8181"
    )]
    host_url: Url,

    /// The token for authentication with the InfluxDB 3 Core server
    #[clap(long = "token", env = "INFLUXDB3_AUTH_TOKEN")]
    auth_token: Option<Secret<String>>,

    /// Include databases that were marked as deleted in the output
    #[clap(long = "show-deleted", default_value = "false")]
    show_deleted: bool,

    /// The format in which to output the list of databases
    #[clap(value_enum, long = "format", default_value = "pretty")]
    output_format: Format,
Repository: influxdata/influxdb
File: tsdb/shard.go (Go)

Code before:
			}
			validField = true
			break
		}
		if !validField {
			if reason == "" {
				reason = fmt.Sprintf(
					"invalid field name: input field \"%s\" on measurement \"%s\" is invalid",
					"time", string(p.Name()))
			}
			dropped++
			continue
		}

		// Skip any points whos keys have been dropped. Dropped has already been incremented for them.
		if len(droppedKeys) > 0 && bytesutil.Contains(droppedKeys, keys[i]) {
			continue
		}

		cont, err := func(p models.Point, iter models.FieldIterator) (cont bool, err error) {
			name := p.Name()
			mf := engine.MeasurementFields(name)
			mf.mu.Lock()
			defer mf.mu.Unlock()
			// Check with the field validator.
			if err := ValidateFields(mf, p, s.options.Config.SkipFieldSizeValidation); err != nil {
				switch err := err.(type) {
				case PartialWriteError:
					if reason == "" {
						reason = err.Reason
					}
					dropped += err.Dropped
					atomic.AddInt64(&s.stats.WritePointsDropped, int64(err.Dropped))
				default:
					return false, err
				}
				return true, nil
			}

			points[j] = points[i]
			j++

			// Create any fields that are missing.
			iter.Reset()
			for iter.Next() {
				fieldKey := iter.FieldKey()

				// Skip fields named "time". They are illegal.
				if bytes.Equal(fieldKey, timeBytes) {
					continue
				}

Review comment (suggestion):
`ValidateFields` knows which fields are not currently in the measurement fields, and this is the only place in the code it is called from. We could avoid iterating over the fields again below looking for unknown fields if `ValidateFields` collected them for us.

Code after:
			}
			validField = true
			break
		}
		if !validField {
			if reason == "" {
				reason = fmt.Sprintf(
					"invalid field name: input field \"%s\" on measurement \"%s\" is invalid",
					"time", string(p.Name()))
			}
			dropped++
			continue
		}

		// Skip any points whose keys have been dropped. Dropped has already been incremented for them.
		if len(droppedKeys) > 0 && bytesutil.Contains(droppedKeys, keys[i]) {
			continue
		}

		err := func(p models.Point, iter models.FieldIterator) error {
			var newFields []*FieldCreate
			var validateErr error
			name := p.Name()
			mf := engine.MeasurementFields(name)
			mf.mu.RLock()
			defer mf.mu.RUnlock()
			// Check with the field validator.
			if newFields, validateErr = ValidateFields(mf, p, s.options.Config.SkipFieldSizeValidation); validateErr != nil {
				var err PartialWriteError
				switch {
				case errors.As(validateErr, &err):
					// This will turn into an error later, outside this lambda
					if reason == "" {
						reason = err.Reason
					}
					dropped += err.Dropped
					atomic.AddInt64(&s.stats.WritePointsDropped, int64(err.Dropped))
				default:
					return err
				}
				return nil
			}

			points[j] = points[i]
			j++
			fieldsToCreate = append(fieldsToCreate, newFields...)
			return nil
		}(p, iter)
		if err != nil {
			return nil, nil, err
		}
Repository: influxdata/influxdb
File: tsdb/shard.go (Go)

Code before:
		for iter.Next() {
			if bytes.Equal(iter.FieldKey(), timeBytes) {
				continue
			}
			validField = true
			break
		}
		if !validField {
			if reason == "" {
				reason = fmt.Sprintf(
					"invalid field name: input field \"%s\" on measurement \"%s\" is invalid",
					"time", string(p.Name()))
			}
			dropped++
			continue
		}

		// Skip any points whos keys have been dropped. Dropped has already been incremented for them.
		if len(droppedKeys) > 0 && bytesutil.Contains(droppedKeys, keys[i]) {
			continue
		}

		cont, err := func(p models.Point, iter models.FieldIterator) (cont bool, err error) {
			name := p.Name()
			mf := engine.MeasurementFields(name)
			mf.mu.Lock()
			defer mf.mu.Unlock()
			// Check with the field validator.
			if err := ValidateFields(mf, p, s.options.Config.SkipFieldSizeValidation); err != nil {
				switch err := err.(type) {
				case PartialWriteError:
					if reason == "" {
						reason = err.Reason
					}
					dropped += err.Dropped
					atomic.AddInt64(&s.stats.WritePointsDropped, int64(err.Dropped))
				default:
					return false, err
				}
				return true, nil
			}

			points[j] = points[i]
			j++

			// Create any fields that are missing.
			iter.Reset()
			for iter.Next() {
				fieldKey := iter.FieldKey()

				// Skip fields named "time". They are illegal.

Review comment (bug):
It feels like there is still a potential race condition. We lock the `mf` while looking up fields here, but then unlock it while we continue to the next point. Another incoming write in a different goroutine could then look up fields in `mf` before this goroutine can create the new fields. Or am I missing something?

Code after:
		for iter.Next() {
			if bytes.Equal(iter.FieldKey(), timeBytes) {
				continue
			}
			validField = true
			break
		}
		if !validField {
			if reason == "" {
				reason = fmt.Sprintf(
					"invalid field name: input field \"%s\" on measurement \"%s\" is invalid",
					"time", string(p.Name()))
			}
			dropped++
			continue
		}

		// Skip any points whose keys have been dropped. Dropped has already been incremented for them.
		if len(droppedKeys) > 0 && bytesutil.Contains(droppedKeys, keys[i]) {
			continue
		}

		err := func(p models.Point, iter models.FieldIterator) error {
			var newFields []*FieldCreate
			var validateErr error
			name := p.Name()
			mf := engine.MeasurementFields(name)
			mf.mu.RLock()
			defer mf.mu.RUnlock()
			// Check with the field validator.
			if newFields, validateErr = ValidateFields(mf, p, s.options.Config.SkipFieldSizeValidation); validateErr != nil {
				var err PartialWriteError
				switch {
				case errors.As(validateErr, &err):
					// This will turn into an error later, outside this lambda
					if reason == "" {
						reason = err.Reason
					}
					dropped += err.Dropped
					atomic.AddInt64(&s.stats.WritePointsDropped, int64(err.Dropped))
				default:
					return err
				}
				return nil
			}

			points[j] = points[i]
			j++
			fieldsToCreate = append(fieldsToCreate, newFields...)
			return nil
		}(p, iter)
Repository: influxdata/influxdb
File: tsdb/shard.go (Go)

Code before:
					reason = err.Reason
				}
				dropped += err.Dropped
				atomic.AddInt64(&s.stats.WritePointsDropped, int64(err.Dropped))
			default:
				return nil, nil, err
			}
			continue
		}

		points[j] = points[i]
		j++

		// Create any fields that are missing.
		iter.Reset()
		for iter.Next() {
			fieldKey := iter.FieldKey()

			// Skip fields named "time". They are illegal.
			if bytes.Equal(fieldKey, timeBytes) {
				continue
			}

			if mf.FieldBytes(fieldKey) != nil {
				continue
			}

			dataType := dataTypeFromModelsFieldType(iter.Type())
			if dataType == influxql.Unknown {
				continue
			}

			fieldsToCreate = append(fieldsToCreate, &FieldCreate{
				Measurement: name,
				Field: &Field{
					Name: string(fieldKey),
					Type: dataType,
				},
			})
		}
	}

	if dropped > 0 {
		err = PartialWriteError{Reason: reason, Dropped: dropped, Database: s.database, RetentionPolicy: s.retentionPolicy}
	}

	return points[:j], fieldsToCreate, err
}

const unPrintReplRune = '?'
const unPrintMaxReplRune = 3

Review comment (performance):
And the second part is here, where the **thread one** has now created the field, of (for example) data type `float`, and **thread two** reaches this point and continues as the field key now exists, but **thread two** wants to create a field of type **integer**. 

I believe the other part of the race is that a snapshot has started for the data from **thread one**, so a new cache accepts the writes for **thread two** of the new data type, as the cache will accept the value, as it does not have any previous data to validate.

Code after:
			if newFields, validateErr = ValidateFields(mf, p, s.options.Config.SkipFieldSizeValidation); validateErr != nil {
				var err PartialWriteError
				switch {
				case errors.As(validateErr, &err):
					// This will turn into an error later, outside this lambda
					if reason == "" {
						reason = err.Reason
					}
					dropped += err.Dropped
					atomic.AddInt64(&s.stats.WritePointsDropped, int64(err.Dropped))
				default:
					return err
				}
				return nil
			}

			points[j] = points[i]
			j++
			fieldsToCreate = append(fieldsToCreate, newFields...)
			return nil
		}(p, iter)
		if err != nil {
			return nil, nil, err
		}
	}
	if dropped > 0 {
		err = PartialWriteError{Reason: reason, Dropped: dropped, Database: s.database, RetentionPolicy: s.retentionPolicy}
	}

	return points[:j], fieldsToCreate, err
}

const unPrintReplRune = '?'
const unPrintMaxReplRune = 3

// makePrintable - replace invalid and non-printable unicode characters with a few '?' runes
func makePrintable(s string) string {
	b := strings.Builder{}
	b.Grow(len(s))
	c := 0
	for _, r := range strings.ToValidUTF8(s, string(unicode.ReplacementChar)) {
		if !unicode.IsPrint(r) || r == unicode.ReplacementChar {
			if c < unPrintMaxReplRune {
				b.WriteRune(unPrintReplRune)
			}
			c++
		} else {
			b.WriteRune(r)
			c = 0
		}
	}
Repository: influxdata/influxdb
File: influxdb3/src/commands/helpers.rs (Rust)

Code before:
// common fruitilities for cli

use std::str::FromStr;

use influxdb3_server::all_paths;
use observability_deps::tracing::trace;

const DISABLED_AUTHZ_TOO_MANY_VALUES_ERR: &str = "--disable-authz cannot take more than 3 items";
const DISABLED_AUTHZ_INVALID_VALUE_ERR: &str =
    "invalid value passed in for --disable-authz, allowed values are health,ping and metrics";

// This custom type is used to parse `--disable-authz health,ping,metrics`, it wasn't straight
// forward to setup num_args and also collect the values into a list with a value_delimiter set.
// Even if it's possible with a bit of clap-fu, still requires mapping those resource names to
// endpoints. This custom type pulls both parsing and mapping logic together, easier to setup
// and test
#[derive(Debug, Clone, Default)]
pub struct DisableAuthzList {
    resources: Vec<String>,
}

impl DisableAuthzList {
    pub(crate) fn get_mapped_endpoints(self) -> Vec<String> {
        trace!(resources = ?&self.resources, "resources to be setup without authz");
        let all_paths_without_authz = self
            .resources
            .iter()
            .flat_map(|path| {
                if path == "health" {
                    return vec![
                        all_paths::API_V3_HEALTH.to_string(),
                        all_paths::API_V1_HEALTH.to_string(),
                    ];
                }


Review comment (security):
```suggestion
    "invalid value passed in for --disable-authz, allowed values are health, ping, and metrics";
```

Code after:
// common fruitilities for cli

use std::{str::FromStr, sync::OnceLock};

use influxdb3_server::all_paths;
use observability_deps::tracing::trace;

const DISABLED_AUTHZ_TOO_MANY_VALUES_ERR: &str = "--disable-authz cannot take more than 3 items";
const DISABLED_AUTHZ_INVALID_VALUE_ERR: &str =
    "invalid value passed in for --disable-authz, allowed values are health, ping, and metrics";

static AUTHZ_DISABLED_RESOURCES: OnceLock<Vec<&'static str>> = OnceLock::new();

// This custom type is used to parse `--disable-authz health,ping,metrics`, it wasn't straight
// forward to setup num_args and also collect the values into a list with a value_delimiter set.
// Even if it's possible with a bit of clap-fu, still requires mapping those resource names to
// endpoints. This custom type pulls both parsing and mapping logic together, easier to setup
// and test
#[derive(Debug, Clone)]
pub struct DisableAuthzList;

impl Default for DisableAuthzList {
    fn default() -> Self {
        AUTHZ_DISABLED_RESOURCES.get_or_init(Vec::new);
        Self {}
    }
}

impl DisableAuthzList {
    pub(crate) fn get_mapped_endpoints(&self) -> &'static Vec<&'static str> {
        let all_paths_without_authz = AUTHZ_DISABLED_RESOURCES
            .get()
            .expect("disabled resource paths to have been loaded");
        trace!(paths_without_authz = ?all_paths_without_authz, "paths setup without authz");
        all_paths_without_authz