Skip to content

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

Closed
ritesh-kapoor wants to merge 3 commits intoapache:masterfrom
ritesh-kapoor:CALCITE-3604
Closed

[CALCITE-3604] Fixing locale error message in ExtractValue Function#1666
ritesh-kapoor wants to merge 3 commits intoapache:masterfrom
ritesh-kapoor:CALCITE-3604

Conversation

@ritesh-kapoor
Copy link
Contributor

No description provided.

*/
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
jamesstarr pushed a commit to jamesstarr/calcite that referenced this pull request Mar 16, 2026
* Removing exception from xml function's error messages;
* Renaming exception method to invalid input for ExtractValue.

close apache#1666
jamesstarr pushed a commit to jamesstarr/calcite that referenced this pull request Mar 16, 2026
* 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