[SPARK-29837][SQL] PostgreSQL dialect: cast to boolean #26463

Ngone51 · 2019-11-11T09:06:56Z

What changes were proposed in this pull request?

Make SparkSQL's cast to boolean behavior be consistent with PostgreSQL when
spark.sql.dialect is configured as PostgreSQL.

Why are the changes needed?

SparkSQL and PostgreSQL have a lot different cast behavior between types by default. We should make SparkSQL's cast behavior be consistent with PostgreSQL when spark.sql.dialect is configured as PostgreSQL.

Does this PR introduce any user-facing change?

Yes. If user switches to PostgreSQL dialect now, they will

get an exception if they input a invalid string, e.g "erut", while they get null before;
get an exception if they input TimestampType, DateType, LongType, ShortType, ByteType, DecimalType, DoubleType, FloatType values, while they get true or false result before.

And here're evidences for those unsupported types from PostgreSQL:

timestamp:

postgres=# select cast(cast('2019-11-11' as timestamp) as boolean);
ERROR:  cannot cast type timestamp without time zone to boolean

date:

postgres=# select cast(cast('2019-11-11' as date) as boolean);
ERROR:  cannot cast type date to boolean

bigint:

postgres=# select cast(cast('20191111' as bigint) as boolean);
ERROR:  cannot cast type bigint to boolean

smallint:

postgres=# select cast(cast(2019 as smallint) as boolean);
ERROR:  cannot cast type smallint to boolean

bytea:

postgres=# select cast(cast('2019' as bytea) as boolean);
ERROR:  cannot cast type bytea to boolean

decimal:

postgres=# select cast(cast('2019' as decimal) as boolean);
ERROR:  cannot cast type numeric to boolean

float:

postgres=# select cast(cast('2019' as float) as boolean);
ERROR:  cannot cast type double precision to boolean

How was this patch tested?

Added and tested manually.

SparkQA · 2019-11-11T11:50:50Z

Test build #113579 has finished for PR 26463 at commit 75d51ba.

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

Ngone51 · 2019-11-11T15:10:43Z

cc @cloud-fan

cloud-fan · 2019-11-11T15:22:33Z

...c/main/scala/org/apache/spark/sql/catalyst/expressions/postgreSQL/PostgreCastToBoolean.scala

+      })
+    case TimestampType | DateType | LongType | ShortType |
+         ByteType | DecimalType() | DoubleType | FloatType =>
+      _ => throw new UnsupportedOperationException(s"cannot cast type $from to boolean")


This means we will get exception at runtime. It's better to detect this kind of type errors and fail at analysis time.

moved into checkInputDataTypes()

cloud-fan · 2019-11-11T15:23:05Z

...c/main/scala/org/apache/spark/sql/catalyst/expressions/postgreSQL/PostgreCastToBoolean.scala

+
+  override def dataType: DataType = BooleanType
+
+  override def nullable: Boolean = true


when the result can be null?

Ah, I see. After this pr, it won't be null.

cloud-fan · 2019-11-11T15:24:00Z

cc @gengliangwang

SparkQA · 2019-11-11T17:42:32Z

Test build #113591 has finished for PR 26463 at commit 2ab1847.

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

Ngone51 · 2019-11-12T02:32:39Z

...c/main/scala/org/apache/spark/sql/catalyst/expressions/postgreSQL/PostgreCastToBoolean.scala

+import org.apache.spark.sql.catalyst.analysis.TypeCheckResult
+import org.apache.spark.sql.catalyst.expressions.{CastBase, Expression, TimeZoneAwareExpression}
+import org.apache.spark.sql.catalyst.expressions.codegen.Block._
+import org.apache.spark.sql.catalyst.expressions.codegen.JavaCode


TODO: remove this unused import.

SparkQA · 2019-11-12T03:20:46Z

Test build #113606 has finished for PR 26463 at commit 6ad100c.

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

cloud-fan · 2019-11-12T03:25:27Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala

@@ -273,7 +273,7 @@ abstract class CastBase extends UnaryExpression with TimeZoneAwareExpression wit
  private[this] def needsTimeZone: Boolean = Cast.needsTimeZone(child.dataType, dataType)

  // [[func]] assumes the input is no longer null because eval already does the null check.
-  @inline private[this] def buildCast[T](a: Any, func: T => Any): Any = func(a.asInstanceOf[T])
+  @inline private[catalyst] def buildCast[T](a: Any, func: T => Any): Any = func(a.asInstanceOf[T])


change it to private should be good enough?

no, but protected[this] could be fine.

this is minor but just want to make it clear. In PostgreCastToBoolean we call super.castToBoolean, why the modifier of buildCast becomes a problem?

Because case StringType inside PostgreCastToBoolean.castToBoolean() needs to call buildCast.

cloud-fan · 2019-11-12T03:27:04Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala

@@ -791,7 +791,7 @@ abstract class CastBase extends UnaryExpression with TimeZoneAwareExpression wit

  // The function arguments are: `input`, `result` and `resultIsNull`. We don't need `inputIsNull`
  // in parameter list, because the returned code will be put in null safe evaluation region.
-  private[this] type CastFunction = (ExprValue, ExprValue, ExprValue) => Block
+  protected[this] type CastFunction = (ExprValue, ExprValue, ExprValue) => Block


cloud-fan · 2019-11-12T03:27:45Z

...c/main/scala/org/apache/spark/sql/catalyst/expressions/postgreSQL/PostgreCastToBoolean.scala

+    copy(timeZoneId = Option(timeZoneId))
+
+  override def checkInputDataTypes(): TypeCheckResult = child.dataType match {
+    case StringType | IntegerType => super.checkInputDataTypes()


why call super.checkInputDataTypes() here? shall we just return success?

I just want the StringType and IntegerType to do Cast.canCast() check in super.checkInputDataTypes(), though I know it's not necessary.

I'll return success directly.

cloud-fan · 2019-11-12T03:29:35Z

...c/main/scala/org/apache/spark/sql/catalyst/expressions/postgreSQL/PostgreCastToBoolean.scala

+
+  override def dataType: DataType = BooleanType
+
+  override def nullable: Boolean = false


what if child is null? should it be child.nullable?

make sense.

cloud-fan · 2019-11-12T03:31:25Z

sql/core/src/test/scala/org/apache/spark/sql/PostgreSQLDialectQuerySuite.scala

@@ -36,7 +36,8 @@ class PostgreSQLDialectQuerySuite extends QueryTest with SharedSparkSession {
    }

    Seq("o", "abc", "").foreach { input =>
-      checkAnswer(sql(s"select cast('$input' as boolean)"), Row(null))
+      intercept[IllegalArgumentException](
+        checkAnswer(sql(s"select cast('$input' as boolean)"), Row(null)))


since we expect error here, we can just run sql(s"select cast('$input' as boolean)").collect

cloud-fan · 2019-11-12T07:47:15Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala

@@ -273,7 +273,7 @@ abstract class CastBase extends UnaryExpression with TimeZoneAwareExpression wit
  private[this] def needsTimeZone: Boolean = Cast.needsTimeZone(child.dataType, dataType)

  // [[func]] assumes the input is no longer null because eval already does the null check.
-  @inline private[this] def buildCast[T](a: Any, func: T => Any): Any = func(a.asInstanceOf[T])
+  @inline protected[this] def buildCast[T](a: Any, func: T => Any): Any = func(a.asInstanceOf[T])


nit: I don't think [this] here can buy us anything. How about just use protected?

cloud-fan · 2019-11-12T07:48:16Z

...c/main/scala/org/apache/spark/sql/catalyst/expressions/postgreSQL/PostgreCastToBoolean.scala

+case class PostgreCastToBoolean(child: Expression, timeZoneId: Option[String])
+  extends CastBase {
+
+  override protected def ansiEnabled = SQLConf.get.ansiEnabled


to be safe, shall we throw exception here? We don't expect to see this method get called. pgsql dialect should not be affected by the ansi mode config.

cloud-fan · 2019-11-12T07:49:04Z

...c/main/scala/org/apache/spark/sql/catalyst/expressions/postgreSQL/PostgreCastToBoolean.scala

+
+  override def checkInputDataTypes(): TypeCheckResult = child.dataType match {
+    case StringType | IntegerType => TypeCheckResult.TypeCheckSuccess
+    case dt => throw new UnsupportedOperationException(s"cannot cast type $dt to boolean")


We should follow other expressions and return TypeCheckResult.TypeCheckFailure here.

cloud-fan · 2019-11-12T07:50:50Z

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/postgreSQL/CastSuite.scala

-    checkPostgreCastStringToBoolean("abc", null)
-    checkPostgreCastStringToBoolean("", null)
+  test("unsupported data types to cast to boolean") {
+    intercept[Exception](checkPostgreCastToBoolean(new Timestamp(1), null))


We expect to see AnalysisException for these cases.

SparkQA · 2019-11-12T08:33:52Z

Test build #113616 has finished for PR 26463 at commit 0b9a78d.

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

cloud-fan · 2019-11-12T15:30:53Z

...c/main/scala/org/apache/spark/sql/catalyst/expressions/postgreSQL/PostgreCastToBoolean.scala

+    case TimestampType | DateType | LongType | ShortType |
+         ByteType | DecimalType() | DoubleType | FloatType =>
+      TypeCheckResult.TypeCheckFailure(s"cannot cast type ${child.dataType} to boolean")
+    case _ => TypeCheckResult.TypeCheckSuccess


It's simpler and safer to list the supported types, isn't it? For example, binary type can pass the check which is wrong.

cloud-fan · 2019-11-12T15:31:39Z

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/postgreSQL/CastSuite.scala

+  test("unsupported data types to cast to boolean") {
+    intercept[Exception](checkPostgreCastToBoolean(new Timestamp(1), null))
+    intercept[Exception](checkPostgreCastToBoolean(new Date(1), null))
+    intercept[Exception](checkPostgreCastToBoolean(1.toLong, null))


It should be intercept[AnalysisException]...

SparkQA · 2019-11-12T18:24:08Z

Test build #113632 has finished for PR 26463 at commit b7c6baf.

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

Ngone51 · 2019-11-13T06:10:29Z

Filed the flaky test: ImageFileFormatTest.test_read_images

SparkQA · 2019-11-13T08:05:01Z

Test build #113677 has finished for PR 26463 at commit 68c7a13.

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

Ngone51 · 2019-11-13T08:11:58Z

Jenkins, retest this please.

SparkQA · 2019-11-13T11:14:59Z

Test build #113685 has finished for PR 26463 at commit 68c7a13.

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

SparkQA · 2019-11-13T17:48:02Z

Test build #113710 has finished for PR 26463 at commit 999ec61.

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

SparkQA · 2019-11-13T18:10:06Z

Test build #113712 has finished for PR 26463 at commit a178b79.

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

cloud-fan · 2019-11-14T03:55:17Z

thanks, merging to master!

Ngone51 · 2019-11-14T04:59:23Z

Thanks a lot ! @cloud-fan

gatorsmile · 2019-12-05T22:18:41Z

...c/main/scala/org/apache/spark/sql/catalyst/expressions/postgreSQL/PostgreCastToBoolean.scala

+  extends CastBase {
+
+  override protected def ansiEnabled =
+    throw new UnsupportedOperationException("PostgreSQL dialect doesn't support ansi mode")


This looks dangerous. Whenever the code checked the flag ansiEnabled, we will throw an exception. We are removing the pgSQL dialect, this will be updated anyway.

### What changes were proposed in this pull request? Reprocess all PostgreSQL dialect related PRs, listing in order: - #25158: PostgreSQL integral division support [revert] - #25170: UT changes for the integral division support [revert] - #25458: Accept "true", "yes", "1", "false", "no", "0", and unique prefixes as input and trim input for the boolean data type. [revert] - #25697: Combine below 2 feature tags into "spark.sql.dialect" [revert] - #26112: Date substraction support [keep the ANSI-compliant part] - #26444: Rename config "spark.sql.ansi.enabled" to "spark.sql.dialect.spark.ansi.enabled" [revert] - #26463: Cast to boolean support for PostgreSQL dialect [revert] - #26584: Make the behavior of Postgre dialect independent of ansi mode config [keep the ANSI-compliant part] ### Why are the changes needed? As the discussion in http://apache-spark-developers-list.1001551.n3.nabble.com/DISCUSS-PostgreSQL-dialect-td28417.html, we need to remove PostgreSQL dialect form code base for several reasons: 1. The current approach makes the codebase complicated and hard to maintain. 2. Fully migrating PostgreSQL workloads to Spark SQL is not our focus for now. ### Does this PR introduce any user-facing change? Yes, the config `spark.sql.dialect` will be removed. ### How was this patch tested? Existing UT. Closes #26763 from xuanyuanking/SPARK-30125. Lead-authored-by: Yuanjian Li <xyliyuanjian@gmail.com> Co-authored-by: Maxim Gekk <max.gekk@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

postgresql dialect: cast to boolean

75d51ba

Ngone51 added 2 commits November 11, 2019 20:56

fix PostgreSQLDialectQuerySuite

53c9a9b

fix postgreSQL/boolean.sql

2ab1847

Ngone51 changed the title ~~[SPARK-29837][SLQ] PostgreSQL dialect: cast to boolean~~ [SPARK-29837][SQL] PostgreSQL dialect: cast to boolean Nov 11, 2019

cloud-fan reviewed Nov 11, 2019

View reviewed changes

dongjoon-hyun added the SQL label Nov 11, 2019

Ngone51 added 2 commits November 12, 2019 10:08

nullable should be false now

6c2c0c7

check unsupported types in checkInputDataTypes()

6ad100c

Ngone51 commented Nov 12, 2019

View reviewed changes

cloud-fan reviewed Nov 12, 2019

View reviewed changes

Ngone51 added 5 commits November 12, 2019 14:27

private[catalyst] -> protected[this]

101a026

remove unused import

be08daf

return TypeCheckSuccess

95caf40

use collect()

8ebd5aa

use child.nullable

0b9a78d

cloud-fan reviewed Nov 12, 2019

View reviewed changes

Ngone51 added 3 commits November 12, 2019 19:20

excludes unsupported types explicitly

fda48c0

protected[this] -> protected

36f60ee

throw exception for PostgreCastToBoolean.ansiEnabled

b7c6baf

cloud-fan reviewed Nov 12, 2019

View reviewed changes

include supportted types explicitly

b9711ae

adjust tests

68c7a13

cloud-fan approved these changes Nov 13, 2019

View reviewed changes

Ngone51 added 2 commits November 13, 2019 21:46

missing BooleanType

999ec61

leave cast( bool as bool) to optimizer

a178b79

cloud-fan closed this in fe1f456 Nov 14, 2019

xuanyuanking mentioned this pull request Dec 4, 2019

[SPARK-30125][SQL] Remove PostgreSQL dialect #26763

Closed

gatorsmile reviewed Dec 5, 2019

View reviewed changes


		override def dataType: DataType = BooleanType

		override def nullable: Boolean = true


		override def dataType: DataType = BooleanType

		override def nullable: Boolean = false

[SPARK-29837][SQL] PostgreSQL dialect: cast to boolean #26463

[SPARK-29837][SQL] PostgreSQL dialect: cast to boolean #26463

Uh oh!

Conversation

Ngone51 commented Nov 11, 2019

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

SparkQA commented Nov 11, 2019

Uh oh!

Ngone51 commented Nov 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Nov 11, 2019

Uh oh!

SparkQA commented Nov 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 12, 2019

Uh oh!

Ngone51 commented Nov 13, 2019

Uh oh!

SparkQA commented Nov 13, 2019

Uh oh!

Ngone51 commented Nov 13, 2019

Uh oh!

SparkQA commented Nov 13, 2019

Uh oh!

SparkQA commented Nov 13, 2019

Uh oh!

SparkQA commented Nov 13, 2019

Uh oh!

cloud-fan commented Nov 14, 2019

Uh oh!