[GitHub] [spark] cloud-fan commented on a change in pull request #26763: [SPARK-30125][SQL] Remove PostgreSQL dialect
cloud-fan commented on a change in pull request #26763: [SPARK-30125][SQL]
Remove PostgreSQL dialect
URL: https://github.com/apache/spark/pull/26763#discussion_r355490037
##
File path: sql/core/src/test/resources/sql-tests/inputs/postgreSQL/int2.sql
##
@@ -92,9 +92,11 @@ WHERE f1 > -32767;
SELECT '' AS five, i.f1, i.f1 - int('2') AS x FROM INT2_TBL i;
-SELECT '' AS five, i.f1, i.f1 / smallint('2') AS x FROM INT2_TBL i;
+-- PostgreSQL `/` is the same with Spark `div` since SPARK-2659.
+SELECT '' AS five, i.f1, i.f1 div smallint('2') AS x FROM INT2_TBL i;
Review comment:
let's keep the tests the same as pgsql. we should still use `/`.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26763: [SPARK-30125][SQL] Remove PostgreSQL dialect
cloud-fan commented on a change in pull request #26763: [SPARK-30125][SQL]
Remove PostgreSQL dialect
URL: https://github.com/apache/spark/pull/26763#discussion_r355488740
##
File path: sql/core/src/test/resources/sql-tests/inputs/postgreSQL/boolean.sql
##
@@ -22,6 +22,7 @@ SELECT false AS `false`;
SELECT boolean('t') AS true;
+-- [SPARK-27931] Trim the string when cast string type to boolean type
Review comment:
we do trim. Do we need to keep the JIRA ID?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26763: [SPARK-30125][SQL] Remove PostgreSQL dialect
cloud-fan commented on a change in pull request #26763: [SPARK-30125][SQL]
Remove PostgreSQL dialect
URL: https://github.com/apache/spark/pull/26763#discussion_r355489133
##
File path: sql/core/src/test/resources/sql-tests/inputs/postgreSQL/boolean.sql
##
@@ -98,7 +101,7 @@ SELECT boolean('f') <= boolean('t') AS true;
-- explicit casts to/from text
SELECT boolean(string('TrUe')) AS true, boolean(string('fAlse')) AS `false`;
-
+-- [SPARK-27931] Trim the string when cast to boolean type
Review comment:
ditto
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26763: [SPARK-30125][SQL] Remove PostgreSQL dialect
cloud-fan commented on a change in pull request #26763: [SPARK-30125][SQL]
Remove PostgreSQL dialect
URL: https://github.com/apache/spark/pull/26763#discussion_r355488147
##
File path:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala
##
@@ -1131,7 +1131,8 @@ class DateExpressionsSuite extends SparkFunSuite with
ExpressionEvalHelper {
checkEvaluation(SubtractDates(Literal(end), epochDate),
IntervalUtils.stringToInterval(UTF8String.fromString("interval 49 years
9 months 4 days")))
checkEvaluation(SubtractDates(epochDate, Literal(end)),
- IntervalUtils.stringToInterval(UTF8String.fromString("interval -49 years
-9 months -4 days")))
+ IntervalUtils.stringToInterval(UTF8String.fromString(
+"interval -49 years -9 months -4 days")))
Review comment:
unnecessary change.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26763: [SPARK-30125][SQL] Remove PostgreSQL dialect
cloud-fan commented on a change in pull request #26763: [SPARK-30125][SQL]
Remove PostgreSQL dialect
URL: https://github.com/apache/spark/pull/26763#discussion_r355488147
##
File path:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala
##
@@ -1131,7 +1131,8 @@ class DateExpressionsSuite extends SparkFunSuite with
ExpressionEvalHelper {
checkEvaluation(SubtractDates(Literal(end), epochDate),
IntervalUtils.stringToInterval(UTF8String.fromString("interval 49 years
9 months 4 days")))
checkEvaluation(SubtractDates(epochDate, Literal(end)),
- IntervalUtils.stringToInterval(UTF8String.fromString("interval -49 years
-9 months -4 days")))
+ IntervalUtils.stringToInterval(UTF8String.fromString(
+"interval -49 years -9 months -4 days")))
Review comment:
uncessary change.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26763: [SPARK-30125][SQL] Remove PostgreSQL dialect
cloud-fan commented on a change in pull request #26763: [SPARK-30125][SQL]
Remove PostgreSQL dialect
URL: https://github.com/apache/spark/pull/26763#discussion_r355485349
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##
@@ -287,7 +286,7 @@ class Analyzer(
case (_, CalendarIntervalType) => Cast(TimeSub(l, r), l.dataType)
case (TimestampType, _) => SubtractTimestamps(l, r)
case (_, TimestampType) => SubtractTimestamps(l, r)
- case (_, DateType) => if (conf.usePostgreSQLDialect) {
+ case (_, DateType) => if (conf.ansiEnabled) {
Review comment:
Actually `SubtractDates` is the ansi behavior.
Since `date - date` is newly added in 3.0 (see
https://issues.apache.org/jira/browse/SPARK-27898), we can just change this to
`case (_, DateType) => SubtractDates(l, r)`
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26763: [SPARK-30125][SQL] Remove PostgreSQL dialect
cloud-fan commented on a change in pull request #26763: [SPARK-30125][SQL]
Remove PostgreSQL dialect
URL: https://github.com/apache/spark/pull/26763#discussion_r355485349
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##
@@ -287,7 +286,7 @@ class Analyzer(
case (_, CalendarIntervalType) => Cast(TimeSub(l, r), l.dataType)
case (TimestampType, _) => SubtractTimestamps(l, r)
case (_, TimestampType) => SubtractTimestamps(l, r)
- case (_, DateType) => if (conf.usePostgreSQLDialect) {
+ case (_, DateType) => if (conf.ansiEnabled) {
Review comment:
Actually `SubtractDates` is the ansi behavior.
Since `date - date` is newly added in 3.0, we can just change this to `case
(_, DateType) => SubtractDates(l, r)`
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26763: [SPARK-30125][SQL] Remove PostgreSQL dialect
cloud-fan commented on a change in pull request #26763: [SPARK-30125][SQL] Remove PostgreSQL dialect URL: https://github.com/apache/spark/pull/26763#discussion_r355483703 ## File path: docs/sql-keywords.md ## @@ -19,7 +19,7 @@ license: | limitations under the License. --- -When `spark.sql.dialect=PostgreSQL` or keep default `spark.sql.dialect=Spark` with setting `spark.sql.dialect.spark.ansi.enabled` to true, Spark SQL will use the ANSI mode parser. +When `spark.sql.dialect.spark.ansi.enabled` is true, Spark SQL will use the ANSI mode parser. Review comment: I don't think we can do a clean revert, we need to check the diff of this PR and see if they make sense. Here we should use `spark.sql.ansi.enabled` 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26763: [SPARK-30125][SQL] Remove PostgreSQL dialect
cloud-fan commented on a change in pull request #26763: [SPARK-30125][SQL]
Remove PostgreSQL dialect
URL: https://github.com/apache/spark/pull/26763#discussion_r354299779
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala
##
@@ -65,15 +65,11 @@ object StringUtils extends Logging {
"(?s)" + out.result() // (?s) enables dotall mode, causing "." to match
new lines
}
- private[this] val trueStrings =
Review comment:
unnecessary change.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26763: [SPARK-30125][SQL] Remove PostgreSQL dialect
cloud-fan commented on a change in pull request #26763: [SPARK-30125][SQL] Remove PostgreSQL dialect URL: https://github.com/apache/spark/pull/26763#discussion_r354299435 ## File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala ## @@ -2126,7 +2126,7 @@ case class DatePart(field: Expression, source: Expression, child: Expression) * is set to 0 and the `microseconds` field is initialized to the microsecond difference * between the given timestamps. */ -case class SubtractTimestamps(endTimestamp: Expression, startTimestamp: Expression) Review comment: ditto, keep it. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26763: [SPARK-30125][SQL] Remove PostgreSQL dialect
cloud-fan commented on a change in pull request #26763: [SPARK-30125][SQL] Remove PostgreSQL dialect URL: https://github.com/apache/spark/pull/26763#discussion_r354299520 ## File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala ## @@ -2143,25 +2143,3 @@ case class SubtractTimestamps(endTimestamp: Expression, startTimestamp: Expressi s"new org.apache.spark.unsafe.types.CalendarInterval(0, 0, $end - $start)") } } - -/** - * Returns the interval from the `left` date (inclusive) to the `right` date (exclusive). - */ -case class SubtractDates(left: Expression, right: Expression) Review comment: ditto 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26763: [SPARK-30125][SQL] Remove PostgreSQL dialect
cloud-fan commented on a change in pull request #26763: [SPARK-30125][SQL]
Remove PostgreSQL dialect
URL: https://github.com/apache/spark/pull/26763#discussion_r354299153
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala
##
@@ -860,14 +853,12 @@ object TypeCoercion {
case Add(l @ DateType(), r @ IntegerType()) => DateAdd(l, r)
case Add(l @ IntegerType(), r @ DateType()) => DateAdd(r, l)
case Subtract(l @ DateType(), r @ IntegerType()) => DateSub(l, r)
- case Subtract(l @ DateType(), r @ DateType()) =>
-if (SQLConf.get.usePostgreSQLDialect) DateDiff(l, r) else
SubtractDates(l, r)
- case Subtract(l @ TimestampType(), r @ TimestampType()) =>
-SubtractTimestamps(l, r)
+ case Subtract(l @ DateType(), r @ DateType()) => DateDiff(l, r)
+ case Subtract(l @ TimestampType(), r @ TimestampType()) =>
TimestampDiff(l, r)
case Subtract(l @ TimestampType(), r @ DateType()) =>
-SubtractTimestamps(l, Cast(r, TimestampType))
+TimestampDiff(l, Cast(r, TimestampType))
Review comment:
we should keep it. It's nothing to do with pgsql dialect.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26763: [SPARK-30125][SQL] Remove PostgreSQL dialect
cloud-fan commented on a change in pull request #26763: [SPARK-30125][SQL]
Remove PostgreSQL dialect
URL: https://github.com/apache/spark/pull/26763#discussion_r354298981
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala
##
@@ -860,14 +853,12 @@ object TypeCoercion {
case Add(l @ DateType(), r @ IntegerType()) => DateAdd(l, r)
case Add(l @ IntegerType(), r @ DateType()) => DateAdd(r, l)
case Subtract(l @ DateType(), r @ IntegerType()) => DateSub(l, r)
- case Subtract(l @ DateType(), r @ DateType()) =>
-if (SQLConf.get.usePostgreSQLDialect) DateDiff(l, r) else
SubtractDates(l, r)
- case Subtract(l @ TimestampType(), r @ TimestampType()) =>
-SubtractTimestamps(l, r)
+ case Subtract(l @ DateType(), r @ DateType()) => DateDiff(l, r)
Review comment:
This is not a simple revert. We want to create `SubtractDates` by default,
and we need to create a legacy config since the pgsql dialect is gone.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26763: [SPARK-30125][SQL] Remove PostgreSQL dialect
cloud-fan commented on a change in pull request #26763: [SPARK-30125][SQL]
Remove PostgreSQL dialect
URL: https://github.com/apache/spark/pull/26763#discussion_r354298297
##
File path:
sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
##
@@ -919,12 +919,6 @@ qualifiedNameList
: qualifiedName (',' qualifiedName)*
;
-functionName
Review comment:
this is for ANSI-compliant, not pgsql dialect, we should keep it.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26763: [SPARK-30125][SQL] Remove PostgreSQL dialect
cloud-fan commented on a change in pull request #26763: [SPARK-30125][SQL] Remove PostgreSQL dialect URL: https://github.com/apache/spark/pull/26763#discussion_r354297985 ## File path: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 ## @@ -52,9 +52,9 @@ grammar SqlBase; } /** - * When true, the behavior of keywords follows ANSI SQL standard. + * When true, ANSI SQL parsing mode is enabled. */ - public boolean SQL_standard_keyword_behavior = false; Review comment: I think `SQL_standard_keyword_behavior` is better. Can we keep it? ANSI is too vague here. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
