szehon-ho commented on code in PR #56985:
URL: https://github.com/apache/spark/pull/56985#discussion_r3539911492
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/CreateStreamingTableParserSuite.scala:
##########
@@ -17,17 +17,30 @@
package org.apache.spark.sql.execution.command.v2
+import org.apache.spark.QueryContext
import org.apache.spark.sql.catalyst.analysis.UnresolvedIdentifier
+import org.apache.spark.sql.catalyst.parser.ParseException
import org.apache.spark.sql.catalyst.plans.logical.{CreateStreamingTable,
TableSpec}
+import org.apache.spark.sql.connector.expressions.{ClusterByTransform,
FieldReference, IdentityTransform}
import org.apache.spark.sql.execution.SparkSqlParser
import org.apache.spark.sql.execution.command.v1.CommandSuiteBase
+import org.apache.spark.sql.types.IntegerType
/**
* The class contains tests for the `CREATE STREAMING TABLE ... AS ...` command
Review Comment:
Nit (pre-existing): this class comment says `CREATE STREAMING TABLE ... AS
...`, but this suite tests the **no-AS** form (`CreateStreamingTable`, not
`CreateStreamingTableAsSelect`). Consider updating to something like:
> The class contains tests for the `CREATE STREAMING TABLE ...` command
(without a subquery).
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/CreateStreamingTableParserSuite.scala:
##########
@@ -17,17 +17,30 @@
package org.apache.spark.sql.execution.command.v2
+import org.apache.spark.QueryContext
import org.apache.spark.sql.catalyst.analysis.UnresolvedIdentifier
+import org.apache.spark.sql.catalyst.parser.ParseException
import org.apache.spark.sql.catalyst.plans.logical.{CreateStreamingTable,
TableSpec}
+import org.apache.spark.sql.connector.expressions.{ClusterByTransform,
FieldReference, IdentityTransform}
import org.apache.spark.sql.execution.SparkSqlParser
import org.apache.spark.sql.execution.command.v1.CommandSuiteBase
+import org.apache.spark.sql.types.IntegerType
/**
* The class contains tests for the `CREATE STREAMING TABLE ... AS ...` command
*/
class CreateStreamingTableParserSuite extends CommandSuiteBase {
protected lazy val parser = new SparkSqlParser()
+ /** Turn an actual QueryContext into an equivalent ExpectedContext to no-op
the check. */
+ private def toExpectedContext(actual: QueryContext): ExpectedContext =
ExpectedContext(
Review Comment:
Nit: this duplicates `toExpectedContext` already defined in
`CreatePipelineDatasetAsSelectParserSuiteBase` (as a `def` at the bottom of
that trait). Fine as-is for this PR; if more pipeline-dataset parser suites
appear, consider hoisting the helper to `CommandSuiteBase` or a small shared
test utility.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/CreateStreamingTableParserSuite.scala:
##########
@@ -54,4 +67,168 @@ class CreateStreamingTableParserSuite extends
CommandSuiteBase {
checkAnalysis = false
)
}
+
+ test("CREATE STREAMING TABLE - PARTITIONED BY is honored") {
+ val cmd = parser
+ .parsePlan("CREATE STREAMING TABLE st PARTITIONED BY (a)")
+ .asInstanceOf[CreateStreamingTable]
+ assert(cmd.partitioning ==
Seq(IdentityTransform(FieldReference(Seq("a")))))
+ }
+
+ test("CREATE STREAMING TABLE - COMMENT is honored") {
Review Comment:
Nit: this is partially redundant with the existing `comparePlans` test
above, which already verifies `COMMENT` parsing. Not a blocker — the explicit
clause test is consistent with the style of the other new tests — but you could
drop this one if you want to avoid duplication.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/CreatePipelineDatasetAsSelectParserSuiteBase.scala:
##########
@@ -86,6 +86,18 @@ trait CreatePipelineDatasetAsSelectParserSuiteBase extends
CommandSuiteBase {
}
}
+ test("Clustering is correctly parsed") {
Review Comment:
Nice addition — putting `CLUSTER BY` here correctly covers both
`CreateStreamingTableAsSelectParserSuite` and
`CreateMaterializedViewAsSelectParserSuite`. (Alternatively this could extend
the existing `"Partitioning is correctly parsed"` test, but a separate test is
clearer since `CLUSTER BY` and `PARTITIONED BY` produce different transform
types.)
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/CreateStreamingTableParserSuite.scala:
##########
@@ -54,4 +67,168 @@ class CreateStreamingTableParserSuite extends
CommandSuiteBase {
checkAnalysis = false
)
}
+
+ test("CREATE STREAMING TABLE - PARTITIONED BY is honored") {
Review Comment:
Nit: minor naming inconsistency with the AS SELECT base suite, which uses
names like `"Partitioning is correctly parsed"` and `"Collation is correctly
parsed"`. These new tests use `"... is honored"`. Either convention is fine;
aligning would make the suite feel more uniform.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/CreateStreamingTableParserSuite.scala:
##########
@@ -54,4 +67,168 @@ class CreateStreamingTableParserSuite extends
CommandSuiteBase {
checkAnalysis = false
)
}
+
+ test("CREATE STREAMING TABLE - PARTITIONED BY is honored") {
+ val cmd = parser
+ .parsePlan("CREATE STREAMING TABLE st PARTITIONED BY (a)")
+ .asInstanceOf[CreateStreamingTable]
+ assert(cmd.partitioning ==
Seq(IdentityTransform(FieldReference(Seq("a")))))
+ }
+
+ test("CREATE STREAMING TABLE - COMMENT is honored") {
+ val cmd = parser
+ .parsePlan("CREATE STREAMING TABLE st COMMENT 'my streaming table'")
+ .asInstanceOf[CreateStreamingTable]
+ assert(cmd.tableSpec.comment == Some("my streaming table"))
+ }
+
+ test("CREATE STREAMING TABLE - TBLPROPERTIES are honored") {
+ val cmd = parser
+ .parsePlan("CREATE STREAMING TABLE st TBLPROPERTIES ('key' = 'value',
'num' = '1')")
+ .asInstanceOf[CreateStreamingTable]
+ assert(cmd.tableSpec.properties == Map("key" -> "value", "num" -> "1"))
+ }
+
+ test("CREATE STREAMING TABLE - CLUSTER BY is honored") {
+ val cmd = parser
+ .parsePlan("CREATE STREAMING TABLE st CLUSTER BY (a)")
+ .asInstanceOf[CreateStreamingTable]
+ assert(cmd.partitioning ==
Seq(ClusterByTransform(Seq(FieldReference(Seq("a"))))))
+ }
+
+ test("CREATE STREAMING TABLE - USING provider is honored") {
+ val cmd = parser
+ .parsePlan("CREATE STREAMING TABLE st USING parquet")
+ .asInstanceOf[CreateStreamingTable]
+ assert(cmd.tableSpec.provider == Some("parquet"))
+ }
+
+ test("CREATE STREAMING TABLE - DEFAULT COLLATION is honored") {
+ val cmd = parser
+ .parsePlan("CREATE STREAMING TABLE st DEFAULT COLLATION UTF8_LCASE")
+ .asInstanceOf[CreateStreamingTable]
+ assert(cmd.tableSpec.collation == Some("UTF8_LCASE"))
+ }
+
+ test("CREATE STREAMING TABLE - column list is honored") {
+ val cmd = parser
+ .parsePlan("CREATE STREAMING TABLE st (id INT, name STRING)")
+ .asInstanceOf[CreateStreamingTable]
+ assert(cmd.columns.map(_.name) == Seq("id", "name"))
+ assert(cmd.columns.head.dataType == IntegerType)
Review Comment:
Nit: this only asserts the first column's type (`IntegerType`). Consider
also checking `cmd.columns(1).dataType == StringType` (or similar) so both
columns' types are verified. Low risk since column parsing is covered more
thoroughly in `CreatePipelineDatasetAsSelectParserSuiteBase`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
