-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
SQL: Fix issue regarding INTERVAL * number #42014
Conversation
Interval * integer number is a valid operation which previously was only supported for foldables (literals) and not when a field was involved. That was because: 1. There was no common type returned for that combination 2. The `BinaryArithmeticOperation` was permitting the multiplication (called by fold()) but the BinaryArithmeticProcessor didn't allow it Moreover the error message for invalid arithmetic operations was wrong because of the issue with the overloading methods of `LoggerMessageFormat.format`. Fixes: elastic#41239
Pinging @elastic/es-search |
Also fixes: #41200 |
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. Left one comment.
DataType l = left().dataType(); | ||
DataType r = right().dataType(); | ||
|
||
if (!(r.isDateOrTimeBased() || DataTypes.isInterval(r))) { |
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.
Lines 60 to 65 in 542d1ee
if (!(r.isDateOrTimeBased() || DataTypes.isInterval(r))) { | |
return new TypeResolution(format(null, "[{}] has arguments with incompatible types [{}] and [{}]", symbol(), l, r)); | |
} | |
if (!(l.isDateOrTimeBased() || DataTypes.isInterval(l))) { | |
return new TypeResolution(format(null, "[{}] has arguments with incompatible types [{}] and [{}]", symbol(), l, r)); | |
} |
The code here seems a duplicate. You could combine the two conditions, no?
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.
Could you add a test with one and multiple negative values to see how the sign is being handled?
Thanks,
@costin could you please explain a bit more? What do you mean with |
|
Interval * integer number is a valid operation which previously was only supported for foldables (literals) and not when a field was involved. That was because: 1. There was no common type returned for that combination 2. The `BinaryArithmeticOperation` was permitting the multiplication (called by fold()) but the BinaryArithmeticProcessor didn't allow it Moreover the error message for invalid arithmetic operations was wrong because of the issue with the overloading methods of `LoggerMessageFormat.format`. Fixes: #41239 Fixes: #41200 (cherry picked from commit 91039ba)
Interval * integer number is a valid operation which previously was only supported for foldables (literals) and not when a field was involved. That was because: 1. There was no common type returned for that combination 2. The `BinaryArithmeticOperation` was permitting the multiplication (called by fold()) but the BinaryArithmeticProcessor didn't allow it Moreover the error message for invalid arithmetic operations was wrong because of the issue with the overloading methods of `LoggerMessageFormat.format`. Fixes: #41239 Fixes: #41200 (cherry picked from commit 91039ba)
Interval * integer number is a valid operation which previously was only supported for foldables (literals) and not when a field was involved. That was because: 1. There was no common type returned for that combination 2. The `BinaryArithmeticOperation` was permitting the multiplication (called by fold()) but the BinaryArithmeticProcessor didn't allow it Moreover the error message for invalid arithmetic operations was wrong because of the issue with the overloading methods of `LoggerMessageFormat.format`. Fixes: #41239 Fixes: #41200 (cherry picked from commit 91039ba)
Interval * integer number is a valid operation which previously was only supported for foldables (literals) and not when a field was involved. That was because: 1. There was no common type returned for that combination 2. The `BinaryArithmeticOperation` was permitting the multiplication (called by fold()) but the BinaryArithmeticProcessor didn't allow it Moreover the error message for invalid arithmetic operations was wrong because of the issue with the overloading methods of `LoggerMessageFormat.format`. Fixes: elastic#41239 Fixes: elastic#41200
Interval * integer number is a valid operation which previously was
only supported for foldables (literals) and not when a field was
involved. That was because:
BinaryArithmeticOperation
was permitting the multiplication(called by fold()) but the BinaryArithmeticProcessor didn't allow it
Moreover the error message for invalid arithmetic operations was wrong
because of the issue with the overloading methods of
LoggerMessageFormat.format
.Fixes: #41239