Skip to content

Spark 3.5: Add Parallelism Parameter Validation to AddFilesProcedure. - #12784

Merged
nastra merged 4 commits into
apache:mainfrom
slfan1989:add_parallelism_validation
Apr 22, 2025
Merged

nastra merged 4 commits into
apache:mainfrom
slfan1989:add_parallelism_validation

Conversation

@slfan1989

Copy link
Copy Markdown
Contributor

Spark 3.5: Add Parallelism Parameter Validation to AddFilesProcedure.

@github-actions github-actions Bot added the spark label Apr 12, 2025
() ->
sql(
"CALL %s.system.add_files(table => '%s', source_table => '%s', parallelism => %d)",
catalogName, tableName, sourceTableName, -1))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: We can inline -1.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your suggestions. I have already made the changes to the code.

slfan1989 and others added 3 commits April 14, 2025 11:19
@slfan1989

Copy link
Copy Markdown
Contributor Author

@manuzhang @ebyhr Could you help review this PR? Thank you very much!

assertThatThrownBy(
() ->
sql(
"CALL %s.system.add_files(table => '%s', source_table => '%s', parallelism => -1)",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: Testing with the boundary value 0 is more natural. I know testMigrateWithInvalidParallelism uses -1, though.

@slfan1989

Copy link
Copy Markdown
Contributor Author

@manuzhang Thank you for the review and approval! If there are no further issues, could you please merge this PR?

@manuzhang

Copy link
Copy Markdown
Member

@slfan1989 It requires an approval from a committer.

@slfan1989

Copy link
Copy Markdown
Contributor Author

@slfan1989 It requires an approval from a committer.

@manuzhang Thank you for your message! @nastra Could you take a look at this PR? Thank you very much!

@nastra
nastra merged commit 04bb3ee into apache:main Apr 22, 2025
@slfan1989

Copy link
Copy Markdown
Contributor Author

@nastra @manuzhang @ebyhr Thank you very much for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants