Skip to content

[CALCITE-4177] Throw exception when deserialize SqlOperator fails, do not return null#2110

Closed
yanlin-Lynn wants to merge 1 commit intoapache:masterfrom
yanlin-Lynn:rexnode_deser
Closed

[CALCITE-4177] Throw exception when deserialize SqlOperator fails, do not return null#2110
yanlin-Lynn wants to merge 1 commit intoapache:masterfrom
yanlin-Lynn:rexnode_deser

Conversation

@yanlin-Lynn
Copy link
Copy Markdown
Contributor

See jira CALCITE-4177

}
return null;
throw new RuntimeException("No operator for " + name + " with kind: "
+ kind + ", and syntax: " + syntax);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will it better to move this exception in CalciteResource.java?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, I'll update

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this a breaking change ? Do you know the background that it returns null before ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agree with @danny0405.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, this breaks a little. I don't know the background why it returns null before.
Hopes someone can help to give the background, but I doubt there exists some special reason, because null will always cause NPE somewhere else.

@danny0405
Copy link
Copy Markdown
Contributor

We better also add some tests there ~

@yanlin-Lynn
Copy link
Copy Markdown
Contributor Author

We better also add some tests there ~

Thanks for review. But I didn't get your point, there is a test case already.

@danny0405
Copy link
Copy Markdown
Contributor

We better also add some tests there ~

Thanks for review. But I didn't get your point, there is a test case already.

Oops, i didn't saw that ~

@yanlin-Lynn yanlin-Lynn force-pushed the rexnode_deser branch 3 times, most recently from ab88c6a to da15f9a Compare August 24, 2020 06:24
@xy2953396112
Copy link
Copy Markdown
Contributor

LGTM~, Can you fix the CI?

@hsyuan
Copy link
Copy Markdown
Member

hsyuan commented Sep 20, 2021

Closed in ce86af8.

@hsyuan hsyuan closed this Sep 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants