-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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
[FLINK-34915][table] Complete DESCRIBE CATALOG
syntax
#24630
Conversation
a114624
to
fc23b38
Compare
fc23b38
to
deecb23
Compare
@LadyForest Hi, Could you please take a review? thanks! |
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.
Hi @liyubin117 thanks for the contribution! I left two comments, PTAL
...table-api-java/src/main/java/org/apache/flink/table/operations/DescribeCatalogOperation.java
Outdated
Show resolved
Hide resolved
...table-api-java/src/main/java/org/apache/flink/table/operations/DescribeCatalogOperation.java
Outdated
Show resolved
Hide resolved
8f7c1ae
to
82e0278
Compare
@flinkbot run azure |
@LadyForest Hi, I have updated as you said, besides, rebased on the latest master branch and CI passed, PTAL, thanks :) |
b0d376c
to
0945797
Compare
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.
Thanks for the update! I left some comments, PTAL
...table-api-java/src/main/java/org/apache/flink/table/operations/DescribeCatalogOperation.java
Outdated
Show resolved
Hide resolved
...table-api-java/src/main/java/org/apache/flink/table/operations/DescribeCatalogOperation.java
Outdated
Show resolved
Hide resolved
...table-api-java/src/main/java/org/apache/flink/table/operations/DescribeCatalogOperation.java
Outdated
Show resolved
Hide resolved
Arrays.asList("comment", "") // TODO: retain for future needs | ||
)); | ||
if (isExtended) { | ||
properties.forEach((key, value) -> rows.add(Arrays.asList("option:" + key, value))); |
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 might be a little bit verbose
desc catalog extended cat2;
+-------------------------+-------------------+
| info name | info value |
+-------------------------+-------------------+
| name | cat2 |
| type | generic_in_memory |
| comment | |
| option:default-database | db |
| option:type | generic_in_memory |
+-------------------------+-------------------+
5 rows in set
!ok
what about
desc catalog extended cat2;
+-------------------------+-------------------+
| info name | info value |
+-------------------------+-------------------+
| name | cat2 |
| type | generic_in_memory |
| comment | |
| option:default-database | db |
+-------------------------+-------------------+
4 rows in set
!ok
or
desc catalog extended cat2;
+-------------------------+-------------------+
| info name | info value |
+-------------------------+-------------------+
| name | cat2 |
| comment | |
| option:default-database | db |
| option:type | generic_in_memory |
+-------------------------+-------------------+
4 rows in set
!ok
if (isExtended) {
properties.entrySet().stream()
.filter(entry -> !"type".equals(entry.getKey()))
.sorted(Map.Entry.comparingByKey())
.forEach(
entry ->
rows.add(
Arrays.asList(
String.format("option:%s", entry.getKey()),
entry.getValue())));
}
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 have updated as the first output style, and will raise a discussion in dev mail. thanks for your suggesion~
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.
...table-api-java/src/main/java/org/apache/flink/table/operations/DescribeCatalogOperation.java
Outdated
Show resolved
Hide resolved
.../src/test/java/org/apache/flink/table/planner/operations/SqlOtherOperationConverterTest.java
Outdated
Show resolved
Hide resolved
0945797
to
9d39e32
Compare
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.
Hi, @liyubin117, thanks for your update. I only left one comment on the doc structure and added a commit to simplify it. PTAL
7ca1f10
to
010f3f8
Compare
@flinkbot run azure |
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.
Thanks for the update. LGTM
What is the purpose of the change
Describe the metadata of an existing catalog. The metadata information includes the catalog’s name, type, and comment. If the optional EXTENDED option is specified, catalog properties are also returned.
Brief change log
Verifying this change
This change added tests and can be verified as follows:
flink-table/flink-sql-client/src/test/resources/sql/catalog_database.q
flink-table/flink-sql-gateway/src/test/resources/sql/catalog_database.q
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation