Skip to content

Conversation

@pvary
Copy link
Contributor

@pvary pvary commented May 31, 2022

Update Hive doc page with the 4.0.0-alpha-1 features

@pvary pvary requested a review from samredai May 31, 2022 11:48
@github-actions github-actions bot added the docs label May 31, 2022
Copy link
Contributor

@szlta szlta left a comment

Choose a reason for hiding this comment

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

Generally looks good, some minor remarks.

@pvary
Copy link
Contributor Author

pvary commented Jun 1, 2022

Generally looks good, some minor remarks.

Thanks @szlta for the review!
Could you please check the changes?

Thanks,
Peter

Copy link
Contributor

@szlta szlta 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, thanks for the corrections.
+1 non-binding
PS: feel free to resolve my comments - seems like I don't have permission to do so...

@lcspinter
Copy link
Contributor

Thanks @pvary for your work!
+1 non-binding

* Creating a table
* Dropping a table
* Reading a table
* Inserting into a table (will append rows) (MapReduce only)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can (will append rows) and (MapReduce only) be expanded to warning/info boxes? Something along the lines of:

{{< hint info >}}
Inserting into a table will append rows to the relevant partitions.
{{< /hint >}}
{{< hint warning >}}
Inserting into a table works with MapReduce only.
{{< /hint >}}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first parenthesis was trying to highlight the difference between the INSERT INTO and the INSERT OVERWRITE.

Could you please check?

* Inserting into a table (will append rows) (MapReduce only)

Iceberg compatibility with Hive 4.0.0-alpha-1
(Using HiveCatalog) supports the following, additional features:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'd word this as

With Hive version 4.0.0-alpha-1 and above, the Iceberg HiveCatalog supports the following additional features:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reworded to this:

With Hive version 4.0.0-alpha-1 and above,
the Iceberg integration when using HiveCatalog supports the following additional features:

The point here is that some of the features are dependent on HiveCatalog, but HiveCatalog is not the full integration. You can use a different catalog implementation (say HadoopCatalog), and most of the features will work, but for example the Hive schema / Iceberg schema sync will work only with HiveCatalog.

Is this better? Any other suggestions?

* Migrating tables in Avro, Parquet, or ORC (Non-ACID) format to Iceberg
* Reading the schema of a table
* Time travel applications
* Inserting into a table (will append rows) (Tez only)
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to my other comment, I would put these in warning or info boxes instead of tacking them onto the end of the line. Those boxes are more visible disclaimers.


You need to do the following steps:
* Loading runtime jar
* Enabling support
Copy link
Contributor

Choose a reason for hiding this comment

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

This could just be one line right?

In order to use Hive 2.3.x or Hive 3.1.x, you must load the Iceberg-Hive runtime jar and enable Iceberg support, either globally or for an individual table using a table property.

and then jump into the sections below that provide more details.

@pvary
Copy link
Contributor Author

pvary commented Jun 8, 2022

@samredai: Could you please check again? Went through your comments, and added 2 new paragraphs.
Thanks,
Peter

@pvary
Copy link
Contributor Author

pvary commented Jun 28, 2022

@samredai: I have checked the conflicts caused by #5115.
This seems to me the relevant part:

---
title: "Hive"
url: hive
weight: 400
menu: main
---

What about the other changes?
It would be good to merge the Hive integration part to the docs as well. What is needed for getting them in?

Thanks,
Peter

@samredai
Copy link
Contributor

@pvary I opened a PR with these changes directly against the latest branch and we'll need to do the same for the 0.13.2 branch. That way this can get released with the current format of the docs site and I'll make sure those changes are carried over to the new site format.

@pvary
Copy link
Contributor Author

pvary commented Jun 28, 2022

Thanks @samredai!
Are you eligible to +1/merge, or shall we wait for @rdblue?

@samredai
Copy link
Contributor

We'll have to wait for @rdblue or another committer to review it and merge

@pvary
Copy link
Contributor Author

pvary commented Jun 29, 2022

Thanks @samredai for the help. With Ryan's review, I was able to merge apache/iceberg-docs#98

@rdblue
Copy link
Contributor

rdblue commented Jun 29, 2022

Looks good to me. Can you rebase? I don't think that we need the _index.md file any more. @samredai, how should we update this?

@samredai
Copy link
Contributor

Instead of _index.md it should be renamed to hive.md and moved up one directory, which means the hive directory can be removed.

@pvary
Copy link
Contributor Author

pvary commented Jun 29, 2022

@samredai: Any other change that we have to keep from the changes came in for docs/hive/_index.md? What I see are only formatting changes

@samredai
Copy link
Contributor

Ah that's right, this has already been added by PR #5115 . That includes all of the changes from this PR here so this should be good to close.

@samredai
Copy link
Contributor

Here's a rebased PR #5161 with the content added to the existing hive.md file. @pvary can you confirm the content all looks good? Then we can merge that and close this PR.

@pvary
Copy link
Contributor Author

pvary commented Jun 29, 2022

Here's a rebased PR #5161 with the content added to the existing hive.md file. @pvary can you confirm the content all looks good? Then we can merge that and close this PR.

Will check it tomorrow. It is quite late here, and the change is big.

Thanks for all the help!

@pvary
Copy link
Contributor Author

pvary commented Jun 30, 2022

Merged #5161 to master.
Thanks @samredai and @krishahn for the help!
Thanks @szlta, @lcspinter and @rdblue for the reviews!

@pvary pvary closed this Jun 30, 2022
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