-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Group Concat function support for separator #16237
Conversation
Signed-off-by: Manan Gupta <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16237 +/- ##
==========================================
+ Coverage 68.40% 68.59% +0.18%
==========================================
Files 1556 1544 -12
Lines 195121 197984 +2863
==========================================
+ Hits 133479 135811 +2332
- Misses 61642 62173 +531 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Manan Gupta <[email protected]>
Signed-off-by: Manan Gupta <[email protected]>
Signed-off-by: Manan Gupta <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that we want to add vitess-tester tests, but there is a cost of adding new test files. For a single query testing should be not update the existing group_concat test in TestGroupConcatAggregation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use vitess_tester
for all new tests, because of how much easier it is. Currently, the workflow is running in 6m 24s
seconds, even after adding this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather like to move all the older tests to vitess_tester
Signed-off-by: Manan Gupta <[email protected]>
Signed-off-by: Manan Gupta <[email protected]>
Signed-off-by: Manan Gupta <[email protected]>
Description
As pointed out in #16238 the query there panics the vtgate which eventually crashes it.
This PR does 2 fixes -
Related Issue(s)
Group_concat
with separator crashes vtgate during planning #16238Checklist
Deployment Notes