Skip to content
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
adb1842
[SPARK-33474][SQL] Support TypeConstructed partition spec value
AngersZhuuuu Nov 19, 2020
ae59115
Update AstBuilder.scala
AngersZhuuuu Nov 19, 2020
c9a97d0
Update AstBuilder.scala
AngersZhuuuu Nov 19, 2020
bcdc7e5
Update SQLQuerySuite.scala
AngersZhuuuu Nov 19, 2020
d171377
FOLLOW COMMENT
AngersZhuuuu Nov 24, 2020
516c070
Update SQLQuerySuite.scala
AngersZhuuuu Nov 24, 2020
251c36b
Update DDLParserSuite.scala
AngersZhuuuu Nov 24, 2020
1590e8a
Merge branch 'master' into SPARK-33474
AngersZhuuuu Nov 24, 2020
6adefa7
Update DDLParserSuite.scala
AngersZhuuuu Nov 24, 2020
05f1962
Update SQLQuerySuite.scala
AngersZhuuuu Nov 24, 2020
e312697
follow comment
AngersZhuuuu Nov 24, 2020
edd270e
Update AstBuilder.scala
AngersZhuuuu Nov 24, 2020
a8f26a1
follow comment
AngersZhuuuu Nov 25, 2020
98986a0
Update AstBuilder.scala
AngersZhuuuu Nov 25, 2020
e2749c3
Update sql-migration-guide.md
AngersZhuuuu Nov 25, 2020
713da66
follow comment
AngersZhuuuu Nov 27, 2020
4898fb4
follow comment
AngersZhuuuu Nov 29, 2020
e875bcf
Update SQLQuerySuite.scala
AngersZhuuuu Nov 29, 2020
0228292
Update SQLQuerySuite.scala
AngersZhuuuu Nov 30, 2020
2db3f14
Merge branch 'master' into SPARK-33474
AngersZhuuuu Dec 1, 2020
055a903
Update
AngersZhuuuu Dec 4, 2020
e6092c1
Merge branch 'master' into SPARK-33474
AngersZhuuuu Dec 9, 2020
06a9321
Update DDLParserSuite.scala
AngersZhuuuu Dec 9, 2020
bc3e347
Merge branch 'master' into SPARK-33474
AngersZhuuuu Dec 21, 2020
0358274
Merge branch 'master' into SPARK-33474
AngersZhuuuu Dec 24, 2020
ca69533
Merge branch 'SPARK-33474' of https://github.com/AngersZhuuuu/spark i…
AngersZhuuuu Dec 24, 2020
894ac90
Merge branch 'master' into SPARK-33474
AngersZhuuuu Feb 20, 2021
0b4b211
update
AngersZhuuuu Feb 20, 2021
7264a3d
Update DDLParserSuite.scala
AngersZhuuuu Feb 20, 2021
b68ee81
Fix build issue
AngersZhuuuu Feb 20, 2021
dc0783a
Update sql-migration-guide.md
AngersZhuuuu Feb 20, 2021
d3b1960
Update AstBuilder.scala
AngersZhuuuu Feb 20, 2021
2e9058a
follow comment
AngersZhuuuu Feb 24, 2021
31c1092
Merge branch 'master' into SPARK-33474
AngersZhuuuu Feb 24, 2021
9f5d569
Update SQLQuerySuite.scala
AngersZhuuuu Feb 24, 2021
5e05adb
follow comment
AngersZhuuuu Feb 26, 2021
dd34027
follow comment
AngersZhuuuu Feb 26, 2021
4cdb4a7
Update AlterTableDropPartitionSuiteBase.scala
AngersZhuuuu Feb 26, 2021
093b062
Update SQLInsertTestSuite.scala
AngersZhuuuu Feb 27, 2021
adc3984
Merge branch 'master' into SPARK-33474
AngersZhuuuu Mar 1, 2021
f6bdbbe
follow comment
AngersZhuuuu Mar 1, 2021
2ef0fd0
Update SQLInsertTestSuite.scala
AngersZhuuuu Mar 1, 2021
4023041
Update SQLInsertTestSuite.scala
AngersZhuuuu Mar 1, 2021
fe5095a
Update SQLInsertTestSuite.scala
AngersZhuuuu Mar 1, 2021
63a4fb4
follow comment
AngersZhuuuu Mar 2, 2021
4f61e60
follow comment
AngersZhuuuu Mar 2, 2021
08c55f6
Update SQLInsertTestSuite.scala
AngersZhuuuu Mar 2, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ import org.apache.spark.sql.catalyst.expressions.aggregate.{First, Last}
import org.apache.spark.sql.catalyst.parser.SqlBaseParser._
import org.apache.spark.sql.catalyst.plans._
import org.apache.spark.sql.catalyst.plans.logical._
import org.apache.spark.sql.catalyst.util.{DateFormatter, IntervalUtils, TimestampFormatter}
import org.apache.spark.sql.catalyst.util.DateTimeUtils.{getZoneId, stringToDate, stringToTimestamp}
import org.apache.spark.sql.catalyst.util.IntervalUtils
import org.apache.spark.sql.catalyst.util.IntervalUtils.IntervalUnit
import org.apache.spark.sql.connector.catalog.{SupportsNamespaces, TableCatalog}
import org.apache.spark.sql.connector.catalog.TableChange.ColumnPosition
Expand Down Expand Up @@ -503,13 +503,32 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with SQLConfHelper with Logg
}
}

def convertTypeConstructedLiteralToString(literal: Literal): String = literal match {
case Literal(data: Int, dataType: DateType) =>
UTF8String.fromString(
Copy link
Member

Choose a reason for hiding this comment

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

Could you clarify why do you need the conversion: String -> UTF8String -> String

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you clarify why do you need the conversion: String -> UTF8String -> String

Forgot where I see similar logical, emmm, so this place don't need this convert to keep standard charset UTF8

DateFormatter(getZoneId(SQLConf.get.sessionLocalTimeZone))
.format(data)).toString
case Literal(data: Long, dataType: TimestampType) =>
UTF8String.fromString(
TimestampFormatter.getFractionFormatter(getZoneId(SQLConf.get.sessionLocalTimeZone))
.format(data)).toString
case Literal(data: CalendarInterval, dataType: CalendarIntervalType) =>
UTF8String.fromString(data.toString).toString
case Literal(data: Array[Byte], dataType: BinaryType) =>
UTF8String.fromBytes(data).toString
case Literal(data, dataType) =>
UTF8String.fromString(data.toString).toString
Copy link
Member

Choose a reason for hiding this comment

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

We need this entry? What's an example to match this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need this entry? What's an example to match this case?

Normally this won't happen, just remove this match case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need this entry? What's an example to match this case?

removed this line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add back...Seems I add this case avoid build problem.

[error] /home/jenkins/workspace/SparkPullRequestBuilder@3/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:506:81: Exhaustivity analysis reached max recursion depth, not all missing cases are reported.
[error] (Please try with scalac -Ypatmat-exhaust-depth 40 or -Ypatmat-exhaust-depth off.)
[error]   private def convertTypeConstructedLiteralToString(literal: Literal): String = literal match {
[error]                                                                                 ^
[error] one error found

Copy link
Member

Choose a reason for hiding this comment

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

Yea, that's the effect of the recent compiler update: #30455

Copy link
Member

Choose a reason for hiding this comment

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

how about throwing an illegal state exception in the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how about throwing an illegal state exception in the case?

Good suggestion , updated.

Yea, that's the effect of the recent compiler update: #30455

Thanks for point out this pr.

}

/**
* Convert a constant of any type into a string. This is typically used in DDL commands, and its
* main purpose is to prevent slight differences due to back to back conversions i.e.:
* String -> Literal -> String.
*/
protected def visitStringConstant(ctx: ConstantContext): String = withOrigin(ctx) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we support all the literals here? e.g.

expression(ctx) match {
  case l: Litral => if (l.value == null) null else Cast(l, StringType, timezone).eval()
  case _ => fail... // this should never happen
}

Copy link
Contributor Author

@AngersZhuuuu AngersZhuuuu Dec 1, 2020

Choose a reason for hiding this comment

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

can we support all the literals here? e.g.

expression(ctx) match {
  case l: Litral => if (l.value == null) null else Cast(l, StringType, timezone).eval()
  case _ => fail... // this should never happen
}

your mean support all constant here

constant
    : NULL                                                                                     #nullLiteral
    | interval                                                                                 #intervalLiteral
    | identifier STRING                                                                        #typeConstructor
    | number                                                                                   #numericLiteral
    | booleanValue                                                                             #booleanLiteral
    | STRING+                                                                                  #stringLiteral
    ;

and use expression to visit this constant then we don't need so many convert logic.
In this way we should add a new method such as :

  protected def visitConstant(ctx: ConstantContext): String = withOrigin(ctx) {
    expression(ctx) match {
      case l: Literal => if (l.value == null) {
        null
      } else {
        Cast(l, StringType, Some(SQLConf.get.sessionLocalTimeZone)).eval()
      }
      case _ => 
        throw IllegalArgumentException("")
    }
  }

I have tried similar way before, but Cast.eval can't run here.
I have tried change CastBase code and extract cast functions as Half-life object class object CastBase. Then call CastBase.castToString(literal).

IMO, we don't need to expand the scope of the modification, but your suggestion is pretty good when we change partition spec to Map[String, Literal].

Copy link
Contributor

@cloud-fan cloud-fan Dec 1, 2020

Choose a reason for hiding this comment

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

... but Cast.eval can't run here.

Why? What's the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... but Cast.eval can't run here.

Why? What's the error?

SInce eval() need input, you mean write it like this ?

  protected def visitConstant(ctx: ConstantContext): String = withOrigin(ctx) {
    expression(ctx) match {
      case l: Literal => if (l.value == null) {
        null
      } else {
        Cast(l, StringType, Some(SQLConf.get.sessionLocalTimeZone))
          .eval(InternalRow.apply(l.value)).toString
      }
      case _ =>
        throw IllegalArgumentException("")
    }
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hwo about current change?

ctx match {
case l: TypeConstructorContext =>
convertTypeConstructedLiteralToString(visitTypeConstructor(l))
case s: StringLiteralContext => createString(s)
case o => o.getText
Copy link
Contributor

Choose a reason for hiding this comment

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

previous, date'xxx' is just a string "date'xxx'"? then we need a migration guide for this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

previous, date'xxx' is just a string "date'xxx'"? then we need a migration guide for this change.

Updated

}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2584,6 +2584,14 @@ abstract class SQLQuerySuiteBase extends QueryTest with SQLTestUtils with TestHi
}
}
}

test("SPARK-33474: Support TypeConstructed partition spec value") {
Copy link
Member

Choose a reason for hiding this comment

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

Why did you place the test in sql/hive? Are your changes hive specific?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why did you place the test in sql/hive? Are your changes hive specific?

No , moved to sql module

withTable("t") {
sql("CREATE TABLE t(name STRING) PARTITIONED BY (part DATE) STORED AS ORC")
Copy link
Member

@maropu maropu Nov 24, 2020

Choose a reason for hiding this comment

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

Please add more tests for the other types and add tests in DDLParserSuite.

Copy link
Member

@maropu maropu Nov 24, 2020

Choose a reason for hiding this comment

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

btw, why did you use stored as orc explicitly for this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

btw, why did you use stored as orc explicitly for this test?

removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please add more tests for the other types and add tests in DDLParserSuite.

Both updated

sql("INSERT INTO t PARTITION(part = date'2019-01-02') VALUES('a')")
checkAnswer(sql("SELECT name, CAST(part AS STRING) FROM t"), Row("a", "2019-01-02"))
}
}
}

@SlowHiveTest
Expand Down