Skip to content

[CALCITE-3604] Fixing locale error message in ExtractValue Function #1666

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.

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

Closed
wants to merge 3 commits into from

Conversation

ritesh-kapoor
Copy link
Contributor

No description provided.

@@ -35,6 +37,10 @@
*/
public class SqlXmlFunctionsTest {

@BeforeAll public static void setUp() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could not find a better way to fix the test case. Explored LocaleEnUs but no luck

@ritesh-kapoor ritesh-kapoor changed the title [CALCITE-3604] Fixing SqlXmlFunctionsTest locale [CALCITE-3604] Fixing SqlXmlFunctionsTest failure caused by localised exception message Dec 17, 2019
@DonnyZone
Copy link
Contributor

Changing the Locale can't fix the issue.
We'd better define a new Exception with customized message.

@ritesh-kapoor ritesh-kapoor changed the title [CALCITE-3604] Fixing SqlXmlFunctionsTest failure caused by localised exception message [CALCITE-3604] Fixing locale error message in ExtractValue Function Dec 17, 2019
ExInst<CalciteException> illegalBehaviorInExtractValueFunc(String errorBehavior, String value,
String xpath);
@BaseMessage("Illegal behavior in EXTRACTVALUE: xml: ''{0}'', xpath expression: ''{1}''")
ExInst<CalciteException> illegalBehaviorInExtractValueFunc(String xml, String xpath);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the exception content is not demonstrated, "Illegal behavior" may be confusing to end-uses.
Can we keep the message style same as that of Json function, i.e., "Invalid input for ..."? E.g.,

  @BaseMessage("Invalid input for JSON_REMOVE: document: ''{0}'', jsonpath expressions: ''{1}''")
  ExInst<CalciteException> invalidInputForJsonRemove(String value, String pathSpecs);  

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I have verified this change in my environment, it works well. Thanks!

@DonnyZone
Copy link
Contributor

LGTM

@danny0405 danny0405 added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Dec 19, 2019
@danny0405 danny0405 closed this in a9b23c7 Dec 21, 2019
wangxlong pushed a commit to wangxlong/calcite that referenced this pull request Feb 13, 2020
* Removing exception from xml function's error messages;
* Renaming exception method to invalid input for ExtractValue.

close apache#1666
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM-will-merge-soon Overall PR looks OK. Only minor things left.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants