Stage provider support amazon, minio and COS

[cpegeric]

What type of PR is this?

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

Which issue(s) this PR fixes:

issue # #18837

What this PR does / why we need it:

1. Remove the check that the provider only supports minio.
2. add 'COS' and 'AMAZON' provider support.

[codiumai-pr-agent-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: 2 🔵🔵⚪⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Key issues to review Incomplete Provider Support The getS3ServiceFromProvider function now supports 'cos', 'amazon', and 'minio', but it doesn't handle unsupported providers. Consider adding a default case to return an error for unsupported providers. Potential Bug The removal of the provider check might allow unsupported providers. Ensure that this change doesn't introduce unexpected behavior with other providers.

[codiumai-pr-agent-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	Add a test case for an unsupported provider to ensure error handling Consider adding a test case for an unsupported provider to ensure the function handles errors correctly. pkg/sql/plan/function/stage_test.go [25-38] func TestS3ServiceProvider(t *testing.T) { protocol, err := getS3ServiceFromProvider("cos") require.Nil(t, err) assert.Equal(t, protocol, "s3") protocol, err = getS3ServiceFromProvider("amazon") require.Nil(t, err) assert.Equal(t, protocol, "s3") protocol, err = getS3ServiceFromProvider("minio") require.Nil(t, err) assert.Equal(t, protocol, "minio") + _, err = getS3ServiceFromProvider("unsupported") + assert.Error(t, err) } Apply this suggestion Suggestion importance[1-10]: 9 Why: Adding a test case for an unsupported provider is crucial for ensuring robust error handling, which is a significant improvement in testing completeness.	9
	Use a map for provider-to-service mapping to improve scalability and maintainability Consider using a map for provider-to-service mapping to make the function more scalable and easier to maintain. pkg/sql/plan/function/stage_util.go [155-163] +var providerServiceMap = map[string]string{ + S3_PROVIDER_COS: S3_SERVICE, + S3_PROVIDER_AMAZON: S3_SERVICE, + S3_PROVIDER_MINIO: MINIO_SERVICE, +} + func getS3ServiceFromProvider(provider string) (string, error) { provider = strings.ToLower(provider) - switch provider { - case S3_PROVIDER_COS: - return S3_SERVICE, nil - case S3_PROVIDER_AMAZON: - return S3_SERVICE, nil - case S3_PROVIDER_MINIO: - return MINIO_SERVICE, nil + if service, ok := providerServiceMap[provider]; ok { + return service, nil + } + return "", fmt.Errorf("unsupported provider: %s", provider) +} Apply this suggestion Suggestion importance[1-10]: 8 Why: Using a map for provider-to-service mapping simplifies the code and improves scalability and maintainability, which is a valuable enhancement.	8
Best practice	Refactor the test function to use table-driven tests for better maintainability Use table-driven tests to make the test cases more concise and easier to maintain. pkg/sql/plan/function/stage_test.go [25-38] func TestS3ServiceProvider(t *testing.T) { - protocol, err := getS3ServiceFromProvider("cos") - require.Nil(t, err) - assert.Equal(t, protocol, "s3") + testCases := []struct { + provider string + expected string + }{ + {"cos", "s3"}, + {"amazon", "s3"}, + {"minio", "minio"}, + } - protocol, err = getS3ServiceFromProvider("amazon") - require.Nil(t, err) - assert.Equal(t, protocol, "s3") - - protocol, err = getS3ServiceFromProvider("minio") - require.Nil(t, err) - assert.Equal(t, protocol, "minio") - + for _, tc := range testCases { + t.Run(tc.provider, func(t *testing.T) { + protocol, err := getS3ServiceFromProvider(tc.provider) + require.NoError(t, err) + assert.Equal(t, tc.expected, protocol) + }) + } } Apply this suggestion Suggestion importance[1-10]: 7 Why: Refactoring to table-driven tests enhances code maintainability and readability, though it is not as critical as adding new test cases.	7

💡 Need additional feedback ? start a PR chat