Skip to content

Conversation

@shivusondur
Copy link
Contributor

@shivusondur shivusondur commented Aug 30, 2019

What changes were proposed in this pull request?

Added the document reference for SHOW TABLE EXTENDED sql command

Why are the changes needed?

For User reference

Does this PR introduce any user-facing change?

yes, it provides document reference for SHOW TABLE EXTENDED sql command

How was this patch tested?

verified in snap

Attached the Snap

image
image
image
image

@shivusondur
Copy link
Contributor Author

@dilipbiswal @gatorsmile
plz review

@dilipbiswal
Copy link
Contributor

@gatorsmile Can you please look at the attached screen shot ? The output kind of scrolls and does not look as pretty ? Any advice on how we should display this ?

Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

(I'm not concerned about the long table display)

Copy link
Member

Choose a reason for hiding this comment

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

Back-tick quote SHOW TABLE EXTENDED?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

"for the table name"
"is present. It will output"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

file-system-specific?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

Is it a full regex? we should say a little more about what's allowed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated the details

@srowen
Copy link
Member

srowen commented Oct 21, 2019

@shivusondur are you able to update?

Copy link
Contributor Author

@shivusondur shivusondur left a comment

Choose a reason for hiding this comment

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

@srowen
I handled all your comments, please check

`Created By`, `Type`, `Provider`, `Table Properties`, `Location`, `Serde Library`, `InputFormat`,
`OutputFormat`, `Storage Properties`, `Partition Provider`, `Partition Columns` and `Schema`.

Users cannot use regular expression for the table name if a partition specification is present. It will output
Copy link
Member

Choose a reason for hiding this comment

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

Last nit: a regular expression. But how about:
"If a partition specification is present, it outputs the given partition's ... Note that a table regex cannot be used with a partition specification."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@srowen
If the PARTITION key present in the query, the pattern mentioned in LIKE will not be treated as a regex. It will be treated as normal name.

for example. If we mention the regex with LIKE and PARTITION keyword, it fails like below.
image

But if mention the full name like below it works
image

Is it better to mention this exception scenario in the examples section?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think that agrees with the text I suggested? there's no problem documenting this in the docs here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@srowen
Documented Exception scenario.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but I don't think you changed this sentence. I'm suggesting a typo fix and rewrite right here.

Copy link
Member

Choose a reason for hiding this comment

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

@shivusondur still at least one more change here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@srowen
Sorry for late, I updated it according to your suggestion.

@shivusondur shivusondur requested a review from srowen November 4, 2019 17:18
@SparkQA
Copy link

SparkQA commented Nov 4, 2019

Test build #4915 has finished for PR 25632 at commit 79db8ca.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@srowen
Copy link
Member

srowen commented Nov 4, 2019

Merged to master

@srowen srowen closed this in eee45f8 Nov 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants