[FLINK-36050][table] Support enhanced `SHOW VIEW` syntax by snuyanzin · Pull Request #25200 · apache/flink

snuyanzin · 2024-08-14T13:35:46Z

What is the purpose of the change

Support enhanced SHOW VIEWS syntax like SHOW VIEWS FROM catalog1.db1 LIKE 't%'

in a way similar to existing SHOW TABLES syntax

Brief change log

parser was extended

Verifying this change

catalog_database.q in sql client and sql gateway

a number of unit tests

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: (yes)

Documentation

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

flinkbot · 2024-08-14T13:38:02Z

CI report:

ae58dc3 Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

snuyanzin · 2024-08-14T13:41:56Z

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


-    public String[] fullDatabaseName() {
-        return Objects.isNull(this.databaseName)
-                ? new String[] {}


probably it's better to reuse lists rather than create arrays per call

jnh5y · 2024-08-14T16:36:34Z

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

+
+The syntax of sql pattern in `LIKE` clause is the same as that of `MySQL` dialect.
+* `%` matches any number of characters, even zero characters, `\%` matches one `%` character.
+* `_` matches exactly one character, `\_` matches one `_` character.


Do we want to provide examples? Or is it similar enough to SHOW TABLES that it'd be redundant?

Or is it similar enough to SHOW TABLES that it'd be redundant?

yep, I had exactly this thought, may be we can explicitly say that SHOW TABLES examples could be also used as examples for SHOW VIEWS

Yeah, maybe that is obvious? Not sure how pedantic to be in docs.

jnh5y · 2024-08-14T16:52:54Z

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

+            final String prefix = notLike ? "NOT " : "";
+            builder.append(String.format(" %sLIKE '%s'", prefix, likePattern));
+        }
        return "SHOW VIEWS";


Ok, I think you want to return builder.build() instead of just SHOW VIEWS.

Is there a test that can be added to catch this?

great catch, thanks

added extra tests covering this for both table and views

jnh5y · 2024-08-14T16:57:07Z

From a read through, just found the one issue and asked the one question. Overall, this looks good and is consistent with #18361

jnh5y · 2024-08-14T21:03:47Z

.../src/test/java/org/apache/flink/table/planner/operations/SqlOtherOperationConverterTest.java

        Operation operation = parse(sql);
        assertThat(operation).isInstanceOf(ShowViewsOperation.class);

-        ShowViewsOperation showTablesOperation = (ShowViewsOperation) operation;


Nice! I missed this typo... Very natural given that one should be using the other PR to make this one!

jnh5y

LGTM!

twalthr

There are several issues with the SHOW operations and a lot of duplicate code. I suggest to:

In CatalogManager make: public @Nullable String getCurrentDatabase()
In CatalogManager make: public @Nullable String getCurrentCatalog()
In CatalogManaged implement: public String qualifyCatalog(@Nullable String catalogName) throwing an error like in qualifyIdentifier.
In CatalogManager implement: public String qualifyDatabase(String catalogName, @Nullable String databaseName). Throwing an error like in qualifyIdentifier.

Implement an AbstractShowOperation class covering Tables, Views, Procedures, (Models in the future)

Signed-off-by: snuyanzin <snuyanzin@gmail.com>

twalthr

Thank you @snuyanzin. This is a great refactoring! I left some last comments but in general the PR looks very good and is well tested.

...-table/flink-table-api-java/src/main/java/org/apache/flink/table/catalog/CatalogManager.java

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

twalthr · 2024-08-19T09:58:46Z

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

-                                .getCatalogOrError(catalogManager.getCurrentCatalog())
-                                .listProcedures(catalogManager.getCurrentDatabase());
+                return catalogManager
+                        .getCatalogOrError(catalogManager.getCurrentCatalog())


calls to getCurrentCatalog() and getCurrentDatabase() should not be required anymore?

here it is a special case for these SHOW operations.
We have here preposition == null which means that we don't have IN or FROM in our SHOW query, so no catalog/database specified. In this case we assume that this SHOW operation should be done against current catalog/database

twalthr · 2024-08-19T10:02:34Z

...main/java/org/apache/flink/table/planner/operations/converters/AbstractSqlShowConverter.java

+        CatalogManager catalogManager = context.getCatalogManager();
+        String catalogName =
+                fullDatabaseName.size() == 1
+                        ? catalogManager.getCurrentCatalog()


here null could enter the flow again. and getOperation is not marked as @Nullable in parameters

yep, depending on case catalog name and database name could be null, marked @Nullable in getOperation

example of such case

SHOW TABLES

here neither catalog nor database is specified and both will be null, then in Operation there will be getCurrentCatalog()/getCurrentDatabase() used

...rc/main/java/org/apache/flink/table/planner/operations/converters/SqlShowViewsConverter.java

twalthr · 2024-08-19T12:02:25Z

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

                // it's to show current_catalog.current_database
                return catalogManager
-                        .getCatalogOrError(catalogManager.getCurrentCatalog())
+                        .getCatalogOrThrowException(catalogManager.getCurrentCatalog())


there should be no call in this class to catalogManager.getCurrentCatalog(). Maybe I'm missing some context?

here it is a case with preposition == null
i.e. when no catalog or database was specified e.g.

SHOW PROCEDURES

in this case we need to use current catalog and database.

In theory we could precalculate it on converters level and then here use only catalogName, databaseName

updated, now there is no getCurrentCatalog/getCurrentDatabase calls in operations

twalthr

Thank you @snuyanzin. Should be good in the next iteration.

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

twalthr

LGTM. Thanks for the refactoring!

[FLINK-36050][table] Enhance SHOW VIEW syntax

2af84b0

snuyanzin commented Aug 14, 2024

View reviewed changes

flinkbot added the component=TableSQL/API label Aug 14, 2024

jnh5y reviewed Aug 14, 2024

View reviewed changes

snuyanzin added 2 commits August 14, 2024 20:46

Address feedback

83f873a

Address feedback

be93cf6

jnh5y reviewed Aug 14, 2024

View reviewed changes

jnh5y approved these changes Aug 14, 2024

View reviewed changes

twalthr reviewed Aug 15, 2024

View reviewed changes

[FLINK-36085][table] Refactor SHOW operations with LIKE/ILIKE

8b0fb64

Signed-off-by: snuyanzin <snuyanzin@gmail.com>

snuyanzin force-pushed the flink36050 branch from 6dc04c1 to 8b0fb64 Compare August 18, 2024 23:43

twalthr reviewed Aug 19, 2024

View reviewed changes

Address feedback

924e182

twalthr reviewed Aug 19, 2024

View reviewed changes

snuyanzin added 4 commits August 19, 2024 15:27

Fix the build

56aa3d4

Address feedback

342c290

Address feedback

fac9cfd

Add missing return and fix test

07f97e0

twalthr reviewed Aug 20, 2024

View reviewed changes

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

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

Address feedback

ae58dc3

twalthr approved these changes Aug 20, 2024

View reviewed changes

snuyanzin closed this in 36347e5 Aug 20, 2024

snuyanzin deleted the flink36050 branch July 29, 2025 08:02

Conversation

snuyanzin commented Aug 14, 2024

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

Uh oh!

flinkbot commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI report:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnh5y commented Aug 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnh5y left a comment

Choose a reason for hiding this comment

Uh oh!

twalthr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

twalthr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

twalthr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

twalthr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

flinkbot commented Aug 14, 2024 •

edited

Loading

twalthr left a comment •

edited

Loading