You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "nchammas (via GitHub)" <gi...@apache.org> on 2024/02/05 07:25:19 UTC
[PR] [MINOR][DOCS] Document default fractional numeric literals in SQL [spark]
nchammas opened a new pull request, #45027:
URL: https://github.com/apache/spark/pull/45027
### What changes were proposed in this pull request?
Explain what type literals like `123.456` and `123.456E0` have in SQL.
### Why are the changes needed?
In Python (and I think Java too) fractional numeric literals are typically floats. To get decimals, you need to provide an explicit postfix or use an explicit class. In Spark, it's the other way around. I found this surprising and couldn't find documentation about it.
I discovered this after reading [SPARK-45786](https://issues.apache.org/jira/browse/SPARK-45786). I did a little searching and came across https://github.com/apache/spark/pull/10796, which shows that we used to default to floats as the fractional numeric literal, but then switched to decimals.
There is an additional wrinkle I discovered in #45003. If the fractional literal has an exponent, then it's a double, not a decimal.
None of this is obvious from the docs, at least not to me.
### Does this PR introduce _any_ user-facing change?
Yes, it clarifies the user-facing documentation about fractional numeric literals.
### How was this patch tested?
No testing.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [MINOR][DOCS] Document default fractional numeric literals in SQL [spark]
Posted by "nchammas (via GitHub)" <gi...@apache.org>.
nchammas commented on code in PR #45027:
URL: https://github.com/apache/spark/pull/45027#discussion_r1477801777
##########
docs/sql-ref-literals.md:
##########
@@ -275,15 +275,30 @@ E [ + | - ] digit [ ... ]
Case insensitive, indicates `DECIMAL`, with the total number of digits as precision and the number of digits to right of decimal point as scale.
+* **default (no postfix, no exponent)**
Review Comment:
Understanding what the literal type is from the syntax is not clear at all. Maybe it's just me, but I spent a good minute or two looking at that first and not getting that `BD` is the default in the case of no exponent.
Now that I know that's true, I can see that this bit of syntax can be reduced as follows:
```
decimal_digits { [ BD ] | [ exponent BD ] } | digit [ ... ] [ exponent ] BD
decimal_digits { [ BD ] | [ exponent BD ] }
decimal_digits { [ BD ] }
decimal_digits
```
But I doubt most users will find this easy to follow just by looking at the syntax.
The new bullets explaining the default explicitly are way easier to understand, and also mirror a similar bullet for integral literals:
https://github.com/apache/spark/blob/b4346ceffab74cae4cc1d6e053d5bde3b5387d5f/docs/sql-ref-literals.md#L197-L199
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [MINOR][DOCS] Clarify docs on default fractional numeric literals in SQL [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn closed pull request #45027: [MINOR][DOCS] Clarify docs on default fractional numeric literals in SQL
URL: https://github.com/apache/spark/pull/45027
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [MINOR][DOCS] Document default fractional numeric literals in SQL [spark]
Posted by "nchammas (via GitHub)" <gi...@apache.org>.
nchammas commented on code in PR #45027:
URL: https://github.com/apache/spark/pull/45027#discussion_r1477808410
##########
docs/sql-ref-literals.md:
##########
@@ -275,15 +275,30 @@ E [ + | - ] digit [ ... ]
Case insensitive, indicates `DECIMAL`, with the total number of digits as precision and the number of digits to right of decimal point as scale.
+* **default (no postfix, no exponent)**
Review Comment:
> FYI, an internal config will also change this behavior
Interesting. I assume we don't want to mention this since it's an internal config, correct?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [MINOR][DOCS] Document default fractional numeric literals in SQL [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #45027:
URL: https://github.com/apache/spark/pull/45027#discussion_r1477794812
##########
docs/sql-ref-literals.md:
##########
@@ -275,15 +275,30 @@ E [ + | - ] digit [ ... ]
Case insensitive, indicates `DECIMAL`, with the total number of digits as precision and the number of digits to right of decimal point as scale.
+* **default (no postfix, no exponent)**
Review Comment:
FYI, an internal config will also change this behavior
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [MINOR][DOCS] Document default fractional numeric literals in SQL [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #45027:
URL: https://github.com/apache/spark/pull/45027#discussion_r1477779401
##########
docs/sql-ref-literals.md:
##########
@@ -275,15 +275,30 @@ E [ + | - ] digit [ ... ]
Case insensitive, indicates `DECIMAL`, with the total number of digits as precision and the number of digits to right of decimal point as scale.
+* **default (no postfix, no exponent)**
Review Comment:
I think we should probably just comebine them into one. @yaooqinn do you know any other cases or variants?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [MINOR][DOCS] Document default fractional numeric literals in SQL [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #45027:
URL: https://github.com/apache/spark/pull/45027#discussion_r1477859257
##########
docs/sql-ref-literals.md:
##########
@@ -275,15 +275,30 @@ E [ + | - ] digit [ ... ]
Case insensitive, indicates `DECIMAL`, with the total number of digits as precision and the number of digits to right of decimal point as scale.
+* **default (no postfix, no exponent)**
Review Comment:
Using a style, such as BNF, like most commercial database vendors do, helps reduce the ambiguity. Users who are unfamiliar with these syntaxes may need to take some time to learn them. It's good to give more instructions and examples without introducing extra maintenance effort, ambiguity, etc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [MINOR][DOCS] Document default fractional numeric literals in SQL [spark]
Posted by "nchammas (via GitHub)" <gi...@apache.org>.
nchammas commented on PR #45027:
URL: https://github.com/apache/spark/pull/45027#issuecomment-1926371390
cc @yaooqinn @HyukjinKwon
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [MINOR][DOCS] Document default fractional numeric literals in SQL [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #45027:
URL: https://github.com/apache/spark/pull/45027#discussion_r1477859953
##########
docs/sql-ref-literals.md:
##########
@@ -275,15 +275,30 @@ E [ + | - ] digit [ ... ]
Case insensitive, indicates `DECIMAL`, with the total number of digits as precision and the number of digits to right of decimal point as scale.
+* **default (no postfix, no exponent)**
Review Comment:
> Interesting. I assume we don't want to mention this since it's an internal config, correct?
Yes, we rarely(maybe never) do that
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [MINOR][DOCS] Clarify docs on default fractional numeric literals in SQL [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #45027:
URL: https://github.com/apache/spark/pull/45027#issuecomment-1928778310
Thanks, merged to master
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [MINOR][DOCS] Document default fractional numeric literals in SQL [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #45027:
URL: https://github.com/apache/spark/pull/45027#discussion_r1477788622
##########
docs/sql-ref-literals.md:
##########
@@ -275,15 +275,30 @@ E [ + | - ] digit [ ... ]
Case insensitive, indicates `DECIMAL`, with the total number of digits as precision and the number of digits to right of decimal point as scale.
+* **default (no postfix, no exponent)**
Review Comment:
IMHO, this new section is not necessary
- Necessary `Parameters` are listed and explained above
- For `decimal/double literals`: their SYNTAXes already described these behaviors
Keeping the examples might be enough.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [MINOR][DOCS] Document default fractional numeric literals in SQL [spark]
Posted by "nchammas (via GitHub)" <gi...@apache.org>.
nchammas commented on code in PR #45027:
URL: https://github.com/apache/spark/pull/45027#discussion_r1477813085
##########
docs/sql-ref-literals.md:
##########
@@ -275,15 +275,30 @@ E [ + | - ] digit [ ... ]
Case insensitive, indicates `DECIMAL`, with the total number of digits as precision and the number of digits to right of decimal point as scale.
+* **default (no postfix, no exponent)**
Review Comment:
I see your point that the syntax technically explains this, though I personally don't find it that user-friendly.
I'm fine with keeping just the examples.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org