Skip to content

3.0: fix: support parquet s3option and '=' object keys on 3.0-dev #24022

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.

Sign up for GitHub

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

Closed
gouhongshen wants to merge 4 commits into from
Closed

3.0: fix: support parquet s3option and '=' object keys on 3.0-dev #24022

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fceb1b7
fix: support parquet format in S3/stage load on 3.0-dev
gouhongshen Mar 30, 2026
b7d91a0
fix: allow '=' in fileservice path validation
gouhongshen Mar 30, 2026
ee82012
test(fileservice): cover '=' path parsing
gouhongshen Mar 30, 2026
78029ca
test(plan): validate parquet format in s3/stage init
gouhongshen Mar 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/fileservice/path.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ func ParsePath(s string) (path Path, err error) {
continue
}
switch r {
case '!', '-', '_', '.', '*', '\'', '(', ')', '@', '/':
case '!', '-', '_', '.', '*', '\'', '(', ')', '@', '/', '=':
continue
Comment on lines 69 to 71
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

ParsePath now allows '=' in the file component, but the existing path parsing tests don’t cover this new allowed character. Please add a unit test case that includes '=' in the file portion (e.g., an S3 key like "a=b/c.txt") to prevent regressions in the validation logic.

Copilot uses AI. Check for mistakes.
}
// printable non-ASCII characters
Expand Down
5 changes: 5 additions & 0 deletions pkg/fileservice/path_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,4 +67,9 @@ func TestParsePath(t *testing.T) {
assert.Nil(t, err)
assert.Equal(t, "矩阵", path.File)
assert.Equal(t, "矩阵", path.String())

path, err = ParsePath("sjrq=20200504/a.parquet")
assert.Nil(t, err)
assert.Equal(t, "sjrq=20200504/a.parquet", path.File)
assert.Equal(t, "sjrq=20200504/a.parquet", path.String())
}
4 changes: 2 additions & 2 deletions pkg/sql/plan/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1707,7 +1707,7 @@ func InitS3Param(param *tree.ExternParam) error {
param.S3Param.ExternalId = param.Option[i+1]
case "format":
format := strings.ToLower(param.Option[i+1])
if format != tree.CSV && format != tree.JSONLINE {
if format != tree.CSV && format != tree.JSONLINE && format != tree.PARQUET {
return moerr.NewBadConfigf(param.Ctx, "the format '%s' is not supported", format)
}
param.Format = format
Comment on lines 1709 to 1713
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

InitS3Param now accepts parquet as a valid format, but there’s no unit test covering the format validation path (including a parquet-allowed case and an unsupported-format rejection). Since pkg/sql/plan/utils_test.go already exists, adding a focused test would help prevent regressions in LOAD DATA URL S3OPTION parsing.

Copilot uses AI. Check for mistakes.
Expand Down Expand Up @@ -1795,7 +1795,7 @@ func InitStageS3Param(param *tree.ExternParam, s stage.StageDef) error {
switch strings.ToLower(param.Option[i]) {
case "format":
format := strings.ToLower(param.Option[i+1])
if format != tree.CSV && format != tree.JSONLINE {
if format != tree.CSV && format != tree.JSONLINE && format != tree.PARQUET {
return moerr.NewBadConfigf(param.Ctx, "the format '%s' is not supported", format)
}
param.Format = format
Comment on lines 1796 to 1801
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

InitStageS3Param’s format validation was extended to allow parquet, but TestInitStageS3Param currently doesn’t exercise the 'format' option at all. Please add a test case that sets param.Option with format='parquet' (and one invalid format) to ensure stage-based S3 loads keep validating formats correctly.

Copilot uses AI. Check for mistakes.
Expand Down
52 changes: 52 additions & 0 deletions pkg/sql/plan/utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,58 @@ func TestInitStageS3Param(t *testing.T) {
Id: 1000}
err = InitStageS3Param(param, s)
require.Nil(t, err)

param = &tree.ExternParam{
ExParamConst: tree.ExParamConst{
Option: []string{"format", "parquet"},
},
}
err = InitStageS3Param(param, s)
require.Nil(t, err)
require.Equal(t, tree.PARQUET, param.Format)

param = &tree.ExternParam{
ExParamConst: tree.ExParamConst{
Option: []string{"format", "badfmt"},
},
}
err = InitStageS3Param(param, s)
require.NotNil(t, err)
}

func TestInitS3ParamFormatValidation(t *testing.T) {
param := &tree.ExternParam{
ExParamConst: tree.ExParamConst{
Option: []string{
"endpoint", "oss-cn-hangzhou.aliyuncs.com",
"region", "cn-hangzhou",
"access_key_id", "ak",
"secret_access_key", "sk",
"bucket", "bkt",
"filepath", "sjrq=20200504/a.parquet",
"format", "parquet",
},
},
}
err := InitS3Param(param)
require.Nil(t, err)
require.Equal(t, tree.PARQUET, param.Format)

param = &tree.ExternParam{
ExParamConst: tree.ExParamConst{
Option: []string{
"endpoint", "oss-cn-hangzhou.aliyuncs.com",
"region", "cn-hangzhou",
"access_key_id", "ak",
"secret_access_key", "sk",
"bucket", "bkt",
"filepath", "sjrq=20200504/a.parquet",
"format", "badfmt",
},
},
}
err = InitS3Param(param)
require.NotNil(t, err)
}

func TestHandleOptimizerHints(t *testing.T) {
Expand Down
Loading