Skip to content
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-10229] [sql-client] Support listing of views #6631

Closed
wants to merge 1 commit into from

Conversation

yanghua
Copy link
Contributor

@yanghua yanghua commented Aug 29, 2018

What is the purpose of the change

This pull request support listing of views

Brief change log

  • Support sql SHOW VIEWS

Verifying this change

This change added tests and can be verified as follows:

  • SqlCommandParserTest#testCommands to test command parsing
  • LocalExecutorITCase#testListViews

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

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

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

@yanghua
Copy link
Contributor Author

yanghua commented Aug 30, 2018

cc @fhueske and @twalthr

@yanghua
Copy link
Contributor Author

yanghua commented Aug 30, 2018

@walterddr and @hequn8128 Can you help me to review this PR? thanks.

@hequn8128
Copy link
Contributor

@yanghua Thanks for your PR. I will take a look this weekend. :-)

@twalthr twalthr changed the title [FLINK-10229][SQL CLIENT] Support listing of views [FLINK-10229] [sql-client] Support listing of views Aug 31, 2018
Copy link
Contributor

@hequn8128 hequn8128 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yanghua I left some suggestions, please take a look. Suggestions mainly include:

  • Correct documents about SHOW VIEWS.
  • Sort view names in alphabetical order.
  • Add list views in Executor and refactor test cases.

Best, Hequn

@@ -503,6 +503,12 @@ Views created within a CLI session can also be removed again using the `DROP VIE
DROP VIEW MyNewView
{% endhighlight %}

Views created within a CLI session can also be shown again using the `SHOW VIEWS` statement:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Views created in a session environment file can also be listed, so change the description to:
Displays all of the views in the current session using the `SHOW VIEWS` statement:

@@ -332,6 +336,17 @@ private void callShowTables() {
terminal.flush();
}

private void callShowViews() {
final Set<String> views = context.getViews().keySet();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Get views from Executor like listTables so that to provide listViews for external systems and gateway mode.

if (views.isEmpty()) {
terminal.writer().println(CliStrings.messageInfo(CliStrings.MESSAGE_EMPTY).toAnsi());
} else {
views.forEach((v) -> terminal.writer().println(v));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it better to sort views in alphabetical order(also for show functions)? Show tables did in this way as table names stored in a TreeSet in table environment.
What do you think? @twalthr @yanghua


executor.validateSession(session);

assertEquals(session.getViews().size(), 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use executor to test list views(similar to testListTables and testListUserDefinedFunctions). There are 2 views in the default environment. Executor will merge environments.

@yanghua
Copy link
Contributor Author

yanghua commented Sep 4, 2018

@hequn8128 thanks for your suggestion, I have refactored this PR, can you review it again?

@yanghua
Copy link
Contributor Author

yanghua commented Sep 6, 2018

@twalthr Would you like to review this PR? thanks~

@@ -587,6 +587,18 @@ abstract class TableEnvironment(val config: TableConfig) {
rootSchema.getTableNames.asScala.toArray
}

def listViews(): Array[String] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can't add this function in TableEnvironment. Reasons are:

  • table.isInstanceOf[RelTable] is not necessary a View. RelTable can be registered through registerTable().
  • TableEnvironment currently doesn't support view. It is only supported in SQL-Client now.

As a choice, you can list views directly from the session passed to the listViews(). Also pay attention to the order.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean that I use the original implementation directly, just move it to the Executor? I used to get from the session, I misunderstood what you mean, I thought it was to refer to the implementation of the listTable method.


final List<String> actualViews = executor.listViews(session);

final List<String> expectedViews = Arrays.asList(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add some other views and test the order.

@yanghua
Copy link
Contributor Author

yanghua commented Sep 8, 2018

@hequn8128 What about this change?

@hequn8128
Copy link
Contributor

Hi @yanghua , thanks for your update. The PR looks good. +1 to merge from my side.
Best, Hequn

@yanghua
Copy link
Contributor Author

yanghua commented Sep 11, 2018

hi @twalthr , hequn helped me to review this PR, does it look good to you?

@yanghua
Copy link
Contributor Author

yanghua commented Sep 17, 2018

hi @fhueske can you review this PR? Thanks.

@yanghua
Copy link
Contributor Author

yanghua commented Sep 26, 2018

hi @twalthr , hequn has reviewed this PR, can you make a deterministic review?

@twalthr
Copy link
Contributor

twalthr commented Oct 1, 2018

@yanghua Sorry, for the delay. I will take a look at it soon. I want to think about our general design for views before.

@yanghua yanghua force-pushed the FLINK-10229 branch 4 times, most recently from f878230 to 5d9ba4d Compare March 11, 2019 11:48
@yanghua
Copy link
Contributor Author

yanghua commented Mar 12, 2019

Have fixed some conflicts and rebased this PR. It has stayed for more than half a year, hope for reviewing. cc @twalthr

@yanghua
Copy link
Contributor Author

yanghua commented May 7, 2020

leaving...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants