You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "beliefer (via GitHub)" <gi...@apache.org> on 2023/02/09 13:35:22 UTC
[GitHub] [spark] beliefer opened a new pull request, #39954: [SPARK-42289][SQL] DS V2 pushdown could let JDBC dialect decide to push down offset and limit
beliefer opened a new pull request, #39954:
URL: https://github.com/apache/spark/pull/39954
### What changes were proposed in this pull request?
Currently, DS V2 pushdown force push down LIMIT clause and OFFSET clause to underlying databases. The behavior is not correct. For example, Oracle and MsSQLServer doesn't support LIMIT clause. DS V2 pushdown could let JDBC dialect decide to push down offset and limit.
### Why are the changes needed?
1. If the underlying database don't support LIMIT clause or OFFSET clause, the corresponding dialect could rejects push down them.
2. Although some underlying database don't support LIMIT clause or OFFSET clause, the corresponding dialect have a lot of trick to finish the same function.
### Does this PR introduce _any_ user-facing change?
'No'.
New feature.
### How was this patch tested?
New test cases.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on a diff in pull request #39954: [SPARK-42289][SQL] DS V2 pushdown could let JDBC dialect decide to push down offset and limit
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #39954:
URL: https://github.com/apache/spark/pull/39954#discussion_r1113789959
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala:
##########
@@ -557,6 +557,20 @@ abstract class JdbcDialect extends Serializable with Logging {
def getJdbcSQLQueryBuilder(options: JDBCOptions): JdbcSQLQueryBuilder =
new JdbcSQLQueryBuilder(this, options)
+ /**
+ * Returns ture if dialect supports LIMIT clause.
+ *
+ * Note: Some build-in dialect supports LIMIT clause with some trick, please see:
+ * {@link OracleDialect.OracleSQLQueryBuilder} and
+ * {@link MsSqlServerDialect.MsSqlServerSQLQueryBuilder}.
+ */
+ def supportsLimit: Boolean = true
+
+ /**
+ * Returns ture if dialect supports OFFSET clause.
+ */
+ def supportsOffset: Boolean = true
Review Comment:
> BTW you still have this `TODO[SPARK-42289]: DS V2 pushdown could let JDBC dialect decide to push down offset` in `OracleDialect` and `MsSqlServerDialect`
Yes. It need another PR. This PR just open the API.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #39954: [SPARK-42289][SQL] DS V2 pushdown could let JDBC dialect decide to push down offset and limit
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #39954:
URL: https://github.com/apache/spark/pull/39954#discussion_r1112636420
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/jdbc/JDBCScanBuilder.scala:
##########
@@ -135,15 +135,16 @@ case class JDBCScanBuilder(
}
override def pushLimit(limit: Int): Boolean = {
- if (jdbcOptions.pushDownLimit) {
+ if (jdbcOptions.pushDownLimit && JdbcDialects.get(jdbcOptions.url).supportsLimit) {
Review Comment:
shall we create a dialect instance as a field of `JDBCScanBuilder`?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #39954: [SPARK-42289][SQL] DS V2 pushdown could let JDBC dialect decide to push down offset and limit
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #39954:
URL: https://github.com/apache/spark/pull/39954#discussion_r1116462115
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala:
##########
@@ -557,6 +557,20 @@ abstract class JdbcDialect extends Serializable with Logging {
def getJdbcSQLQueryBuilder(options: JDBCOptions): JdbcSQLQueryBuilder =
new JdbcSQLQueryBuilder(this, options)
+ /**
+ * Returns ture if dialect supports LIMIT clause.
+ *
+ * Note: Some build-in dialect supports LIMIT clause with some trick, please see:
+ * {@link OracleDialect.OracleSQLQueryBuilder} and
+ * {@link MsSqlServerDialect.MsSqlServerSQLQueryBuilder}.
+ */
+ def supportsLimit: Boolean = true
+
+ /**
+ * Returns ture if dialect supports OFFSET clause.
+ */
+ def supportsOffset: Boolean = true
Review Comment:
We can do it in the PR switching the default value of JDBC pushdown options.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #39954: [SPARK-42289][SQL] DS V2 pushdown could let JDBC dialect decide to push down offset and limit
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #39954:
URL: https://github.com/apache/spark/pull/39954#discussion_r1116461015
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala:
##########
@@ -557,6 +557,20 @@ abstract class JdbcDialect extends Serializable with Logging {
def getJdbcSQLQueryBuilder(options: JDBCOptions): JdbcSQLQueryBuilder =
new JdbcSQLQueryBuilder(this, options)
+ /**
+ * Returns ture if dialect supports LIMIT clause.
+ *
+ * Note: Some build-in dialect supports LIMIT clause with some trick, please see:
+ * {@link OracleDialect.OracleSQLQueryBuilder} and
+ * {@link MsSqlServerDialect.MsSqlServerSQLQueryBuilder}.
+ */
+ def supportsLimit: Boolean = true
+
+ /**
+ * Returns ture if dialect supports OFFSET clause.
+ */
+ def supportsOffset: Boolean = true
Review Comment:
I think it's better to use false as the default value, and override it in builtin dialects in return true.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on pull request #39954: [SPARK-42289][SQL] DS V2 pushdown could let JDBC dialect decide to push down offset and limit
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #39954:
URL: https://github.com/apache/spark/pull/39954#issuecomment-1425025059
ping @huaxingao cc @cloud-fan
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #39954: [SPARK-42289][SQL] DS V2 pushdown could let JDBC dialect decide to push down offset and limit
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #39954:
URL: https://github.com/apache/spark/pull/39954#issuecomment-1442736842
thanks, merging to master!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #39954: [SPARK-42289][SQL] DS V2 pushdown could let JDBC dialect decide to push down offset and limit
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #39954:
URL: https://github.com/apache/spark/pull/39954#discussion_r1114255805
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/jdbc/JDBCScanBuilder.scala:
##########
@@ -126,24 +126,23 @@ case class JDBCScanBuilder(
upperBound: Double,
withReplacement: Boolean,
seed: Long): Boolean = {
- if (jdbcOptions.pushDownTableSample &&
- JdbcDialects.get(jdbcOptions.url).supportsTableSample) {
+ if (jdbcOptions.pushDownTableSample && dialect.supportsTableSample) {
Review Comment:
shall we make two more changes?
1. remove the JDBC option for pushdown (false by default AFAIK)
2. jdbc dialect should return false in these new pushdown APIs, to keep the old false by default behavior.
3. override the pushdown APIs in builtin dialects if we have tests for them.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/jdbc/JDBCScanBuilder.scala:
##########
@@ -126,24 +126,23 @@ case class JDBCScanBuilder(
upperBound: Double,
withReplacement: Boolean,
seed: Long): Boolean = {
- if (jdbcOptions.pushDownTableSample &&
- JdbcDialects.get(jdbcOptions.url).supportsTableSample) {
+ if (jdbcOptions.pushDownTableSample && dialect.supportsTableSample) {
Review Comment:
shall we make a few more changes?
1. remove the JDBC option for pushdown (false by default AFAIK)
2. jdbc dialect should return false in these new pushdown APIs, to keep the old false by default behavior.
3. override the pushdown APIs in builtin dialects if we have tests for them.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #39954: [SPARK-42289][SQL] DS V2 pushdown could let JDBC dialect decide to push down offset and limit
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #39954:
URL: https://github.com/apache/spark/pull/39954#discussion_r1116461352
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala:
##########
@@ -557,6 +557,20 @@ abstract class JdbcDialect extends Serializable with Logging {
def getJdbcSQLQueryBuilder(options: JDBCOptions): JdbcSQLQueryBuilder =
new JdbcSQLQueryBuilder(this, options)
+ /**
+ * Returns ture if dialect supports LIMIT clause.
+ *
+ * Note: Some build-in dialect supports LIMIT clause with some trick, please see:
+ * {@link OracleDialect.OracleSQLQueryBuilder} and
+ * {@link MsSqlServerDialect.MsSqlServerSQLQueryBuilder}.
+ */
+ def supportsLimit: Boolean = true
+
+ /**
+ * Returns ture if dialect supports OFFSET clause.
+ */
+ def supportsOffset: Boolean = true
Review Comment:
given that we will turn on the JDBC option to enable pushdown soon.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on pull request #39954: [SPARK-42289][SQL] DS V2 pushdown could let JDBC dialect decide to push down offset and limit
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #39954:
URL: https://github.com/apache/spark/pull/39954#issuecomment-1442775690
@cloud-fan @huaxingao Thank you!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #39954: [SPARK-42289][SQL] DS V2 pushdown could let JDBC dialect decide to push down offset and limit
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #39954:
URL: https://github.com/apache/spark/pull/39954#discussion_r1114255805
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/jdbc/JDBCScanBuilder.scala:
##########
@@ -126,24 +126,23 @@ case class JDBCScanBuilder(
upperBound: Double,
withReplacement: Boolean,
seed: Long): Boolean = {
- if (jdbcOptions.pushDownTableSample &&
- JdbcDialects.get(jdbcOptions.url).supportsTableSample) {
+ if (jdbcOptions.pushDownTableSample && dialect.supportsTableSample) {
Review Comment:
shall we make two more changes?
1. remove the JDBC option for pushdown (false by default AFAIK)
2. jdbc dialect should return false in these new pushdown APIs, to keep the old false by default behavior.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on a diff in pull request #39954: [SPARK-42289][SQL] DS V2 pushdown could let JDBC dialect decide to push down offset and limit
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #39954:
URL: https://github.com/apache/spark/pull/39954#discussion_r1115191715
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/jdbc/JDBCScanBuilder.scala:
##########
@@ -126,24 +126,23 @@ case class JDBCScanBuilder(
upperBound: Double,
withReplacement: Boolean,
seed: Long): Boolean = {
- if (jdbcOptions.pushDownTableSample &&
- JdbcDialects.get(jdbcOptions.url).supportsTableSample) {
+ if (jdbcOptions.pushDownTableSample && dialect.supportsTableSample) {
Review Comment:
OK. Let's do it in next PR.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] huaxingao commented on a diff in pull request #39954: [SPARK-42289][SQL] DS V2 pushdown could let JDBC dialect decide to push down offset and limit
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #39954:
URL: https://github.com/apache/spark/pull/39954#discussion_r1113730251
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala:
##########
@@ -557,6 +557,20 @@ abstract class JdbcDialect extends Serializable with Logging {
def getJdbcSQLQueryBuilder(options: JDBCOptions): JdbcSQLQueryBuilder =
new JdbcSQLQueryBuilder(this, options)
+ /**
+ * Returns ture if dialect supports LIMIT clause.
+ *
+ * Note: Some build-in dialect supports LIMIT clause with some trick, please see:
+ * {@link OracleDialect.OracleSQLQueryBuilder} and
+ * {@link MsSqlServerDialect.MsSqlServerSQLQueryBuilder}.
+ */
+ def supportsLimit: Boolean = true
+
+ /**
+ * Returns ture if dialect supports OFFSET clause.
+ */
+ def supportsOffset: Boolean = true
Review Comment:
Do all the dialects support offset? If not, do we need to set this to false in the dialect?
BTW you still have this `TODO[SPARK-42289]: DS V2 pushdown could let JDBC dialect decide to push down offset` in `OracleDialect` and `MsSqlServerDialect`
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan closed pull request #39954: [SPARK-42289][SQL] DS V2 pushdown could let JDBC dialect decide to push down offset and limit
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #39954: [SPARK-42289][SQL] DS V2 pushdown could let JDBC dialect decide to push down offset and limit
URL: https://github.com/apache/spark/pull/39954
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on a diff in pull request #39954: [SPARK-42289][SQL] DS V2 pushdown could let JDBC dialect decide to push down offset and limit
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #39954:
URL: https://github.com/apache/spark/pull/39954#discussion_r1113748723
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala:
##########
@@ -557,6 +557,20 @@ abstract class JdbcDialect extends Serializable with Logging {
def getJdbcSQLQueryBuilder(options: JDBCOptions): JdbcSQLQueryBuilder =
new JdbcSQLQueryBuilder(this, options)
+ /**
+ * Returns ture if dialect supports LIMIT clause.
+ *
+ * Note: Some build-in dialect supports LIMIT clause with some trick, please see:
+ * {@link OracleDialect.OracleSQLQueryBuilder} and
+ * {@link MsSqlServerDialect.MsSqlServerSQLQueryBuilder}.
+ */
+ def supportsLimit: Boolean = true
+
+ /**
+ * Returns ture if dialect supports OFFSET clause.
+ */
+ def supportsOffset: Boolean = true
Review Comment:
Most databases can directly or indirectly support offset. so I think we should keep true 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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org