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

Document hive storage table properties #10817

Merged
merged 1 commit into from
Feb 24, 2022

Conversation

rosewms
Copy link
Contributor

@rosewms rosewms commented Jan 26, 2022

Issue: #5581

@mosabua
Copy link
Member

mosabua commented Jan 26, 2022

This is wrong. The properties are NOT catalog properties .. they are table properties ..

@mosabua mosabua removed the request for review from bitsondatadev January 26, 2022 22:05
@rosewms rosewms force-pushed the rw-hive-tbl-properties branch from 6b47b31 to 31eed1c Compare January 27, 2022 20:51
@rosewms rosewms requested a review from mosabua January 27, 2022 21:10
@mosabua
Copy link
Member

mosabua commented Jan 27, 2022

Pretty sure there are more table properties. Check in #8905 and maybe in the code and/or a running system

docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
@bitsondatadev
Copy link
Member

I wonder if it makes sense to move the table properties and "runtime" properties in general to a different page. If I were to place these anywhere it might be below the static config file properties. That seems really out of the way though and buried.

Just one thought, let's split config file props and runtime props on different pages.

@mosabua
Copy link
Member

mosabua commented Jan 28, 2022

While that is a feasible approach and the Hive connector page is in dire need of refactor and such .. I dont want to take this work into the scope of this PR. We just want to document the table properties, explain what they are and a bit how to use them

@rosewms rosewms force-pushed the rw-hive-tbl-properties branch from 31eed1c to 3ebc2b0 Compare January 28, 2022 21:57
@rosewms rosewms requested review from mosabua and hashhar January 31, 2022 18:16
@rosewms rosewms force-pushed the rw-hive-tbl-properties branch from 3ebc2b0 to 356041f Compare January 31, 2022 21:46
docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
@rosewms rosewms force-pushed the rw-hive-tbl-properties branch from 356041f to 793dee7 Compare February 4, 2022 17:30
@rosewms rosewms requested a review from hashhar February 4, 2022 17:48
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

LGTM % comments.

docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
@mosabua
Copy link
Member

mosabua commented Feb 7, 2022

@hashhar I reviewed this now and @rosewms will address various comments.

However I have a question left ... in this PR we document table properties ... do we need another section for schema properties? Or are they the same? If they are the same then we should add that they are also honoured for CREATE SCHEMA WITH .. and if not .. we should document those next..

@hashhar
Copy link
Member

hashhar commented Feb 8, 2022

Schema properties are separate. See https://trino.io/docs/current/sql/create-schema.html#description for how to find them.

docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
@rosewms rosewms force-pushed the rw-hive-tbl-properties branch from 793dee7 to ed305c4 Compare February 11, 2022 00:03
@rosewms rosewms force-pushed the rw-hive-tbl-properties branch from ed305c4 to 4076810 Compare February 15, 2022 01:00
@rosewms rosewms requested a review from mosabua February 15, 2022 01:04
@rosewms rosewms force-pushed the rw-hive-tbl-properties branch from 4076810 to 8af411d Compare February 15, 2022 19:59
docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
@rosewms rosewms force-pushed the rw-hive-tbl-properties branch 2 times, most recently from 0963480 to bbc246f Compare February 17, 2022 19:54
@rosewms rosewms requested review from losipiuk and findepi February 17, 2022 20:02
@rosewms rosewms removed the WIP label Feb 18, 2022
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

Looks good to go % comments.

docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
Comment on lines +1050 to +1051
- The serialization format for ``NULL`` value. Requires TextFile, RCText,
or SequenceFile format.
Copy link
Member

Choose a reason for hiding this comment

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

Just for information - null_format can be set to a string that is then interpreted as a marker for NULL values in table files.

e.g. with null_format = 'NULL_VALUE_MARKER' and a TextFile table with some line like a\u0001NULL_VALUE_MARKER\u0001null value would get read as 3 columns a, NULL and null value.

If we want to document this it should be a separate effort - not part of this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mosabua how should I proceed here?

Copy link
Member

Choose a reason for hiding this comment

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

We should create an issue to follow up IMO?

docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
@rosewms rosewms requested a review from Ordinant February 22, 2022 20:21
@rosewms rosewms force-pushed the rw-hive-tbl-properties branch from bbc246f to d69e2c1 Compare February 23, 2022 18:37
@hashhar hashhar merged commit 26c8027 into trinodb:master Feb 24, 2022
@github-actions github-actions bot added this to the 372 milestone Feb 24, 2022
@bitsondatadev
Copy link
Member

bitsondatadev commented Feb 25, 2022

Yay @rosewms! Thanks for finishing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants