Skip to content
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

case search "date_add" function #31388

Merged
merged 12 commits into from
Apr 5, 2022
Merged

case search "date_add" function #31388

merged 12 commits into from
Apr 5, 2022

Conversation

snopoke
Copy link
Contributor

@snopoke snopoke commented Apr 1, 2022

CEP: #31325

Product Description

Adds a 'date_add' function to the case search query language to support calculating relative dates:

date_add('2020-02-29', 'years', 1) = "2021-02-28"

Technical Summary

Implements the date_add function as outlined in the CEP. See test cases for usage.

Best reviewed by commit.

Feature Flag

Case Search, CLE

Safety Assurance

Safety story

New case search function should not have any impact until it is used in a query. The code for the function and case search queries in general is well tested.

Automated test coverage

Added tests for the specific function.

QA Plan

None

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@snopoke snopoke added the product/feature-flag Change will only affect users who have a specific feature flag enabled label Apr 1, 2022
@snopoke snopoke requested review from millerdev and esoergel April 1, 2022 19:58
@snopoke snopoke requested a review from proteusvacuum as a code owner April 1, 2022 19:58
@@ -12,6 +12,8 @@
def unwrap_value(domain, node):
"""Returns the value of the node if it is wrapped in a function, otherwise just returns the node
"""
if isinstance(node, (str, int, float, bool)):
return node
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious what this achieves that the if not isinstance(node, FunctionCall) line doesn't catch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. What I was wanting to do was only permit certain types here. For example it should not return a BinaryExpression. But on more reflection perhaps that doesn't belong here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

assert node.name == 'date_add'
if len(node.args) != 3:
raise XPathFunctionException(
_("The \"date_add\" function expects three arguments"),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe add which arguments it needs?

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_("The \"date_add\" function expects three arguments"),
_("The \"date_add\" function expects three arguments, got {count}".format(
count=len(node.args),
)),

try:
result = date + relativedelta(**{interval_type: quantity})
except ValueError as e:
raise XPathFunctionException(str(e), serialize(node))
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think this will show a python style error to the user which might be hard to understand. Is there another way we could show this error to the user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've handled the value error but kept the try block for unexpected exceptions: 3c448f5

assert node.name == 'date_add'
if len(node.args) != 3:
raise XPathFunctionException(
_("The \"date_add\" function expects three arguments"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_("The \"date_add\" function expects three arguments"),
_("The \"date_add\" function expects three arguments, got {count}".format(
count=len(node.args),
)),

Copy link
Contributor

@millerdev millerdev left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the formatting error on my got {count} suggestion.

@snopoke snopoke merged commit a82a27c into master Apr 5, 2022
@snopoke snopoke deleted the sk/date_add branch April 5, 2022 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/feature-flag Change will only affect users who have a specific feature flag enabled
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants