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.

Sign up for GitHub

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

Jump to bottom

docs: elucidate obscure discussion points #523

Merged
merged 11 commits into from
Nov 22, 2023
Merged

docs: elucidate obscure discussion points #523

merged 11 commits into from
Nov 22, 2023

Conversation

EpsilonPrime
Copy link
Member

  • Various description updates.
  • Adds to Substrait Fiddle the list of third party tools.
  • Changes discussion points to use admonitions to separate them better from the rest of the page.

@EpsilonPrime
feat: clarify map behavior
0ad0169 
   * Map keys must be unique.
   * Map keys must not be NULL.
   * The map key type may be nullable.

This is based on the current restrictions found in the wild.  For example, DuckDB specifically calls out that keys must be unique in its implementation and Spark specifically calls out that keys may not be NULL.  This makes sense as map keys often need to be hashable for performance reasons.

If for some reason we need a map type that supports repeated keys it may be better to create an alterate type (or a clarifying parameter/option).
@EpsilonPrime
docs: elucidate various
c140468 
Also adds to Substrait Fiddle the list of third party tools and updates
some pages with admonitions.
westonpace
westonpace previously requested changes Jul 21, 2023
View reviewed changes
Copy link
Member

@westonpace westonpace left a comment

Choose a reason for hiding this comment

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

This looks pretty good. Thanks for taking a stab at cleaning things up.

site/docs/expressions/embedded_functions.md Show resolved Hide resolved
site/docs/expressions/subqueries.md Show resolved Hide resolved
site/docs/expressions/user_defined_functions.md Show resolved Hide resolved
site/docs/relations/basics.md Outdated Show resolved Hide resolved
site/docs/relations/basics.md Outdated Show resolved Hide resolved
site/docs/tools/third_party_tools.md Outdated Show resolved Hide resolved
site/docs/types/type_parsing.md Outdated Show resolved Hide resolved
site/docs/types/type_parsing.md Outdated Show resolved Hide resolved
site/docs/types/type_parsing.md Outdated Show resolved Hide resolved
site/mkdocs.yml Outdated Show resolved Hide resolved
@EpsilonPrime EpsilonPrime requested a review from westonpace July 28, 2023 06:45
@EpsilonPrime
Copy link
Member Author

@westonpace Any further thoughts here?

@EpsilonPrime EpsilonPrime requested a review from vbarua as a code owner October 19, 2023 03:42
@EpsilonPrime EpsilonPrime dismissed westonpace’s stale review November 22, 2023 19:23

Sufficient time has passed.

@EpsilonPrime EpsilonPrime merged commit df0fa8e into substrait-io:main Nov 22, 2023
13 checks passed
@EpsilonPrime EpsilonPrime deleted the docoverhaul branch November 22, 2023 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jacques-n jacques-n jacques-n approved these changes

@cpcloud cpcloud Awaiting requested review from cpcloud cpcloud is a code owner

@westonpace westonpace Awaiting requested review from westonpace westonpace is a code owner

@vbarua vbarua Awaiting requested review from vbarua vbarua is a code owner

Assignees

@westonpace westonpace

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@EpsilonPrime @jacques-n @westonpace