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

Filtering the prefix in custom query log for pinot response comparator #5643

Merged
merged 5 commits into from
Feb 6, 2024

Conversation

bowenxia
Copy link
Contributor

@bowenxia bowenxia commented Feb 5, 2024

What changed?
Filtering the prefix in custom query log for pinot response comparator

Why?
The custom attributes in the log has "Attr." prefix. In order for the Pinot response comparator to directly use the log, we need to get rid of it.

How did you test it?
unit test

Potential risks

Release notes

Documentation Changes

@coveralls
Copy link

coveralls commented Feb 5, 2024

Pull Request Test Coverage Report for Build 018d7bd1-46c3-4caf-a92c-891061f7e2e1

  • -1 of 4 (75.0%) changed or added relevant lines in 1 file are covered.
  • 53 unchanged lines in 9 files lost coverage.
  • Overall coverage decreased (-0.02%) to 62.652%

Changes Missing Coverage Covered Lines Changed/Added Lines %
common/persistence/pinotVisibilityTripleManager.go 3 4 75.0%
Files with Coverage Reduction New Missed Lines %
common/task/parallel_task_processor.go 2 93.06%
service/history/execution/mutable_state_util.go 2 37.63%
service/history/task/transfer_active_task_executor.go 2 72.22%
service/matching/taskListManager.go 2 80.2%
service/history/task/fetcher.go 3 85.57%
service/frontend/wrappers/metered/metered.go 4 67.6%
service/history/task/transfer_standby_task_executor.go 4 85.77%
common/persistence/sql/workflowStateMaps.go 8 81.25%
service/history/execution/mutable_state_task_refresher.go 26 56.65%
Totals Coverage Status
Change from base Build 018d7b4b-3d19-4694-90fe-65c2b94ea996: -0.02%
Covered Lines: 92153
Relevant Lines: 147087

💛 - Coveralls

}

func filterAttrPrefix(str string) string {
str = strings.Replace(str, "`Attr.", "", -1)
Copy link
Member

Choose a reason for hiding this comment

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

this can unintentionally replace value part in the query. correct way to do this would be to parse the whole query with regex capture groups or AST and then perform the sanitization. however this is probably a super edge case so don't block yourself on this but keep the current behavior caveat documented in the code and also add an example false positive test case to demonstrate it

Copy link
Contributor Author

@bowenxia bowenxia Feb 6, 2024

Choose a reason for hiding this comment

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

Comment and false positive unit test added.

@bowenxia bowenxia merged commit 83d55c7 into master Feb 6, 2024
16 checks passed
@bowenxia bowenxia deleted the xbowen_add_filter_for_comparator_log branch February 6, 2024 02:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants