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.

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

[MINOR][SQL][DOCS] consistency in argument naming for time functions #29007

Closed

Conversation

MichaelChirico
Copy link
Contributor

What changes were proposed in this pull request?

Rename documented argument format as fmt, to match the same argument name in several other SQL date/time functions, to wit, date_format, date_trunc, trunc, to_date, and to_timestamp all use fmt. Also format_string and printf use the same abbreviation in their argument strfmt.

Why are the changes needed?

Consistency -- I was trying to scour the documentation for functions with arguments using Java string formatting, it would have been nice to rely on searching for fmt instead of my more manual approach.

Does this PR introduce any user-facing change?

In the documentation only

How was this patch tested?

No tests

@SparkQA
Copy link

SparkQA commented Jul 6, 2020

Test build #125030 has finished for PR 29007 at commit 524023b.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@srowen
Copy link
Member

srowen commented Jul 6, 2020

Jenkins retest this please

@srowen
Copy link
Member

srowen commented Jul 6, 2020

OK, probably fine if it's just docs and for consistency. That's all of them?

@SparkQA
Copy link

SparkQA commented Jul 6, 2020

Test build #125088 has started for PR 29007 at commit 524023b.

@MichaelChirico
Copy link
Contributor Author

That's all of them?

In fact I found one more (in from_unixtime). Fixed now.

Pattern searched:

usage[^)]*format

@shaneknapp
Copy link
Contributor

test this please

@SparkQA
Copy link

SparkQA commented Jul 6, 2020

Test build #125095 has finished for PR 29007 at commit 5e2ae96.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jul 6, 2020

Test build #125105 has finished for PR 29007 at commit 5e2ae96.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@bart-samwel
Copy link

May I suggest going the other way, i.e., avoiding abbreviation? I don't think there's any value in abbreviating, other than that it makes the argument's meaning less immediately clear. fmt could mean many things -- ferment, forment, foment. Of course it's kind of a standard abbreviation for CS savvy folks, but if this is for ordinary users, then it may not be.

@MichaelChirico
Copy link
Contributor Author

fmt could very clearly not mean any of those things in this context 😄

Further, spelled-out format is written in the plain-English usage section of every one of these functions so I don't see any scope for confusion.

I chose to abbreviate because it was easier for my use case -- format has more than one meaning in the documentation (e.g. DDL format), so looking for fmt made my search more precise -- distinguishing argument names from plain English descriptions

@bart-samwel
Copy link

Thinking about this some more, there may actually be something more important, which is compatibility with named arguments. The names in the docs should probably match the names in the code. And we can't change the names in the code because of API compatibility -- existing callers may refer to them using named arguments.

@MichaelChirico
Copy link
Contributor Author

Note that this PR only changes the docstrings in the SQL API, AFAIK SQL doesn't support named arguments

@bart-samwel
Copy link

People don't use those references for non-SQL calls?

@MichaelChirico
Copy link
Contributor Author

no, since there are subtle but important differences and the respective APIs are easy to find right alongside one another on the Spark docs website.

I can personally assure I read R for R reference, SQL for SQL reference, and Python for python reference.

I also see headings for Java and Scala referenced in the same directory, but I rarely use those APIs so can't personally say I use them.

@srowen
Copy link
Member

srowen commented Jul 7, 2020

I also prefer 'format' in general, but if this change is the smaller one, let's just do it rather than argue it much more. Someone can bulk change it again to format if desired.

@srowen
Copy link
Member

srowen commented Jul 9, 2020

Jenkins test this please

@SparkQA
Copy link

SparkQA commented Jul 9, 2020

Test build #125483 has started for PR 29007 at commit 5e2ae96.

@srowen
Copy link
Member

srowen commented Jul 11, 2020

Jenkins test this please

@SparkQA
Copy link

SparkQA commented Jul 11, 2020

Test build #125685 has finished for PR 29007 at commit 5e2ae96.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@srowen srowen changed the title [SPARK-XXXXX][SQL][DOCS] consistency in argument naming for time functions [MINOR][SQL][DOCS] consistency in argument naming for time functions Jul 12, 2020
@srowen srowen closed this in 6ae400c Jul 12, 2020
@srowen
Copy link
Member

srowen commented Jul 12, 2020

Merged to master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants