Aggr statements with same error msg to keep error msg in 1.2-dev

[xzxiong]

User description

Aggr statements with same error msg and keep the error msg info #18537

What type of PR is this?

• API-change
• BUG
• Improvement
• Documentation
• Feature
• Test and CI
• Code Refactoring

Which issue(s) this PR fixes:

issue #https://github.com/matrixorigin/MO-Cloud/issues/4005

ref pr : #18537

What this PR does / why we need it:

changes:

• aggr erorr statement filter with same error msg.

---

PR Type

Enhancement, Tests

---

Description

• Enhanced error handling by retaining error messages in statement aggregation.
• Added a new Error field in the Key struct to include error messages.
• Implemented a function to convert error objects to strings for consistent key generation.
• Introduced comprehensive tests to verify the handling of error messages in statement keys.
• Added SQL test cases and expected results to validate the aggregation of error statements.

---

Changes walkthrough 📝

	Relevant files
Enhancement	report_statement.go Enhance error handling in statement aggregation                    pkg/util/trace/impl/motrace/report_statement.go Retained error message in statement aggregation. Added Error field to Key struct. Introduced getErrorString function to handle error messages. +18/-3
Tests	report_statement_test.go Add tests for statement key error handling                              pkg/util/trace/impl/motrace/report_statement_test.go Added tests for StatementInfo.Key function. Verified error message handling in key generation. +105/-1  aggr_error_stmt.result Add expected results for error statement aggregation          test/distributed/cases/statement_query_type/aggr_error_stmt.result Added expected results for aggregated error statements. Included multiple SQL parser error scenarios. +33/-0    aggr_error_stmt.sql Add test cases for error statement aggregation                      test/distributed/cases/statement_query_type/aggr_error_stmt.sql Added test cases for error statement aggregation. Simulated multiple SQL parser error scenarios. +32/-0    aggr_error_stmt.result Add summary results for error statement aggregation            test/distributed/cases/zz_statement_query_type/aggr_error_stmt.result Added summary results for aggregated error statements. Validated count and sum of aggregated errors. +5/-0      aggr_error_stmt.sql Add SQL to verify error statement aggregation results        test/distributed/cases/zz_statement_query_type/aggr_error_stmt.sql Added SQL to check aggregated error statement results. Ensured result format consistency. +6/-0

---

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

[qodo-merge-pro]

PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Key issues to review Code Change The Error field is now retained in the Key struct, which may affect how statements are aggregated. This change should be carefully reviewed to ensure it doesn't introduce unintended side effects in statement aggregation logic. New Function A new function getErrorString has been introduced to convert error objects to strings. This function should be reviewed for correctness and edge cases. Modified Function The Key method of StatementInfo has been significantly modified. It now includes the error string in the key. This change should be reviewed for potential impacts on existing code that relies on this method.

[qodo-merge-pro]

PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

PR Code Suggestions ✨

Category	Suggestion	Score
Enhancement	Refactor the test function to use a more concise table-driven test structure Consider using table-driven tests for the TestStatementInfo_Key function to improve test readability and maintainability. pkg/util/trace/impl/motrace/report_statement_test.go [384-481] func TestStatementInfo_Key(t *testing.T) { - now := time.Now() nowSub5sec := now.Add(-5 * time.Second) dummyError := moerr.NewInternalError(context.TODO(), "dummy test errmsg") - type fields struct { - SessionID [16]byte - SqlSourceType string - StatementType string - Status StatementInfoStatus - Error error - ResponseAt time.Time - } - type args struct { + + tests := []struct { + name string + info StatementInfo duration time.Duration - } - tests := []struct { - name string - fields fields - args args - want table.WindowKey + want Key }{ { name: "normal", - fields: fields{ + info: StatementInfo{ SessionID: NilSesID, SqlSourceType: "source_01", StatementType: "stmt_type_01", Status: StatementStatusSuccess, Error: nil, ResponseAt: now, }, - args: args{duration: 5 * time.Second}, + duration: 5 * time.Second, want: Key{ SessionID: NilSesID, StatementType: "stmt_type_01", Window: now.Truncate(5 * time.Second), Status: StatementStatusSuccess, SqlSourceType: "source_01", Error: "", }, }, // ... other test cases ... } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - s := &StatementInfo{ - SessionID: tt.fields.SessionID, - SqlSourceType: tt.fields.SqlSourceType, - StatementType: tt.fields.StatementType, - Status: tt.fields.Status, - Error: tt.fields.Error, - ResponseAt: tt.fields.ResponseAt, - } - assert.Equalf(t, tt.want, s.Key(tt.args.duration), "Key(%v)", tt.args.duration) - t.Logf("error: %v", tt.want.(Key).Error) + got := tt.info.Key(tt.duration) + assert.Equal(t, tt.want, got) + t.Logf("error: %v", tt.want.Error) }) } } Suggestion importance[1-10]: 8 Why: Refactoring to a more concise table-driven test structure significantly improves test readability and maintainability, making it easier to add or modify test cases in the future.	8
	Rename the method to better reflect its purpose Consider using a more descriptive name for the Key method, such as GenerateKey, to better reflect its purpose of creating a new Key struct. pkg/util/trace/impl/motrace/report_statement.go [270-279] -func (s *StatementInfo) Key(duration time.Duration) any { +func (s *StatementInfo) GenerateKey(duration time.Duration) any { return Key{ SessionID: s.SessionID, StatementType: s.StatementType, Window: s.ResponseAt.Truncate(duration), Status: s.Status, SqlSourceType: s.SqlSourceType, Error: getErrorString(s.Error), } } Apply this suggestion Suggestion importance[1-10]: 7 Why: Renaming the method to GenerateKey provides clarity on its purpose, enhancing code readability and maintainability, which is beneficial for understanding the codebase.	7
Best practice	Use a constant for the empty string instead of a literal Consider using a constant or a predefined variable for the empty string instead of a literal "" in the getErrorString function. This can improve code readability and maintainability. pkg/util/trace/impl/motrace/report_statement.go [263-268] +const emptyErrorString = "" + func getErrorString(err error) string { if err == nil { - return "" + return emptyErrorString } return err.Error() } Apply this suggestion Suggestion importance[1-10]: 5 Why: While using a constant for the empty string can improve readability and maintainability, the benefit is minor in this context as the function is simple and the literal is clear.	5