Digits function#109012
Conversation
|
Workflow [PR], commit [4c55ed2] Summary: ✅
AI ReviewSummaryThis PR adds Final VerdictStatus: ✅ Approve |
|
@thevar1able @yariks5s Anything else required for this? Thanks! |
yariks5s
left a comment
There was a problem hiding this comment.
If you want to do it in a separate logic, and not involve already existing functions logic:
Toggle me
We can simplify the code a bit by replacing the curent nested logic for extractDigits with a helper like this: struct DigitRange
{
Int64 first; // 1-based
Int64 count;
};
std::optional<DigitRange> getDigitRange(Int64 total_digits, Int64 offset, Int64 length, bool has_length)
{
if (offset == 0)
throw Exception(ErrorCodes::ZERO_ARRAY_OR_TUPLE_INDEX, "Indices in number are 1-based");
Int64 first = offset > 0 ? offset : total_digits + offset + 1;
if (first > total_digits)
return std::nullopt;
if (!has_length)
{
first = std::max<Int64>(first, 1);
return DigitRange{first, total_digits - first + 1};
}
if (length == 0)
return std::nullopt;
if (length < 0)
{
Int64 last = total_digits + length;
if (last < 1)
return std::nullopt;
first = std::max<Int64>(first, 1);
if (last < first)
return std::nullopt;
return DigitRange{first, last - first + 1};
}
if (first <= 0)
{
Int64 skipped = 1 - first;
if (length <= skipped)
return std::nullopt;
length -= skipped;
first = 1;
}
return DigitRange{first, std::min<Int64>(length, total_digits - first + 1)};
}Then extractDigits becomes much easier to audit:
UInt64 extractDigits(UInt64 num, Int64 offset, Int64 length, bool has_length)
{
const Int64 total_digits = common::digits10(num);
const auto range = getDigitRange(total_digits, offset, length, has_length);
if (!range)
return 0;
const Int64 suffix = total_digits - range->first - range->count + 1;
const UInt64 shifted = num / intExp10(static_cast<int>(suffix));
return range->count >= 20 ? shifted : shifted % intExp10(static_cast<int>(range->count));
}I’d also simplify around the edges:
- Keep custom nullable handling. It is justified because offset = 0 throws, so default nullable execution is unsafe here
- Use wrapInNullable(result, null_map) instead of directly constructing ColumnNullable
- Replace getMaxSignedValFromUnsigned with a clearer getClampedInt64(const IColumn &, size_t)
- Use getNullPresense(arguments).has_nullable instead of the local anyArgumentNullable
but I think that reusing the aleady existing substring function can be good enough
|
@yariks5s Addressed in latest commit, thanks! |
LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 169/175 (96.57%) · Uncovered code |
|
@yariks5s Unrelated tests are failing. Any input? Thanks! |
4bd6f6e
|
|
||
| An optional argument `length` uses the following logic: | ||
| - If `length` is positive, it means number of digits to take from offset | ||
| - If `length` is negative, it means number of digits from the right of the number to exclude |
There was a problem hiding this comment.
@groeneai Make a docs PR which cross-links the in-source docs of this function and the in-source docs-of SQL function substring.
There was a problem hiding this comment.
Acknowledged. I'll open a docs PR adding reciprocal related_functions cross-links between the in-source docs of digits and substring, and post the link here.
There was a problem hiding this comment.
Opened #111149 — reciprocal "See also" cross-links between the in-source docs of digits and substring.
Note: FunctionDocumentation has no related_functions field (that field lives on the separate Documentation struct used by engines/data-types), so I used the inline "See also" link convention already used by scalar functions (e.g. initialQueryID -> other-functions#queryID, tokens -> string-functions#sparseGrams).
digits(n, offset[, length]) is the numeric analogue of substring on a string, so add reciprocal "See also" links between the in-source docs of the two functions. Requested by @rschu1ze on PR ClickHouse#109012. Uses the inline See-also convention already used by scalar functions (FunctionDocumentation has no related_functions field). Docs-only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Closes: #9077
Changelog category (leave one):
Changelog entry:
Add
digits(n, offset[, length])function which returns the number innwhich starts at indexoffset(1-indexed)and haslengthnumber of digits. Iflengthis not provided, the function returns the number from offset till end.Version info
26.7.1.871(included in26.7and later)