Skip to content

[CALCITE-3008] Create AuxiliaryConverterFactory#1170

Closed
walterddr wants to merge 3 commits intoapache:masterfrom
walterddr:branch-exp-CALCITE-3008
Closed

[CALCITE-3008] Create AuxiliaryConverterFactory#1170
walterddr wants to merge 3 commits intoapache:masterfrom
walterddr:branch-exp-CALCITE-3008

Conversation

@walterddr
Copy link

Copy link
Member

@hsyuan hsyuan left a comment

Choose a reason for hiding this comment

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

Why do you want pass factory class instead of just interface AuxiliaryConverter in SqlToRelConverter?

@walterddr
Copy link
Author

Why do you want pass factory class instead of just interface AuxiliaryConverter in SqlToRelConverter?

That's actually a good point. SqlToRelConverter requires us to generate one AuxiliaryConverter instance for each SqlFunction (e.g. the List), so it would be very inefficient to pass a reflective class object and then call the newInstance method.

I do have the option to change the AuxiliaryConverter API to do

RexNode convert(RexBuilder rexBuilder, RexNode groupCall, RexNode e, SqlFunction sqlCall);

With the extra parameter, we can make the AuxiliaryConverter static. This require to change a public interface which I hesitated to do so.

Any advice?

@hsyuan
Copy link
Member

hsyuan commented Apr 18, 2019

Let's discuss in JIRA.

@walterddr
Copy link
Author

close and opened #1219

@walterddr walterddr closed this May 19, 2019
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