Skip to content

Add Iceberg configuration properties in docs#7200

Merged
phd3 merged 1 commit intotrinodb:masterfrom
amitds1997:iceberg-docs
Mar 14, 2021
Merged

Add Iceberg configuration properties in docs#7200
phd3 merged 1 commit intotrinodb:masterfrom
amitds1997:iceberg-docs

Conversation

@amitds1997
Copy link
Copy Markdown
Contributor

Added configurable properties for Iceberg connectors. Have purposefully left out iceberg.use-file-size-from-metadata since it has been marked deprecated.

@cla-bot cla-bot bot added the cla-signed label Mar 7, 2021
Copy link
Copy Markdown
Member

@phd3 phd3 left a comment

Choose a reason for hiding this comment

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

Looks like a good improvement.

(In future, it'd be worth making the configuration section more exhaustive, but that might be difficult to do without making a more fundamental change of isolating/renaming some hive configs.)

@phd3 phd3 requested review from electrum and mosabua March 9, 2021 07:29
Iceberg configuration properties
--------------------------------

================================================== ============================================================ ============
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Convert this to a list table please

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I assume this just means adding them as different sub-sections. Let me know if it's otherwise.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh .. sorry.. here is an example of a list table -

Cache Configuration Parameters
in
https://github.com/trinodb/trino/blob/master/docs/src/main/sphinx/connector/hive-caching.rst

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Can you review this once again?

--------------------------------

================================================== ============================================================ ============
Property Name Description Default
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Property name

Copy link
Copy Markdown
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

A few small changes requested..

Possible values are ``PARQUET``, or ``ORC``.

``iceberg.compression-codec`` The compression codec to use when writing files. ``GZIP``
Possible values are ``NONE``, ``SNAPPY``, ``LZ4``,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe better to have the values in a bullet list

@amitds1997 amitds1997 requested a review from mosabua March 10, 2021 07:47
Copy link
Copy Markdown
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

One last mini tweak.. but essentially done. Awesome job.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Remove the bolding of the title .. if we want that as a general thing we do it across all tables or just update the CSS

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Understood! Done 👍🏽

@amitds1997 amitds1997 requested a review from mosabua March 12, 2021 03:09
Copy link
Copy Markdown
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Ship it!

@amitds1997 amitds1997 requested a review from phd3 March 12, 2021 06:53
Copy link
Copy Markdown
Member

@phd3 phd3 left a comment

Choose a reason for hiding this comment

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

lgtm, just one comment.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is purely relevant for writing, I'd revert to that wording.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, somewhere when rewriting, I must have rewritten this. Done

Added configurable properties for Iceberg connectors. Have purposefully
left out `iceberg.use-file-size-from-metadata` since it has been marked
deprecated.
@amitds1997 amitds1997 requested a review from phd3 March 13, 2021 02:31
@phd3 phd3 merged commit 0e4a8a9 into trinodb:master Mar 14, 2021
@phd3
Copy link
Copy Markdown
Member

phd3 commented Mar 14, 2021

Thanks @amitds1997 !

v-jizhang added a commit to v-jizhang/presto that referenced this pull request May 18, 2022
Cherry-pick of trinodb/trino#7200

Co-authored-by: Amit Singh <amitds1997@gmail.com>
NikhilCollooru pushed a commit to prestodb/presto that referenced this pull request May 19, 2022
Cherry-pick of trinodb/trino#7200

Co-authored-by: Amit Singh <amitds1997@gmail.com>
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.

3 participants