[FLINK-31481][table] Support enhanced show databases syntax #23612

jeyhunkarimov · 2023-10-28T21:32:03Z

What is the purpose of the change

Support enhanced show functions syntax described in FLIP-297

Brief change log

Changelog from:

SHOW DATABASES

to:

SHOW DATABASES [ ( FROM | IN ) catalog_name] [ [NOT] (LIKE | ILIKE) <sql_like_pattern> ]

Verifying this change

Extended integration tests flink-table/flink-sql-client/src/test/resources/sql/catalog_database.q
Extended unit test flink-table/flink-sql-parser/src/test/java/org/apache/flink/sql/parser/FlinkSqlParserImplTest.java -> testShowDataBases()
Added unit tests in flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/operations/SqlOtherOperationConverterTest.java -> testShowDatabases()

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): no
The public API, i.e., is any changed class annotated with @Public(Evolving): no
The serializers: no
The runtime per-record code paths (performance sensitive): no
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
The S3 file system connector: no

Documentation

Does this pull request introduce a new feature? yes
If yes, how is the feature documented? docs

flinkbot · 2023-10-28T21:37:38Z

CI report:

29cb2f6 Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

jeyhunkarimov · 2023-11-06T08:49:45Z

Hi @MartijnVisser could you please check this PR?

jeyhunkarimov · 2023-11-20T13:45:19Z

Hi @jnh5y @dawidwys , could you guys check this PR?

...k-table-api-java/src/main/java/org/apache/flink/table/operations/ShowDatabasesOperation.java

...ain/java/org/apache/flink/table/planner/operations/converters/SqlShowDatabasesConverter.java

...k-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/dql/SqlShowDatabases.java

jnh5y · 2023-11-27T19:16:51Z

This is looking pretty good. Since implementing FLIP-297 is assigned to @chucheng92, we should get his input as well!

@jeyhunkarimov have you signed up for the Flink JIRA? You may need to request access on the Flink dev list. (I tried to find instructions, but I am not seeing them immediately.)

chucheng92 · 2023-11-28T02:25:23Z

This is looking pretty good. Since implementing FLIP-297 is assigned to @chucheng92, we should get his input as well!

@jeyhunkarimov have you signed up for the Flink JIRA? You may need to request access on the Flink dev list. (I tried to find instructions, but I am not seeing them immediately.)

@jeyhunkarimov good work! Currently i'm busy with other work, feel free to assign to yourself.

jeyhunkarimov · 2023-11-29T11:02:40Z

Thanks for the comment @chucheng92 and thank you for the review @jnh5y .

I addressed your comments. Could you please check if you have any other comments?

P.s I have jira account (jID: jeyhunkarimov). I can assign tasks to myself in other apache projects (e.g., kafka). However, in Flink this is not possible. I will ask for the required permissions via dev mailing list.

jeyhunkarimov · 2023-12-18T09:29:34Z

Hi @jnh5y could you please check if your comments are addressed? Thanks!

jnh5y · 2023-12-18T13:42:10Z

...k-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/dql/SqlShowDatabases.java

+        if (this.catalogName != null && this.catalogName.names.size() > 1) {
+            throw new ParseException(
+                    String.format(
+                            "Show databases from/in identifier [ %s ] format error, catalog cannot contain dot character.",


As something that I'm unsure about... I wonder if a catalog name can contain a . if it quoted with backticks.

If so, then this statement is inaccurate.

I believe you can quote any character.

Given the responses, could you please update the exception to e.g.

"Show databases from/in identifier [ %s ] format error, catalog must be a single part identifier."

...k-table-api-java/src/main/java/org/apache/flink/table/operations/ShowDatabasesOperation.java

jnh5y · 2023-12-18T13:52:29Z

Hi @jnh5y could you please check if your comments are addressed? Thanks!

Hi @jeyhunkarimov, sorry to be slow to look again. Things are looking good. @dawidwys would you be willing to take a quick read through. I've left some comments where I think you could provide some insight about what needs to change for this work to be merged.

dawidwys

Thank you @jeyhunkarimov for the work! The PR looks really good! I put some small comments. I think we can get it in shortly!

docs/content.zh/docs/dev/table/sql/show.md

...k-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/dql/SqlShowDatabases.java

...k-table-api-java/src/main/java/org/apache/flink/table/operations/ShowDatabasesOperation.java

...ain/java/org/apache/flink/table/planner/operations/converters/SqlShowDatabasesConverter.java

jeyhunkarimov · 2024-01-09T05:13:25Z

@flinkbot run azure

dawidwys

The changes look good. There is one last comment to be addressed and I'll merge the PR. Thank you @jeyhunkarimov

dawidwys · 2024-01-12T09:32:46Z

...k-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/dql/SqlShowDatabases.java

+        if (this.catalogName != null && this.catalogName.names.size() > 1) {
+            throw new ParseException(
+                    String.format(
+                            "Show databases from/in identifier [ %s ] format error, catalog cannot contain dot character.",


Given the responses, could you please update the exception to e.g.

"Show databases from/in identifier [ %s ] format error, catalog must be a single part identifier."

dawidwys

lgtm

jnh5y reviewed Nov 27, 2023

View reviewed changes

...k-table-api-java/src/main/java/org/apache/flink/table/operations/ShowDatabasesOperation.java Outdated Show resolved Hide resolved

jnh5y reviewed Nov 27, 2023

View reviewed changes

...k-table-api-java/src/main/java/org/apache/flink/table/operations/ShowDatabasesOperation.java Outdated Show resolved Hide resolved

jnh5y reviewed Nov 27, 2023

View reviewed changes

...ain/java/org/apache/flink/table/planner/operations/converters/SqlShowDatabasesConverter.java Outdated Show resolved Hide resolved

jnh5y reviewed Nov 27, 2023

View reviewed changes

...k-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/dql/SqlShowDatabases.java Show resolved Hide resolved

jeyhunkarimov force-pushed the FLINK-31481 branch 2 times, most recently from ad52b5f to 78cafaf Compare November 29, 2023 06:26

jnh5y reviewed Dec 18, 2023

View reviewed changes

...k-table-api-java/src/main/java/org/apache/flink/table/operations/ShowDatabasesOperation.java Outdated Show resolved Hide resolved

dawidwys self-assigned this Dec 18, 2023

dawidwys reviewed Jan 8, 2024

View reviewed changes

jeyhunkarimov force-pushed the FLINK-31481 branch 2 times, most recently from 4e8b7e5 to 652a9cf Compare January 8, 2024 21:41

jeyhunkarimov force-pushed the FLINK-31481 branch 5 times, most recently from e889b90 to 633fd8d Compare January 11, 2024 17:11

dawidwys requested changes Jan 12, 2024

View reviewed changes

[FLINK-31481][table] Support enhanced show databases syntax

29cb2f6

jeyhunkarimov force-pushed the FLINK-31481 branch from 633fd8d to 29cb2f6 Compare January 12, 2024 10:41

dawidwys approved these changes Jan 15, 2024

View reviewed changes

dawidwys merged commit 6c050e9 into apache:master Jan 15, 2024

flinkbot added component=TableSQL/Planner component=TableSQL/API labels Apr 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-31481][table] Support enhanced show databases syntax #23612

[FLINK-31481][table] Support enhanced show databases syntax #23612

jeyhunkarimov commented Oct 28, 2023

flinkbot commented Oct 28, 2023 •

edited

jeyhunkarimov commented Nov 6, 2023

jeyhunkarimov commented Nov 20, 2023

jnh5y commented Nov 27, 2023

chucheng92 commented Nov 28, 2023

jeyhunkarimov commented Nov 29, 2023

jeyhunkarimov commented Dec 18, 2023

jnh5y Dec 18, 2023

dawidwys Jan 8, 2024

dawidwys Jan 12, 2024

jnh5y commented Dec 18, 2023

dawidwys left a comment

jeyhunkarimov commented Jan 9, 2024

dawidwys left a comment •

edited

dawidwys Jan 12, 2024

dawidwys left a comment

[FLINK-31481][table] Support enhanced show databases syntax #23612

[FLINK-31481][table] Support enhanced show databases syntax #23612

Conversation

jeyhunkarimov commented Oct 28, 2023

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

flinkbot commented Oct 28, 2023 • edited

CI report:

jeyhunkarimov commented Nov 6, 2023

jeyhunkarimov commented Nov 20, 2023

jnh5y commented Nov 27, 2023

chucheng92 commented Nov 28, 2023

jeyhunkarimov commented Nov 29, 2023

jeyhunkarimov commented Dec 18, 2023

jnh5y Dec 18, 2023

Choose a reason for hiding this comment

dawidwys Jan 8, 2024

Choose a reason for hiding this comment

dawidwys Jan 12, 2024

Choose a reason for hiding this comment

jnh5y commented Dec 18, 2023

dawidwys left a comment

Choose a reason for hiding this comment

jeyhunkarimov commented Jan 9, 2024

dawidwys left a comment • edited

Choose a reason for hiding this comment

dawidwys Jan 12, 2024

Choose a reason for hiding this comment

dawidwys left a comment

Choose a reason for hiding this comment

flinkbot commented Oct 28, 2023 •

edited

dawidwys left a comment •

edited