Skip to content

[CALCITE-6667] Arrow adapter should push down arithmetic operations#4031

Closed
caicancai wants to merge 2 commits intoapache:mainfrom
caicancai:6667
Closed

[CALCITE-6667] Arrow adapter should push down arithmetic operations#4031
caicancai wants to merge 2 commits intoapache:mainfrom
caicancai:6667

Conversation

@caicancai
Copy link
Member

@caicancai caicancai commented Nov 3, 2024

@caicancai
Copy link
Member Author

Currently, only simple constant addition and subtraction operations are supported. I did not perform complex type tests because there are some other problems with the current adaptation of the arrow type.

case PLUS:
right = translateArithmeticOp(left, right);
return translateBinary2(op, ((RexCall) left).operands.get(0), right);
case MINUS:
Copy link
Contributor

Choose a reason for hiding this comment

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

these two cases could be merged

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

@caicancai caicancai changed the title [CALCITE-6667] The Filter operator supports simple addition and subtraction operations [CALCITE-6667] Arrow adapter should push down arithmetic operations Nov 4, 2024
@caicancai caicancai requested a review from mihaibudiu November 4, 2024 13:10
@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 4, 2024

* @return A new RexNode representing the adjusted right operand
*/
private RexNode translateArithmeticOp(RexNode left, RexNode right) {
RexNode leftOperand = ((RexCall) left).operands.get(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

This optimization is already part of PROJECT_REDUCE_EXPRESSIONS. I don't think it should be replicated in any specific backend.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if it works, and it seems no one has tried it yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what you mean, in RelOptRulesTest there are at least 59 tests that use this optimization.

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand what you mean, but it seems that no other adapters are specified for testing. I understand that RelOptRulesTest is for those Jdbc protocol dialects, such as mysql, postgres and so on. I am not sure whether adapters such as arrow, es, etc. are effective.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is incorrect, the RelOptRules are designed to work for all dialects.
A Calcite-based compiler will have a parser, a converter, an optimizer, and a back-end.
The job of doing optimizations belongs to the optimizer, not to the back-end.
If you put optimizations in the back-end, you have to reimplement them for every target.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for your answer, so I just need to translate the plan without any optimization, I will improve this pr later

Copy link
Member Author

Choose a reason for hiding this comment

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

@mihaibudiu It seems that Arrow's API does not support operations like "intField\" + 1 = 12. I had to convert it to "intField\" = 11 in Calcite.

Should I call the PROJECT_REDUCE_EXPRESSIONS optimization rule in the arrow adapter?

Copy link
Contributor

Choose a reason for hiding this comment

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

Adapters should never call optimizations.
This particular optimization does constant folding, and the expression you show does not have any constant subexpressions, so this optimization will not help.

@caicancai caicancai closed this Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants