-
Notifications
You must be signed in to change notification settings - Fork 156
Support decimal literal with Calcite #3673
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
Conversation
Signed-off-by: Lantao Jin <[email protected]>
@@ -166,7 +166,7 @@ void populate() { | |||
registerOperator(ADD, SqlStdOperatorTable.PLUS); | |||
registerOperator(SUBTRACT, SqlStdOperatorTable.MINUS); | |||
registerOperator(MULTIPLY, SqlStdOperatorTable.MULTIPLY); | |||
// registerOperator(DIVIDE, SqlStdOperatorTable.DIVIDE); | |||
registerOperator(TRUNCATE, SqlStdOperatorTable.TRUNCATE); |
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.
This is a missing function which is not related to this PR. But we need it in CalciteMathematicalFunctionIT.java
|
||
import org.opensearch.sql.common.setting.Settings; | ||
|
||
public class CalcitePPLCryptographicFunctionPushdownIT extends CalcitePPLCryptographicFunctionIT { |
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.
missing IT which is not related to this issue.
public class CalcitePPLDateTimeBuiltinFunctionPushdownIT | ||
extends CalcitePPLDateTimeBuiltinFunctionIT { |
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.
incorrect inheritance which is not related to this issue.
|
||
import org.opensearch.sql.common.setting.Settings; | ||
|
||
public class CalcitePPLFillnullPushdownIT extends CalcitePPLFillnullIT { |
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.
missing IT which is not related to this issue.
|
||
import org.opensearch.sql.common.setting.Settings; | ||
|
||
public class CalcitePPLIPFunctionPushdownIT extends CalcitePPLIPFunctionIT { |
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.
ditto
if (isCalciteEnabled()) { | ||
verifySchema(result, schema("f", null, "int")); | ||
} else { | ||
verifySchema(result, schema("f", null, "bigint")); | ||
} |
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.
return types between v2 and v3 are different, not related to this issue.
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.
Why dose this IT could pass before?
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.
Why dose this IT could pass before?
CalciteMathematicalFunctionIT
was missing before this PR.
Signed-off-by: Lantao Jin <[email protected]>
Signed-off-by: Lantao Jin <[email protected]>
Signed-off-by: Lantao Jin <[email protected]>
Signed-off-by: Lantao Jin <[email protected]>
Signed-off-by: Lantao Jin <[email protected]>
public class Literal extends UnresolvedExpression { | ||
|
||
private final Object value; | ||
private final DataType type; | ||
|
||
public Literal(Object value, DataType dataType) { | ||
if (dataType == DataType.DECIMAL && value instanceof Double) { |
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.
When will we create a decimal literal with double value?
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.
This is to be compatible with calls like new Literal(2.0, Decimal)
, otherwise we can only use new Literal(new BigDecimal("2.0"), Decimal)
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.
Thanks for the change! One minor suggestion would be to add a small note in the documentation about this change to help users understand the precision behavior, but that could also be addressed separately.
Sure, I will summary the break changes for 3.1.0 release in a new PR. |
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.
Thanks for the changes! Should we also update user manualhttps://github.com/opensearch-project/sql/blob/main/docs/user/ppl/general/datatypes.rst?
Decimal is not a new supported datatype, we'd better update that doc after #3619 addressed. I will highlight this breaking changes in release notes. |
Description
Support decimal literal with Calcite
d
(double),f
(float) suffixRelated Issues
Resolves #3614
Check List
--signoff
.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.