Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[CALCITE-6684] Arrow adapter should supports filter conditions of Decimal type #4043

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+70 −15
Open

[CALCITE-6684] Arrow adapter should supports filter conditions of Decimal type #4043

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
30ae763
[CALCITE-6684] Arrow adapter should supports filter conditions of Dec…
caicancai Nov 10, 2024
df012d8
Change to sqltype judgment
caicancai Nov 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions arrow/src/main/java/org/apache/calcite/adapter/arrow/ArrowTable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comply with Calcite's default Decimal type

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

case "string":
return TreeBuilder.makeStringLiteral(literal.substring(1, literal.length() - 1));
default:
Expand Down
8 changes: 5 additions & 3 deletions arrow/src/main/java/org/apache/calcite/adapter/arrow/ArrowTranslator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ private String translateBinary(String op, String rop, RexCall call) {
private String translateOp2(String op, String name, RexLiteral right) {
Object value = literalValue(right);
String valueString = value.toString();
String valueType = getLiteralType(value);
String valueType = getLiteralType(value, right.getType());

if (value instanceof String) {
final RelDataTypeField field = requireNonNull(rowType.getField(name, true, false), "field");
Expand Down Expand Up @@ -234,8 +234,10 @@ private String translateUnaryOp(String op, String name) {
return name + " " + op;
}

private static String getLiteralType(Object literal) {
if (literal instanceof BigDecimal) {
private static String getLiteralType(Object literal, RelDataType type) {
if (type.getSqlTypeName() == SqlTypeName.DECIMAL) {
return "decimal";
} else if (literal instanceof BigDecimal) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

BigDecimal bigDecimalLiteral = (BigDecimal) literal;
int scale = bigDecimalLiteral.scale();
if (scale == 0) {
Expand Down
20 changes: 20 additions & 0 deletions arrow/src/test/java/org/apache/calcite/adapter/arrow/ArrowAdapterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -943,4 +943,24 @@ static void initializeArrowState(@TempDir Path sharedTempDir)
.returns(result)
.explainContains(plan);
}

/** Test case for
* <a href="https://issues.apache.org/jira/browse/CALCITE-6684">[CALCITE-6684]
* Arrow adapter should supports filter conditions of Decimal type</a>. */
@Test void testArrowProjectFieldsWithDecimalFilter() {
String sql = "select \"decimalField\"\n"
+ "from arrowdatatype\n"
+ "where \"decimalField\" = 1.00";
String plan = "PLAN=ArrowToEnumerableConverter\n"
+ " ArrowProject(decimalField=[$8])\n"
+ " ArrowFilter(condition=[=($8, 1)])\n"
+ " ArrowTableScan(table=[[ARROW, ARROWDATATYPE]], fields=[[0, 1, 2, 3, 4, 5, 6, 7, 8, 9]])\n\n";
String result = "decimalField=1.00\n";

CalciteAssert.that()
.with(arrow)
.query(sql)
.returns(result)
.explainContains(plan);
}
}
Loading