[SPARK-42131][SQL] Extract the function that construct the select statement for JDBC dialect. #39667

beliefer · 2023-01-20T09:13:30Z

What changes were proposed in this pull request?

Currently, JDBCRDD uses fixed format for SELECT statement.

val sqlText = options.prepareQuery +
      s"SELECT $columnList FROM ${options.tableOrQuery} $myTableSampleClause" +
      s" $myWhereClause $getGroupByClause $getOrderByClause $myLimitClause $myOffsetClause"

But some databases have different syntax. For example, MS SQL Server uses keyword TOP to describe LIMIT clause or Top N.
The LIMIT clause of MS SQL Server show below.

SELECT TOP(1) Model, Color, Price  
      FROM dbo.Cars  
      WHERE Color = 'blue'

The Top N of MS SQL Server show below.

SELECT TOP(1) Model, Color, Price  
FROM dbo.Cars  
WHERE Color = 'blue'  
ORDER BY Price ASC

This PR lets JDBC dialect could define their own syntax.

Why are the changes needed?

Extract the function that construct the select statement for JDBC dialect.

Does this PR introduce any user-facing change?

'No'.
New feature.

How was this patch tested?

N/A

sadikovi · 2023-01-22T22:22:11Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala

Do we have unit tests that cover it? If not, maybe it is a good time to add them to make sure the string is generated correctly.

There are already a lot of unit tests.

Could you list them? I could not find them when I was trying to add a unit test.

Almost all tests of JDBCSuite or JDBCV2Suite will call getSQLText.

I don't think this is accurate, those tests only run against H2 database, there are no unit tests that verify correctness for other dialects. Moreover, there are no tests at all to check the correctness of the generated SQL string.

sadikovi · 2023-01-29T22:09:48Z

Again, I don't see how this makes it better than simple changes in #39660. If the aim of this PR to improve JdbcDialects, then I think we should have a small design doc + make the API extensible. For example, adding TOP creates a question whether to add another parameter to getSQLText or silently overwrite it in the dialect.

beliefer · 2023-01-30T02:12:53Z

whether to add another parameter to getSQLText or silently overwrite it in the dialect.

We could think TOP n as the special LIMIT n.

cloud-fan · 2023-01-30T02:49:41Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala

The problem with this approach is we need to change it every time we add a new feature (think about the table sample pushdown).

JdbcSQLBuilder has been created.

beliefer · 2023-01-31T08:39:00Z

ping @cloud-fan @huaxingao cc @sadikovi

cloud-fan · 2023-01-31T08:57:34Z

@beliefer can you implement a MsSqlServerJdbcSQLBuilder, override withLimit to implement the TOP N feature?

…tement for JDBC dialect.

cloud-fan · 2023-02-01T14:50:56Z

...egration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MsSqlServerIntegrationSuite.scala

    assert(msg.contains("UpdateColumnNullability is not supported"))
  }

+  test("simple scan with LIMIT") {


shall we move the tests to the parent test suite?

cloud-fan · 2023-02-01T14:52:00Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/MsSqlServerDialect.scala

+        case (SortDirection.ASCENDING, NullOrdering.NULLS_FIRST) =>
+          s"$sortKey $sortDirection"
+        case (SortDirection.ASCENDING, NullOrdering.NULLS_LAST) =>
+          s"CASE WHEN $sortKey IS NULL THEN 1 ELSE 0 END, $sortKey $sortDirection"


is this a standard way to do NULLS LAST in mssql?

It's not a standard way and just is a little skill.

seems like this is a common workaround: https://dba-presents.com/databases/sql-server/36-order-by-and-nulls-last-in-sql-server

huaxingao · 2023-02-02T03:38:42Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcSQLQueryBuilder.scala

+/**
+ * The builder to generate jdbc sql query.
+ *
+ * @since 3.4.0


Is this for 3.4.0?

I'm not sure.

This fixes a flaw in the jdbc dialect API. It would be great to merge it before 3.4 rc cut, if there is no objection.

After a second thought, people may find problems of this API after using is more. Let's merge it to master branch only and change it to 3.5.0

cloud-fan · 2023-02-02T04:16:54Z

connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/V2JDBCTest.scala

  val namespaceOpt: Option[String] = None

-  private def catalogAndNamespace =
+  protected def catalogAndNamespace =


unnecessary change?

sadikovi

Do we have any mechanism to ensure that any builder that overrides build method needs to be updated if a new feature is introduced?

sadikovi · 2023-02-02T04:16:54Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcSQLQueryBuilder.scala

+/**
+ * The builder to generate jdbc sql query.
+ *
+ * @since 3.4.0


Can we remove this?

sadikovi · 2023-02-02T04:19:26Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/MsSqlServerDialect.scala

+
+  class MsSqlServerSQLQueryBuilder(dialect: JdbcDialect, options: JDBCOptions)
+    extends JdbcSQLQueryBuilder(dialect, options) {
+    override def build(): String = {


nit: new line before the method.

It's not necessary but is OK too.

sadikovi · 2023-02-02T04:21:16Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/MsSqlServerDialect.scala


  override def getLimitClause(limit: Integer): String = {
-    ""
+    if (limit > 0 ) s"TOP $limit" else ""


Isn't limit = 0 valid?

Spark optimizer returns empty relation directly.

sadikovi · 2023-02-02T04:24:07Z

Why can we not introduce getTopNExpression to the query builder? Although it acts like limit the semantics are slightly different.

sadikovi · 2023-02-02T04:25:14Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala

  }

+  /**
+   * returns the SQL builder for the SELECT statement.


nit: Returns ...

sadikovi · 2023-02-02T04:25:43Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcSQLQueryBuilder.scala

+    this
+  }
+
+  def withGroupByColumns(groupByColumns: Option[Array[String]]): JdbcSQLQueryBuilder = {


Please add javadoc for the API methods to explain the purpose and the input.

cloud-fan · 2023-02-02T04:31:51Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcSQLQueryBuilder.scala

+import org.apache.spark.sql.execution.datasources.v2.TableSampleInfo
+
+/**
+ * The builder to generate jdbc sql query.


We need a bit more API doc. This builder is to build a single SELECT query, so all the withXXX methods will be invoked at most once. The invocation order does not matter, as all these clauses follow the natural SQL order: sample the table first, then filter, then group by, then sort, then offset, then limit.

cc @sadikovi , this also means we don't need a dedicate topN API, If both sort and limit are present, it means top N. This is also how SQL language represents top N.

cloud-fan · 2023-02-06T11:31:37Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcSQLQueryBuilder.scala

+   */
+  protected var tableSampleClause: String = ""
+
+  def withColumns(columns: Array[String]): JdbcSQLQueryBuilder = {


can we add api doc for each with method?

like if the column name is the raw name or quoted name following dialect's SQL syntax?

cloud-fan · 2023-02-06T11:33:10Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcSQLQueryBuilder.scala

+    this
+  }
+
+  def withTableSample(sample: Option[TableSampleInfo]): JdbcSQLQueryBuilder = {


I don't think the parameter type should be Option. Spark should not invoke withTableSample at all if there is no table sample in the query.

cloud-fan · 2023-02-06T11:34:40Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/MySQLDialect.scala

    supportedFunctions.contains(funcName)

  class MySQLSQLBuilder extends JDBCSQLBuilder {
+    override def visitSortOrder(


So when we push down top N to mysql before this PR, it throws syntax error?

Yes. Many dialects cannot treat it correctly. The reason is MySQL doesn't support NullOrdering.

sadikovi

Looks good. I left a few questions.
What is the ETA to merge this PR?

sadikovi · 2023-02-06T22:56:08Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/OracleDialect.scala

+  override def getLimitClause(limit: Integer): String = {
+    // Oracle doesn't support LIMIT clause.
+    // We can use rownum <= n to limit the number of rows in the result set.
+    if (limit > 0 ) s"WHERE rownum <= $limit" else ""


nit: no space after 0.

sadikovi · 2023-02-06T22:56:38Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/OracleDialect.scala

+    override def build(): String = {
+      val selectStmt = s"SELECT $columnList FROM ${options.tableOrQuery} $tableSampleClause" +
+        s" $whereClause $groupByClause $orderByClause"
+      if (limit > 0) {


Thanks for fixing the limit. Could you add a test for Oracle JDBC limit? Those integration tests don't test limit, otherwise we would have encountered this issue before.

The two test case simple scan with LIMIT and simple scan with top N are in the parent trait.

cloud-fan · 2023-02-07T02:41:53Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcSQLQueryBuilder.scala

+
+  /**
+   * The columns names that following dialect's SQL syntax.
+   * e.g. The column name is the raw name or quoted name.


so it's the raw name string or quoted name?

From API's perspective, I think it's better to pass raw name string like a#b instead of 'a#b'. And the implementation should call dialect API to quote it. We can change it later as this PR will be master branch only.

OK. It seems need more change.

cloud-fan · 2023-02-07T02:42:29Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcSQLQueryBuilder.scala

+  /**
+   * Constructs the GROUP BY clause that following dialect's SQL syntax.
+   */
+  def withGroupByColumns(groupByColumns: Option[Array[String]]): JdbcSQLQueryBuilder = {


Same here, the with method should take an optional parameter.

cloud-fan · 2023-02-07T02:42:46Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcSQLQueryBuilder.scala

+    this
+  }
+
+  def build(): String = {


api doc here as well.

cloud-fan · 2023-02-07T02:44:27Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/MsSqlServerDialect.scala

+  class MsSqlServerSQLQueryBuilder(dialect: JdbcDialect, options: JDBCOptions)
+    extends JdbcSQLQueryBuilder(dialect, options) {
+
+    // TODO[SPARK-42289]: DS V2 pushdown could let JDBC dialect decide to push down offset


what does this TODO mean?

Spark force push down offset to databases, but some databases do not support offset.

cloud-fan · 2023-02-07T12:37:10Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRDD.scala


-    val myLimitClause: String = dialect.getLimitClause(limit)
-    val myOffsetClause: String = dialect.getOffsetClause(offset)
+    groupByColumns.foreach(builder.withGroupByColumns)


withXXX returns a new builder. We can's assume the implementation always returns this

let's do

var builder = ... groupByColumns.foreach { groupBy => builder = ... }

cloud-fan · 2023-02-08T14:17:18Z

thanks, merging to master!

beliefer · 2023-02-09T01:18:37Z

@cloud-fan @sadikovi Thank you!

github-actions bot added the SQL label Jan 20, 2023

beliefer mentioned this pull request Jan 20, 2023

[SPARK-42128][SQL] Support TOP (N) for MS SQL Server dialect as an alternative to Limit pushdown #39660

Closed

sadikovi reviewed Jan 22, 2023

View reviewed changes

cloud-fan reviewed Jan 30, 2023

View reviewed changes

beliefer added 9 commits February 1, 2023 20:34

[SPARK-42131][SQL] Extract the function that construct the select sta…

ffc4dde

…tement for JDBC dialect.

Update code

af6594b

Update code

7efe032

create JdbcSQLBuilder

b99d877

Update code

2a75bf2

Update code

9351495

Update code

8729bf8

MsSqlServerDialect supports limit and top n

950279d

Update code

a643743

beliefer force-pushed the SPARK-42131 branch from cec80c4 to a643743 Compare February 1, 2023 13:08

cloud-fan reviewed Feb 1, 2023

View reviewed changes

huaxingao reviewed Feb 2, 2023

View reviewed changes

Update code

87dc32c

cloud-fan reviewed Feb 2, 2023

View reviewed changes

sadikovi reviewed Feb 2, 2023

View reviewed changes

cloud-fan reviewed Feb 2, 2023

View reviewed changes

beliefer added 2 commits February 2, 2023 17:00

Update code

15a57f7

Update code

67ce69e

beliefer added 2 commits February 2, 2023 22:26

Update code

7d85701

Update code

d1c9124

cloud-fan reviewed Feb 6, 2023

View reviewed changes

Update code

04a56c8

sadikovi reviewed Feb 6, 2023

View reviewed changes

Update code

42f2c65

sadikovi approved these changes Feb 7, 2023

View reviewed changes

Update code

41ab6bd

cloud-fan reviewed Feb 7, 2023

View reviewed changes

beliefer added 2 commits February 7, 2023 11:46

Update code

d674b80

Update code

1b3571c

cloud-fan reviewed Feb 7, 2023

View reviewed changes

cloud-fan approved these changes Feb 7, 2023

View reviewed changes

Update code

06b6314

cloud-fan closed this in 060a2b8 Feb 8, 2023

[SPARK-42131][SQL] Extract the function that construct the select statement for JDBC dialect. #39667

[SPARK-42131][SQL] Extract the function that construct the select statement for JDBC dialect. #39667

Uh oh!

Conversation

beliefer commented Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

beliefer Jan 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sadikovi commented Jan 29, 2023

Uh oh!

beliefer commented Jan 30, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beliefer commented Jan 31, 2023

Uh oh!

cloud-fan commented Jan 31, 2023

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!

sadikovi left a comment

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!

beliefer Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

sadikovi commented Feb 2, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beliefer commented Jan 20, 2023 •

edited

Loading

beliefer Jan 30, 2023 •

edited

Loading

beliefer Feb 2, 2023 •

edited

Loading

beliefer Feb 6, 2023 •

edited

Loading