Skip to content

Conversation

@zhangbutao
Copy link
Contributor

What changes were proposed in this pull request?

Why are the changes needed?

Please see HIVE-29213 & and HIVE-29296 to check the details.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Local Beeline test.

if (row.isMeta) {
v = beeLine.getColorBuffer().center(row.values[i], row.sizes[i]);
if (beeLine.getOpts().getColor() && rows.isPrimaryKeyCol(i)) {
buf.cyan(v.getMono());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Retrieving the primary key in Beeline is just for coloring the output. Removing this code blocks is safe.

@sonarqubecloud
Copy link

@deniskuzZ
Copy link
Member

why not simply revert JIRA that introduced PK fetching?

@zhangbutao
Copy link
Contributor Author

why not simply revert JIRA that introduced PK fetching?

The code for PK in Beeline was introduced during the implementation of HS2, rather than being implemented through a separate JIRA. See 96083e7

Copy link
Member

@deniskuzZ deniskuzZ left a comment

Choose a reason for hiding this comment

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

+1

@InvisibleProgrammer
Copy link
Contributor

According to my memory, removing this part of the code means the coloring output only means that the header has different font type than the result set data.
What about removing the coloring functionality at all? With this change Beeline still accepts the color parameter. But what is the effect of that parameter?

@zhangbutao
Copy link
Contributor Author

According to my memory, removing this part of the code means the coloring output only means that the header has different font type than the result set data.

As I mentioned, this part of the code relates to the primary key and has never taken effect in Hive beeline, so I'm not very clear about the color or font of its output display.

What about removing the coloring functionality at all? With this change Beeline still accepts the color parameter. But what is the effect of that parameter?

Regarding whether to remove the entire coloring functionality code, I'm not entirely sure if there are real-world users who actually use the color option. However, through a Google search for the keyword beeline --color, I found many related articles. Therefore, I assume that the existence of the --color option is meaningful, and there is no need to completely remove the coloring functionality at this point.

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