Skip to content

Conversation

@ajantha-bhat
Copy link
Member

@ajantha-bhat ajantha-bhat commented Sep 2, 2024

Table of contents in the spec web page is not generated for spec and its subsection after #10948. It is super hard to navigate the spec without the table of contents.

The reason is mkdocs only consider first H1 for TOC generation.
More info: squidfunk/mkdocs-material#818

So, keeping spec as H2 and adjusted its sub topics.
Note that the double # removal is not fully reverted, only added back one # . It is still kept at the same level as expected in the PR#10948

Also locally verified the new table of contents by building it in the site directory using make serve and hosted at http://localhost:8000/spec

@RussellSpitzer
Copy link
Member

I think this is probably a reasonable thing to do. Looking online, apparently switching to a single H1 element will also help out screen readers and accessibility software as well so sounds like a good thing to do. I would like to hear if anyone else has strong opnions though, especially since we have a few inflight Spec changes.

@ajantha-bhat
Copy link
Member Author

ping @rdblue



# Specification
## Specification
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that this is the only problem. It appears that if there are multiple H1 sections, only the first one shows up in TOC. For the other changes, we do want to avoid over-nesting, which was originally added a few website iterations ago to get things to stop showing up in the TOC.

I think for at least a few of these, we can just change Specification and not other sections. For example, Terms is already H3, so there's no need to make it H4 because there is no intermediate H3 level above it. Everything before Schemas and Data Types falls in that category.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, I just changed spec to H2 and reverted other changes. Rendered site LGTM.

Updated the PR.

Writers are not allowed to commit files with a partition spec that contains a field with an unknown transform.

## Schemas and Data Types
### Schemas and Data Types
Copy link
Contributor

Choose a reason for hiding this comment

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

Because the container (specification) is going to be H2, I think this is a decision point. Do we care that things beyond this point are nested within Specification? I think I would rather have headings that are more distinct in the body of the spec than worry about these sections being nested within Spec. I'm curious what other people think though. @RussellSpitzer?

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, I just changed spec to H2 and reverted other changes. Rendered site LGTM.

Updated the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rdblue I think we want to add additional nesting because we can limit the depth in the ToC configuration. That way we can have proper structure and not have the table of contents explode.

Copy link
Member Author

Choose a reason for hiding this comment

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

The maximum depth we have is 3, do we have any plan to add depth limit? If not, maybe we don't have to argue on this.

Copy link
Member

Choose a reason for hiding this comment

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

I really don't have any preference. I'm fine with just pumping all the nesting one more step deep.

Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't right though. The Specification header should be above the others (e.g. Schemas and Data Types) not at the same level.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, that sounds fine to me.

@ajantha-bhat
Copy link
Member Author

ajantha-bhat commented Oct 15, 2024

TOC with this change

image

@danielcweeks
Copy link
Contributor

@ajantha-bhat I feel like this has bounced back and forth on the levels, but we should be focused on getting the structure correct first and then addressing the ToC. This current version has Specification at the same level as its children, which is not correct.

L2/H2 should be:

## Format Versioning
## Goals
## Overview
## Specification
## Appendix A: Format-specific Requirements
## Appendix B: 32-bit Hash Requirements
## Appendix C: JSON serialization
## Appendix D: Single-value serialization
## Appendix E: Format version changes
## Appendix F: Implementation Notes

Everything else should be nested below.

@ajantha-bhat
Copy link
Member Author

@danielcweeks: Thanks for the feedback. I have updated it accordingly.

New TOC looks like this.

image

@ajantha-bhat ajantha-bhat added this to the Iceberg 1.7.0 milestone Oct 17, 2024
@rdblue
Copy link
Contributor

rdblue commented Oct 19, 2024

I think this is fine. @danielcweeks is the organization what you want?

@danielcweeks
Copy link
Contributor

@ajantha-bhat looks like we have conflicts. It would be good to get this in, but I don't think this section of the docs is tied to the 1.7.0 release.

@ajantha-bhat
Copy link
Member Author

@danielcweeks: Thanks for the review. Conflict was due to recent row-lineage merge. I have resolved it now. PR is ready.

@RussellSpitzer RussellSpitzer merged commit 7738e1d into apache:main Oct 25, 2024
@RussellSpitzer
Copy link
Member

Thanks @ajantha-bhat and @danielcweeks , @rdblue , @manuzhang and @amogh-jahagirdar for review

zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Specification Issues that may introduce spec changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants