-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[Core] remove duplicated schema names in list schema in JDBC catalog #4440
Conversation
@@ -155,7 +156,8 @@ public List<String> listDatabases() { | |||
row -> row.getString(JdbcUtils.DATABASE_NAME), | |||
JdbcUtils.LIST_ALL_PROPERTY_DATABASES_SQL, | |||
catalogKey)); | |||
return databases; | |||
|
|||
return databases.stream().distinct().collect(Collectors.toList()); |
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 am curious what the root cause of this is.
Why does databases contain the duplicate - is it ever valid?
Are there other implications to leaving this list with duplicates?
Would it not be better to fix this when the duplicate was added?
Also it would be good to add a junit.
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.
+1 curious
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.
There are two tables in JDBC catalog, paimon_database_properties
contains database properties, paimon_tables
contains table , schema, catalog relations. The same database will present in these two tables if creating a database and tables because Paimon will automatically add some properties when creating schema.
@davidradl do you have any suggestion where to place the test?
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 add a test case?
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.
sure
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.
@JingsongLi @davidradl add a test, please help to review again.
+1 |
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.
+1
Purpose
fixes: #4439