Skip to content

Add table functions to function list#17395

Merged
mosabua merged 1 commit into
trinodb:masterfrom
colebow:colebow/add-to-functions-list
May 10, 2023
Merged

Add table functions to function list#17395
mosabua merged 1 commit into
trinodb:masterfrom
colebow:colebow/add-to-functions-list

Conversation

@colebow
Copy link
Copy Markdown
Member

@colebow colebow commented May 8, 2023

Description

Additional docs for #16716

Release notes

(x) This is not user-visible or docs only and no release notes are required.

@cla-bot cla-bot Bot added the cla-signed label May 8, 2023
@colebow colebow requested review from findepi, kasiafi and mosabua May 8, 2023 21:44
@github-actions github-actions Bot added the docs label May 8, 2023
Comment thread docs/src/main/sphinx/functions/list.rst Outdated
Comment thread docs/src/main/sphinx/functions/list.rst Outdated
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.

or remove () here ... and instead do a

sequence (table function)

or something like that?

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 ()

Copy link
Copy Markdown
Member

@kasiafi kasiafi May 10, 2023

Choose a reason for hiding this comment

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

When () is removed, sequence TF renders different than exclude_columns TF.
I think we should keep (), and additionally add a note like (table function) for disambiguation.

So, it would be:
- :ref:sequence ( ) <sequence_table_function> (table function)

Comment thread docs/src/main/sphinx/functions/list.rst Outdated
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 ()

@colebow colebow force-pushed the colebow/add-to-functions-list branch from 381ebbb to 0704d78 Compare May 9, 2023 18:19
Comment thread docs/src/main/sphinx/functions/list.rst Outdated
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.

So how would a reader distinguish the two sequence functions by looking at this list .. dont we need to have some sort of differentiation @findepi and @colebow ?

Copy link
Copy Markdown
Member Author

@colebow colebow May 10, 2023

Choose a reason for hiding this comment

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

I feel like the best fix for this is renaming one of the functions - having two different functions with the same name seems problematic in general.

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.

Scalar and table functions are in different namespaces, so we shouldn’t rename them just because they get mixed up in this list.

An alternative would be to have a separate list for table functions, or add a disambiguation parenthetical.

Copy link
Copy Markdown
Member

@mosabua mosabua May 10, 2023

Choose a reason for hiding this comment

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

Thats what I suggested in another comment .. and I think this is better than having the same wording twice with different links

sequencce
sequence (table function)

and only add that for table functions.. otherwise we end up with lots of (scalar function)

or we could do

sequence (scalar function, table function)

and link the ones in brackets.. but personally I think that is worse than having two lines..

@colebow colebow force-pushed the colebow/add-to-functions-list branch 3 times, most recently from 05c4f36 to acd0bb0 Compare May 10, 2023 20:21
@colebow colebow force-pushed the colebow/add-to-functions-list branch from acd0bb0 to 4976e0e Compare May 10, 2023 20:22
@colebow
Copy link
Copy Markdown
Member Author

colebow commented May 10, 2023

I think I most prefer @kasiafi's suggestion here - sequence() (table function) feels the most appropriate, as it makes the function appear most similar to all other functions. I also like disambiguating the scalar function for extra clarity, so I've done that to the line above. I don't think we need to proactively disambiguate other functions on this list (or separate out other table functions) unless they share names with other functions.

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.

I like the agreed approach..

@mosabua mosabua merged commit b7f70d9 into trinodb:master May 10, 2023
@github-actions github-actions Bot added this to the 417 milestone May 10, 2023
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