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.

Sign up for GitHub

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

Jump to bottom

fix(utils): evaluate date parser multiple holiday results correctly #23685

Merged
merged 5 commits into from
Apr 26, 2023
Merged

fix(utils): evaluate date parser multiple holiday results correctly #23685

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c3f7a01
Fix datetime_eval holiday multiple result issue
arkid15r Apr 13, 2023
d9b7c17
Update dependencies.
arkid15r Apr 21, 2023
8b1e3a8
Sync with the master.
arkid15r Apr 21, 2023
0453db1
Merge branch 'master' into fix-date-eval-parser
arkid15r Apr 21, 2023
2e8e9b8
Revert local mypy fix.
arkid15r Apr 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ def get_git_sha() -> str:
"graphlib-backport",
"gunicorn>=20.1.0; sys_platform != 'win32'",
"hashids>=1.3.1, <2",
"holidays>=0.17.2, <0.18",
"holidays>=0.22, <0.23",
"humanize",
"isodate",
"markdown>=3.0",
Expand Down
4 changes: 2 additions & 2 deletions superset/utils/date_parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,8 +386,8 @@ def eval(self) -> datetime:
country = country.eval() if country else "US"

holiday_lookup = country_holidays(country, years=[holiday_year], observed=False)
searched_result = holiday_lookup.get_named(holiday)
if len(searched_result) == 1:
searched_result = holiday_lookup.get_named(holiday, lookup="istartswith")
if len(searched_result) > 0:
return dttm_from_timetuple(searched_result[0].timetuple())
raise ValueError(
_("Unable to find such a holiday: [%(holiday)s]", holiday=holiday)
Expand Down
22 changes: 22 additions & 0 deletions tests/unit_tests/utils/date_parser_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,14 @@ def mock_parse_human_datetime(s: str) -> Optional[datetime]:
return datetime(2017, 4, 7)
elif s in ["5 days", "5 days ago"]:
return datetime(2016, 11, 2)
elif s == "2000-01-01T00:00:00":
return datetime(2000, 1, 1)
elif s == "2018-01-01T00:00:00":
return datetime(2018, 1, 1)
elif s == "2018-12-31T23:59:59":
return datetime(2018, 12, 31, 23, 59, 59)
elif s == "2022-01-01T00:00:00":
return datetime(2022, 1, 1)
else:
return None

Expand Down Expand Up @@ -260,12 +264,30 @@ def test_datetime_eval() -> None:
expected = datetime(2018, 9, 3, 0, 0, 0)
assert result == expected

result = datetime_eval(
"holiday('Eid al-Fitr', datetime('2000-01-01T00:00:00'), 'SA')"
)
expected = datetime(2000, 1, 8, 0, 0, 0)
assert result == expected

result = datetime_eval(
"holiday('Boxing day', datetime('2018-01-01T00:00:00'), 'UK')"
)
expected = datetime(2018, 12, 26, 0, 0, 0)
assert result == expected

result = datetime_eval(
"holiday('Juneteenth', datetime('2022-01-01T00:00:00'), 'US')"
)
expected = datetime(2022, 6, 19, 0, 0, 0)
assert result == expected

result = datetime_eval(
"holiday('Independence Day', datetime('2022-01-01T00:00:00'), 'US')"
)
expected = datetime(2022, 7, 4, 0, 0, 0)
assert result == expected

Comment on lines +279 to +290
Copy link
Member

Choose a reason for hiding this comment

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

Hi there, the US holiday case was added to L273 so the duplicated test case is not necessary. Thanks for supporting more widely holiday types!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @zhaoyongjie,

I read L273 as UK's (technically GB) test. And even if it was the US test I'd suggest to keep the L279-289 Juneteenth related tests as they specifically cover the wrong parser behaviour this PR trying to address.

Thanks for reviewing this!

result = datetime_eval(
"lastday(dateadd(datetime('2018-01-01T00:00:00'), 1, month), month)"
)
Expand Down