[CALCITE-6684] Arrow adapter should supports filter conditions of Decimal type #4043
Conversation
caicancai
changed the title
| @@ -194,6 +194,8 @@ private static TreeNode makeLiteralNode(String literal, String type) { | |||
| return TreeBuilder.makeLiteral(parseFloat(literal)); | |||
| case "double": | |||
| return TreeBuilder.makeLiteral(parseDouble(literal)); | |||
| case "decimal": | |||
| return TreeBuilder.makeDecimalLiteral(literal, 19, 0); | |||
There was a problem hiding this comment.
Comply with Calcite's default Decimal type
There was a problem hiding this comment.
You should use the type system to retrieve the default type, assuming it is available here.
Maybe there is a default type for Arrow, but the "default" type for Calcite is irrelevant in this context.
There was a problem hiding this comment.
There is no way this can be right, this drops all digits after the decimal point.
At least make 19 and 0 some constants like ARROW_DEFAULT_PRECISION and ARROW_DEFAULT_SCALE.
| private static String getLiteralType(Object literal, RelDataType type) { | ||
| if (type.getSqlTypeName() == SqlTypeName.DECIMAL) { | ||
| return "decimal"; | ||
| } else if (literal instanceof BigDecimal) { |
There was a problem hiding this comment.
since now you are passing type information to this function you should use it everywhere. There is no reason to guess the type based on the Java type of the literal.
|
| @@ -194,6 +194,8 @@ private static TreeNode makeLiteralNode(String literal, String type) { | |||
| return TreeBuilder.makeLiteral(parseFloat(literal)); | |||
| case "double": | |||
| return TreeBuilder.makeLiteral(parseDouble(literal)); | |||
| case "decimal": | |||
| return TreeBuilder.makeDecimalLiteral(literal, 19, 0); | |||
There was a problem hiding this comment.
You should use the type system to retrieve the default type, assuming it is available here.
Maybe there is a default type for Arrow, but the "default" type for Calcite is irrelevant in this context.
| } else if (type.getSqlTypeName() == SqlTypeName.VARCHAR | ||
| || type.getSqlTypeName() == SqlTypeName.CHAR) { | ||
| return "string"; | ||
| } else { |
There was a problem hiding this comment.
there are many unhandled cases, but perhaps this is a good start.
There was a problem hiding this comment.
@mihaibudiu
Unfortunately, Arrow does not have a default decimal type.
The Arrow adapter currently converts Decimal types to DECIMAL(19, 0) by default. I don't know why the previous person implemented it this way. It seems that he simply called the calcite native API without considering any precision issues.
So in this pr, the decimal type I specified is also DECIMAL(19, 0)
I am working on a new type system for the Arrow adapter, which seems to be a big project
https://issues.apache.org/jira/browse/CALCITE-6684