Fuzzer #1389: ARG_XXX Decimal Casts #10742
Conversation
Instead of trying to track the combinatorial explosion of ordering types for DECIMAL arguments, just return a function that will force the binder to inject casts as if there were no custom bind function.
|
I was trying this out, bumped against this problem (that is independent, so can be addressed separately): Connected to this, are casts performed by this PR always information-preserving? |
|
Yes casting to double is lossy for numerics - it would be better to add the |
The cast from HUGEINT to DOUBLE is lossy and can generate incorrect results. fixes: duckdblabs-duckdb-internal#1294
|
Combined both into this PR since it was small and @carlopi had such a nice test! |
carlopi
left a comment
There was a problem hiding this comment.
I think in a nicer world GetArgMinMaxFunctionBy should never throw and have some other way to communicate it can't handle a type (given previously we were building functions that would only end up throwing), and after this PR also potentially (say with some more changes in).
Also, I am curious how we can guarantee that a Cast preserves information, I am not sure if we have that information (or maybe it's implicitly guaranteed?).
BUT, both problems were there before, and this PR is a clear improvement, so I think it might be just fine to merge this (after adding back at least INT16).
| case PhysicalType::INT8: | ||
| return GetArgMinMaxFunctionInternal<OP, ARG_TYPE, int8_t>(by_type, type); | ||
| case PhysicalType::INT16: | ||
| return GetArgMinMaxFunctionInternal<OP, ARG_TYPE, int16_t>(by_type, type); | ||
| case PhysicalType::INT32: | ||
| return GetArgMinMaxFunctionInternal<OP, ARG_TYPE, int32_t>(by_type, type); | ||
| case PhysicalType::INT64: | ||
| return GetArgMinMaxFunctionInternal<OP, ARG_TYPE, int64_t>(by_type, type); | ||
| case PhysicalType::FLOAT: | ||
| return GetArgMinMaxFunctionInternal<OP, ARG_TYPE, float>(by_type, type); |
There was a problem hiding this comment.
I would avoid removing INT16 (in particular) since it's used by GetDecimalArgMinMaxFunction. Also INT8 and FLOAT probably are fine staying.
There was a problem hiding this comment.
I removed them to reduce the code footprint back to what it was before I made the fix.
There was a problem hiding this comment.
The impact on runtime memory is minimal because we only store one value. There is some casting overhead but that is the same as for non-DECIMAL types.
There was a problem hiding this comment.
Right, forgot it was in the connected PR. All cool then.
|
Thanks! LGTM |
Merge pull request duckdb/duckdb#10742 from hawkfish/fuzzer-decimal-order Merge pull request duckdb/duckdb#10832 from krlmlr/f-setup-python-v5
Instead of trying to track the combinatorial explosion of ordering types for DECIMAL arguments, just return a function that will force the binder to inject casts as if there were no custom bind function.