-
Notifications
You must be signed in to change notification settings - Fork 25.5k
ES|QL: Add unsigned long and mixed numeric types support to Decay function #134440
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
ES|QL: Add unsigned long and mixed numeric types support to Decay function #134440
Conversation
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
ℹ️ Important: Docs version tagging👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version. We use applies_to tags to mark version-specific features and changes. Expand for a quick overviewWhen to use applies_to tags:✅ At the page level to indicate which products/deployments the content applies to (mandatory) What NOT to do:❌ Don't remove or replace information that applies to an older version 🤔 Need help?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good start! But I think it doesn't address unsigned long values - please check the comments
x-pack/plugin/esql/qa/testFixtures/src/main/resources/decay.csv-spec
Outdated
Show resolved
Hide resolved
.../esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/score/Decay.java
Show resolved
Hide resolved
.../src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/score/DecayTests.java
Outdated
Show resolved
Hide resolved
testCaseSuppliers.addAll(longRandomTestCases()); | ||
|
||
// Unsigned Long Linear | ||
testCaseSuppliers.addAll(unsignedLongTestCase(0L, 10L, 10000000L, 200L, 0.33, "linear", 1.0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check how unsigned long test cases are generated for some ideas on how to generate numbers that don't fit into long
} else if (isMillisOrNanos(valueType)) { | ||
return validateOriginAndScale(DataType::isMillisOrNanos, "datetime or date_nanos", DataType::isTimeDuration, "time_duration"); | ||
} else if (isUnsignedLong(valueType)) { | ||
return validateOriginAndScale(DataType::isUnsignedLong, "unsigned long", DataType::isUnsignedLong, "unsigned_long"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's enough to check if origin and scale are numeric? It should be possible to mix and match numeric types, i.e. use an unsigned long origin but an int scale etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point, I wanted to restrict it first to make it a bit simpler, but giving it a second thought it definitely makes sense to support mixed numeric types.
Adjusted with:
Full disclosure: we need to the same for offset
. I'll add it to #134789, which I've opened yesterday and address it in a separate PR.
} | ||
|
||
private Object convertToExpectedType(Object value, DataType valueType, DataType targetType) { | ||
if (targetType == INTEGER && value instanceof Integer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can use EsqlDataTypeConverter.convert()
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right! Adjusted with Use EsqlDataTypeConverter for most cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @timgrein !
Follow-up for #132729.
This PR adds
unsigned_long
support to thedecay(...)
function