Skip to content

Conversation

@JingGe
Copy link
Contributor

@JingGe JingGe commented Nov 14, 2022

What is the purpose of the change

Currently, the default value 30 of DEFAULT_MAX_COLUMN_WIDTH in PrintStyle has been used when table.execute().print() has been called. There is no way to adjust it if user wants to print more content of some columns.

Users want to configure the max display column width in a unified way while:

using CLI
Using Table API
Running job in batch execution mode
Running job in streaming execution mode

For further information, please refer to https://cwiki.apache.org/confluence/display/FLINK/FLIP-279+Unified+the+max+display+column+width+for+SqlClient+and+Table+APi+in+both+Streaming+and+Batch+execMode

Brief change log

  • Add new TableConfigOption.DISPLAY_MAX_COLUMN_WIDTH with default value 30
  • Use the config value while executeQueryOperation
  • TableConfigOption.DISPLAY_MAX_COLUMN_WIDTH will be the fallback key of SqlClientOptions.DISPLAY_MAX_COLUMN_WIDTH
  • SqlClientOptions.DISPLAY_MAX_COLUMN_WIDTH will used both in batch and in stream mode
  • deprecate SqlClientOptions.DISPLAY_MAX_COLUMN_WIDTH
  • update test

Verifying this change

This change added tests and can be verified by executing CliTableauResultViewTest and DisplayMaxColumnWidthSQLExampleTCase.

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) Configurations are changed.
  • 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, Kubernetes/Yarn, 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)

@flinkbot
Copy link
Collaborator

flinkbot commented Nov 14, 2022

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@JingGe JingGe changed the title [FLINK-30025][table] the max column width is resizable while printing the query result to the client console. [FLINK-30025][table] the max column width should be resizable while printing the query result to the client console. Nov 14, 2022
@JingGe JingGe force-pushed the hotfix-table-print-max-col-length branch 3 times, most recently from d6777b1 to 8222ff4 Compare November 15, 2022 21:38
@JingGe
Copy link
Contributor Author

JingGe commented Nov 15, 2022

@flinkbot run azure

1 similar comment
@JingGe
Copy link
Contributor Author

JingGe commented Nov 15, 2022

@flinkbot run azure

Copy link
Member

@fsk119 fsk119 left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. I left some comments.

@JingGe JingGe force-pushed the hotfix-table-print-max-col-length branch from 8222ff4 to adb3568 Compare January 10, 2023 22:54
@JingGe JingGe force-pushed the hotfix-table-print-max-col-length branch 4 times, most recently from d0acd8b to 733eb0c Compare January 20, 2023 03:36
@JingGe JingGe changed the title [FLINK-30025][table] the max column width should be resizable while printing the query result to the client console. [FLINK-30025][table] Unified the max display column width for SqlClient and Table APi in both Streaming and Batch execMode Jan 20, 2023
@JingGe JingGe force-pushed the hotfix-table-print-max-col-length branch 4 times, most recently from 6c2fe89 to 97b3b75 Compare January 20, 2023 16:54
@JingGe
Copy link
Contributor Author

JingGe commented Jan 27, 2023

@fsk119 @wuchong would you like to review this PR?

Copy link
Member

@wuchong wuchong left a comment

Choose a reason for hiding this comment

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

I think the change didn't verify impacts to SQL CLI.

Could you add tests in flink-table/flink-sql-client/src/test/resources/sql/select.q to
(1) set table.print.max-column-width=40 should also work
(2) truncating should also work on batch mode.

And please also add tests in flink-table/flink-sql-client/src/test/resources/sql/table.q to verify truncating doesn't affect show create table with table names longer than 30 characters.

@JingGe JingGe force-pushed the hotfix-table-print-max-col-length branch 2 times, most recently from 8f852b0 to 690f194 Compare January 30, 2023 17:54
@JingGe
Copy link
Contributor Author

JingGe commented Jan 30, 2023

All tests have been added.

@JingGe JingGe force-pushed the hotfix-table-print-max-col-length branch from 690f194 to 2d4e0a0 Compare January 30, 2023 23:57
Copy link
Member

@wuchong wuchong left a comment

Choose a reason for hiding this comment

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

Please append fixup commits to address comments. Please DO NOT squash or amend commits. Otherwise, the reviewer doesn't know what was updated and has to review the whole pull request again. You can squash or modify commits after the review pass.

Copy link
Member

@wuchong wuchong left a comment

Choose a reason for hiding this comment

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

LGMT. I only left some minor comments.
You can rebase and restructure your commits now.

@JingGe JingGe force-pushed the hotfix-table-print-max-col-length branch from 8ae5474 to 9cc74e1 Compare February 1, 2023 18:52
@wuchong
Copy link
Member

wuchong commented Feb 2, 2023

Waiting for branch cut.

@JingGe JingGe force-pushed the hotfix-table-print-max-col-length branch from 9cc74e1 to defdde0 Compare February 2, 2023 09:30
@JingGe JingGe force-pushed the hotfix-table-print-max-col-length branch 3 times, most recently from c5581cb to 8fcdbe5 Compare February 5, 2023 15:13
@JingGe
Copy link
Contributor Author

JingGe commented Feb 5, 2023

@flinkbot run azure

1 similar comment
@JingGe
Copy link
Contributor Author

JingGe commented Feb 5, 2023

@flinkbot run azure

@JingGe
Copy link
Contributor Author

JingGe commented Feb 5, 2023

It's weird. Running the CliClientITCase locally in IDE or via maven has no failures. It seems CI has some special setting that the result of explain changelog_mode, estimated_cost, plan_advice, json_execution_plan select user, product from orders; contains smaller or bigger nodes' id, 1 digit numbers instead of 2. Therefore the calculated print width of the border lines are different.

@JingGe
Copy link
Contributor Author

JingGe commented Feb 6, 2023

@flinkbot run azure

@JingGe JingGe force-pushed the hotfix-table-print-max-col-length branch 3 times, most recently from 1ae8cf1 to 7c640ff Compare February 7, 2023 02:28
@JingGe
Copy link
Contributor Author

JingGe commented Feb 13, 2023

Found reason that node ids are two digit numbers running locally but are three digit numbers in the CI env which make the header and footer of the printing result have different lengths.

@JingGe
Copy link
Contributor Author

JingGe commented Feb 14, 2023

@flinkbot run azure

@JingGe JingGe force-pushed the hotfix-table-print-max-col-length branch from 84621c4 to 91eaae2 Compare February 14, 2023 06:28
@JingGe JingGe force-pushed the hotfix-table-print-max-col-length branch from 91eaae2 to 8b2d903 Compare February 14, 2023 15:46
…de ids have been removed because the ids might have different length in different envs.
@JingGe JingGe force-pushed the hotfix-table-print-max-col-length branch from 8b2d903 to 8c6a9ab Compare February 14, 2023 15:47
@JingGe JingGe merged commit c03efae into apache:master Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants