Aleksandr Efimov has posted comments on this change. ( http://gerrit.cloudera.org:8080/24422 )
Change subject: IMPALA-15081: Text time filters in profile tool ...................................................................... Patch Set 10: (7 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 lik Updated in PS11. The subject now names profile tool; the description says ISO-8601 extended format (RFC 3339 style). http://gerrit.cloudera.org:8080/#/c/24422/10//COMMIT_MSG@13 PS10, Line 13: Add --min_time and --max_time as timezone-aware ISO-8601 : alternatives. The new flags are converted to the same Unix epoch : millisecond bounds used by the existing filtering path, while rejecting : mixed use with the epoch millisecond flags. > Can you add some examples of what is accepted? For example are named timezo Added examples in PS11. Only Z and numeric offsets (+HH:MM, +HHMM, +HH) are accepted; named timezones are rejected. http://gerrit.cloudera.org:8080/#/c/24422/10//COMMIT_MSG@18 PS10, Line 18: New flags rather than a second accepted form on the existing ones. The > nit: the paragraph is a bit verbose IMO Shortened the explanation of the two flag names in PS11. 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 val Fixed in PS11. The empty fractional part is rejected, with regression cases for both --min_time and --max_time. http://gerrit.cloudera.org:8080/#/c/24422/10/be/src/util/impala-profile-tool.cc@652 PS10, Line 652: ParseTimezoneAwareTime > Isn't there a lib that does all this from us? Switched to absl::ParseTime in PS11. Kept a syntax guard because ParseTime also accepts lowercase z, normalizes leap seconds and truncates extra fractional digits. 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 Updated in PS11. Offsets are tested at first+1 ms against --min_timestamp, and at first-1 ms for --max_time, for +03:00, +05:30, -0200 and +03. 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 Added invalid-input checks for --max_time in PS11, including its name in the error. -- 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 10:16:07 +0000 Gerrit-HasComments: Yes
