Spark 3.5: Fix the setting of equalAuthorities in RemoveOrphanFilesProcedure #10334
Conversation
|
change LGTM, could you please add a test to |
|
@nastra Sure! The test case for equal authorities has been added to |
| @@ -647,19 +648,33 @@ public void testRemoveOrphanFilesProcedureWithPrefixMode() | |||
| "CALL %s.system.remove_orphan_files(" | |||
| + "table => '%s'," | |||
| + "equal_schemes => map('file1', 'file')," | |||
| + "equal_authorities => map('localhost', '%s')," | |||
There was a problem hiding this comment.
rather than modifying an existing test I think we should just have a simple & separate test method that reproduces the underlying issue
There was a problem hiding this comment.
Thanks for the suggestion, that makes sense. The test logic has been moved to a separate method testRemoveOrphanFilesProcedureWithEqualAuthorities().
|
|
||
| // Drop table in afterEach has purge and fails due to invalid scheme "file1" used in this test | ||
| // Dropping the table here | ||
| sql("DROP TABLE %s", tableName); |
There was a problem hiding this comment.
I don't think the comment applies here, so we can remove dropping the table here, since it will be dropped in afterEach
There was a problem hiding this comment.
Thanks for pointing out that. Sorry for overlook the incorrect comment here, but I notice that it has the same problem when executing drop table purge in afterEach, since the file path is not correct. So I fix the comment and keep the dropping table statement here, do you think this is suitable?
There was a problem hiding this comment.
I haven't seen any issue when running the full test class locally, so I'm surprised it failed for you. Could you please remove this drop and we'll see if it fails/passes on CI?
There was a problem hiding this comment.
It doesn't fail, but print a lot of error log, the same as test method testRemoveOrphanFilesProcedureWithPrefixMode()
There was a problem hiding this comment.
ok thanks for clarifying. It seems testRemoveOrphanFilesProcedureWithPrefixMode() also doesn't explicitly fail but logs a lot of errors in the log when the table isn't dropped within the method itself. That being said, it's ok to keep the DROP here
|
@hantangwangd could you also please backport this to Spark 3.3 / 3.4? Opening one PR where this is fixed for 3.3 + 3.4 should be fine. Thanks a lot |
|
@nastra No problem, my pleasure! |
This PR correct the setting of procedure parameter
equalAuthoritiesforRemoveOrphanFilesProcedure. Previously it seems to be wrongly set.