-
Notifications
You must be signed in to change notification settings - Fork 345
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.
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
[#3701]refactor(API): Refactoring SupportsSchemas.listSchemas() method to return String[] #3722
Conversation
Missing changes in Python client? |
The change in Python will be tracked with another issue: #3732 |
Overall LGTM |
Arrays.stream(nameIdentifiers).map(NameIdentifier::name).collect(Collectors.toSet()); | ||
Assertions.assertTrue(schemaNames.contains(schemaName)); | ||
String[] schemaNames = schemas.listSchemas(); | ||
Assertions.assertTrue(Arrays.asList(schemaNames).contains(schemaName)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can check if you can use ArrayUtils.contains()
without introducing extra dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand; Arrays is the JDK utility class, while ArrayUtils is apache commons lang; As this is so simple a case, I don't think it is necessary to use ArrayUtils here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a personal preference and the current implementation looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, thanks~
@@ -81,7 +81,7 @@ public NameIdentifier[] listSchemas() throws NoSuchCatalogException { | |||
ErrorHandlers.schemaErrorHandler()); | |||
resp.validate(); | |||
|
|||
return resp.identifiers(); | |||
return Arrays.stream(resp.identifiers()).map(NameIdentifier::name).toArray(String[]::new); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, will the server side of this interface remain the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you respond to it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, server side won't change; The "SupportsSchemas" has two version, one is for client, the other is for server side. This change has already been made in the main branch.
LGTM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
… method to return String[] (apache#3722) ### What changes were proposed in this pull request? Currently, the SupportsSchemas.listSchemas() method returns an array of NameIdentifier to represents the schemas. Actually a String[] will be more clear and easier to use. ### Why are the changes needed? To make the API more clear to use. Fix: apache#3701 ### Does this PR introduce _any_ user-facing change? Almost not; Just return type changed, will be more simple. ### How was this patch tested? Yes, many test cases cover this API.
What changes were proposed in this pull request?
Currently, the SupportsSchemas.listSchemas() method returns an array of NameIdentifier to represents the schemas. Actually a String[] will be more clear and easier to use.
Why are the changes needed?
To make the API more clear to use.
Fix: #3701
Does this PR introduce any user-facing change?
Almost not; Just return type changed, will be more simple.
How was this patch tested?
Yes, many test cases cover this API.