Arnab Karmakar has posted comments on this change. ( http://gerrit.cloudera.org:8080/24422 )
Change subject: IMPALA-15081: Add ISO timestamp filters ...................................................................... Patch Set 10: (4 comments) Looks good overall! Just left a few minor comments. http://gerrit.cloudera.org:8080/#/c/24422/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24422/10//COMMIT_MSG@7 PS10, Line 7: IMPALA-15081: Add ISO timestamp filters Since only the extended format is accepted I think mentioning something like "ISO-8601 extended format (RFC 3339 style)" is more accurate. http://gerrit.cloudera.org:8080/#/c/24422/10/be/src/util/impala-profile-tool.cc File be/src/util/impala-profile-tool.cc: http://gerrit.cloudera.org:8080/#/c/24422/10/be/src/util/impala-profile-tool.cc@642 PS10, Line 642: TrimTrailingFractionalZeros I checked that --min_time=2026-08-03T01:54:55.Z is accepted. That's not valid ISO 8601. I think we should include the case in filter_rejects_invalid_time too. http://gerrit.cloudera.org:8080/#/c/24422/10/tests/observability/test_profile_tool.py File tests/observability/test_profile_tool.py: http://gerrit.cloudera.org:8080/#/c/24422/10/tests/observability/test_profile_tool.py@156 PS10, Line 156: test_iso8601_timestamp_filter I think a lower bound that lands earlier than intended produces exactly the same output. So these might also pass: - the offset being ignored entirely, - the offset magnitude being wrong in that direction. Can we test offsets at the boundary by expressing first+1 ms in +05:30, -0200 and +03 forms, and diff the output against --min_timestamp=first+1. Or set --max_time to first-1 ms in offset form and assert the output is empty. --min_time=2026-08-02T21:54:55.212-0200 (2 hours too early) and the correct 2026-08-02T23:54:55.212-0200 both return the full output http://gerrit.cloudera.org:8080/#/c/24422/10/tests/observability/test_profile_tool.py@218 PS10, Line 218: def test_iso8601_timestamp_filter_rejects_invalid_time(self): Since the parser is shared so the risk is low but we could check --max_time. It would cover the flag name in the error message. -- To view, visit http://gerrit.cloudera.org:8080/24422 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2c43e7535db48518b7d9dd0cbb87398e639c3b72 Gerrit-Change-Number: 24422 Gerrit-PatchSet: 10 Gerrit-Owner: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Mon, 05 Oct 2026 04:17:16 +0000 Gerrit-HasComments: Yes
