@Mytherin
This is my only change from the original implementation (from DBConfig).
There used to be a bug when we try to set values like 343897438957348957 TiB (the process actually crashed)

I can see that the bug doesn't reproduce on 1.4.2 but does reproduce for me in main and is fixed by this implementation.

Uploaded the diff between the two, to make the review easier for this method

Instead of throwing an exception in the function, could you implement this as:

bool StringUtil::TryParseFormattedBytes(const string &arg, idx_t result) {
    // ....
}

idx_t StringUtil::ParseFormattedBytes(const string &arg) {
    idx_t result;
    if (!TryParseFormattedBytes(arg, result)) {
        // throw
    }
}

This avoids having to do try/catch in other places, and the need to add the new exception class NonNumericMemoryException.

But then we end up throwing the same exception for all failure cases,
with this change, DBConfig would no longer be able to catch a specific exception type (as it does now) since all parsing errors would collapse into a single generic one. Then, the client loses the ability to distinguish between different error scenarios in the existing behaviour.

Could return a string with the error message instead? If this string is empty(), parsing has succeded without errors. If a non-empty string is returned, we can throw a ParserException with the string

@lnkuiper I understand, I'll follow this pattern then :)
Just to better understand the design preference here - is returning an error message (instead of throwing a typed exception) considered a more idiomatic approach in util functions in the codebase?
I'm asking because I'd love to understand the rationale in cases like this, especially around consistency with existing error handling patterns in DuckDB.

Yes, we prefer to return a bool/enum/string that tells us whether the function was successful, rather than try/catch, especially for "shallow" function calls like this.

We do have some try/catch, but this is when the error you need to catch is deeply nested in a function call chain.

got it, thanks :)

@lnkuiper I do notice that it makes the part when I handle a specific exception (in order convert it to a more specific exception 'memory limit) less neat.
I decided just to remove the limit from the string.

@lnkuiper I think I understand the direction, but I am still a bit confused about how we are expected to handle diffrent kinds of errors with this pattern.

I'll give a concrete example from my latest changes.

I no longer want the parse_formatted_bytes to be responsible for negative to infinite conversion anymore.
I still want the DBConfig to support negative memories and to convert it to infinite (to preserve behaviour).
But now, it means that under TryParseFormattedBytes I should return an error string:

	if (limit < 0) {
		return "Memory cannot be negative";
	}

And then in DBConfig::ParseMemoeryLimit:

	idx_t result;
	string error = StringUtil::TryParseFormattedBytes(arg, result);

	if (!error.empty()) {
		if (error == "Memory cannot be negative") {
			result = -1;
		} else {
			throw ParserException(error);
		}
	}

	return result;

This feels a bit odd to handle errors like this, since it requires branching on specific error strings to decide control flow.
My intuition would be that if we need to distinguish between different failure modes, it might be clearer to return some structured error (for example an enum or error code), but that also starts to look like reimplementing exceptions in a different form.

So I wanted to sanity check - is matching on error strings the intended pattern here, or is there a cleaner way you would expect this to be handled? Curious to hear your thoughts :)

I think we should be able to remove the try/catch here now and set NULL based on string.empty() when using TryParseFormattedBytes.

I think we should remove try_parse_formatted_bytes entirely actually. This is what try is for - you can use try(parse_formatted_bytes(...)).

@Mytherin I tried using the try on it and it didn't work. It seems like it doesn't catch exceptions during runtime, but is used only for casting failures and such. Am I missing something?

@lnkuiper I also removed the try-catch for now

The TRY operator only works for these exception types.

bool Exception::IsExecutionError(ExceptionType type) {
	switch (type) {
	case ExceptionType::INVALID_INPUT:
	case ExceptionType::OUT_OF_RANGE:
	case ExceptionType::CONVERSION:
		return true;
	default:
		return false;
	}
}

I think it makes sense to throw a InvalidInputException in your function, but keep it a ParserException for the config option.

oh cool! I didn't notice that. Thanks! :)

Nit: ParseFormattedBytesFunction

good catch, fixed :)