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/03/13 10:57:13 UTC
[GitHub] [spark] beliefer opened a new pull request, #40396: [SPARK-42772][SQL] Change the default value of JDBC options about push down to true
beliefer opened a new pull request, #40396:
URL: https://github.com/apache/spark/pull/40396
### What changes were proposed in this pull request?
Currently, DS V2 pushdown could let JDBC dialect decide to push down `OFFSET`, `LIMIT` and table sample. Because some databases doesn't support one of them, so we should change the default value of these pushdown API false. If one database support the syntax, the JDBC dialect should overwrite the value.
We also have a lot of JDBC options about push down, such as `pushDownOffset`. Users could change the option value to allow or disallow push down.
### Why are the changes needed?
This PR change all JDBC v2 pushdown options to true and change all the dialect's pushdown API to false.
### Does this PR introduce _any_ user-facing change?
'Yes'.
The default behavior of pushdown framework is not push down SQL syntax to JDBC data source.
Users could control the pushdown enable or disable with JDBC options about push down, such as `pushDownOffset`.
### How was this patch tested?
Test cases updated.
--
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] dongjoon-hyun commented on a diff in pull request #40396: [SPARK-42772][SQL] Change the default value of JDBC options about push down to true
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #40396:
URL: https://github.com/apache/spark/pull/40396#discussion_r1136512197
##########
docs/sql-migration-guide.md:
##########
@@ -22,6 +22,10 @@ license: |
* Table of contents
{:toc}
+## Upgrading from Spark SQL 3.4 to 3.5
+
+- Since Spark 3.5, the JDBC options related to DS V2 pushdown are `true` by default. e.g. `pushDownAggregate`. To restore the legacy behavior, please set them to `false`. e.g. set `spark.sql.catalog.your_catalog_name.pushDownAggregate` to `false`.
Review Comment:
- `default. e.g.` -> `default, e.g.`
- We had better enumerate all changes for the end users. If we mentioned only one configuration, they have no idea how to recover the others.
--
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] dongjoon-hyun commented on a diff in pull request #40396: [SPARK-42772][SQL] Change the default value of JDBC options about push down to true
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #40396:
URL: https://github.com/apache/spark/pull/40396#discussion_r1136637368
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/MsSqlServerDialect.scala:
##########
@@ -212,5 +212,5 @@ private object MsSqlServerDialect extends JdbcDialect {
override def getJdbcSQLQueryBuilder(options: JDBCOptions): JdbcSQLQueryBuilder =
new MsSqlServerSQLQueryBuilder(this, options)
- override def supportsOffset: Boolean = false
+ override def supportsLimit: Boolean = true
Review Comment:
We remove `supportsOffset`?
--
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] dongjoon-hyun commented on a diff in pull request #40396: [SPARK-42772][SQL] Change the default value of JDBC options about push down to true
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #40396:
URL: https://github.com/apache/spark/pull/40396#discussion_r1136632844
##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/DB2IntegrationSuite.scala:
##########
@@ -61,9 +61,6 @@ class DB2IntegrationSuite extends DockerJDBCIntegrationV2Suite with V2JDBCTest {
override def sparkConf: SparkConf = super.sparkConf
.set("spark.sql.catalog.db2", classOf[JDBCTableCatalog].getName)
.set("spark.sql.catalog.db2.url", db.getJdbcUrl(dockerIp, externalPort))
- .set("spark.sql.catalog.db2.pushDownAggregate", "true")
- .set("spark.sql.catalog.db2.pushDownLimit", "true")
- .set("spark.sql.catalog.db2.pushDownOffset", "true")
Review Comment:
Could you revert this? Usually, we keep these explicitly because this is a kind of prerequisite of the test case. It's also good for code readability.
##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MsSqlServerIntegrationSuite.scala:
##########
@@ -58,8 +58,6 @@ class MsSqlServerIntegrationSuite extends DockerJDBCIntegrationV2Suite with V2JD
override def sparkConf: SparkConf = super.sparkConf
.set("spark.sql.catalog.mssql", classOf[JDBCTableCatalog].getName)
.set("spark.sql.catalog.mssql.url", db.getJdbcUrl(dockerIp, externalPort))
- .set("spark.sql.catalog.mssql.pushDownAggregate", "true")
- .set("spark.sql.catalog.mssql.pushDownLimit", "true")
Review Comment:
ditto.
--
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 #40396: [SPARK-42772][SQL] Change the default value of JDBC options about push down to true
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #40396: [SPARK-42772][SQL] Change the default value of JDBC options about push down to true
URL: https://github.com/apache/spark/pull/40396
--
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 #40396: [SPARK-42772][SQL] Change the default value of JDBC options about push down to true
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #40396:
URL: https://github.com/apache/spark/pull/40396#issuecomment-1471116456
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] beliefer commented on a diff in pull request #40396: [SPARK-42772][SQL] Change the default value of JDBC options about push down to true
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #40396:
URL: https://github.com/apache/spark/pull/40396#discussion_r1136664231
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala:
##########
@@ -583,12 +583,16 @@ abstract class JdbcDialect extends Serializable with Logging {
* {@link OracleDialect.OracleSQLQueryBuilder} and
* {@link MsSqlServerDialect.MsSqlServerSQLQueryBuilder}.
*/
- def supportsLimit: Boolean = true
+ def supportsLimit: Boolean = false
Review Comment:
`supportsLimit` is a new API and it is not in branch-3.4
--
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 #40396: [SPARK-42772][SQL] Change the default value of JDBC options about push down to true
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #40396:
URL: https://github.com/apache/spark/pull/40396#issuecomment-1471226525
@dongjoon-hyun @cloud-fan @huaxingao @sadikovi 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] beliefer commented on a diff in pull request #40396: [SPARK-42772][SQL] Change the default value of JDBC options about push down to true
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #40396:
URL: https://github.com/apache/spark/pull/40396#discussion_r1136607610
##########
docs/sql-migration-guide.md:
##########
@@ -22,6 +22,10 @@ license: |
* Table of contents
{:toc}
+## Upgrading from Spark SQL 3.4 to 3.5
+
+- Since Spark 3.5, the JDBC options related to DS V2 pushdown are `true` by default. e.g. `pushDownAggregate`. To restore the legacy behavior, please set them to `false`. e.g. set `spark.sql.catalog.your_catalog_name.pushDownAggregate` to `false`.
Review Comment:
It hears good to me.
--
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 #40396: [SPARK-42772][SQL] Change the default value of JDBC options about push down to true
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #40396:
URL: https://github.com/apache/spark/pull/40396#discussion_r1136665341
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/MsSqlServerDialect.scala:
##########
@@ -212,5 +212,5 @@ private object MsSqlServerDialect extends JdbcDialect {
override def getJdbcSQLQueryBuilder(options: JDBCOptions): JdbcSQLQueryBuilder =
new MsSqlServerSQLQueryBuilder(this, options)
- override def supportsOffset: Boolean = false
+ override def supportsLimit: Boolean = true
Review Comment:
Because the default value is false now.
--
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 #40396: [SPARK-42772][SQL] Change the default value of JDBC options about push down to true
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #40396:
URL: https://github.com/apache/spark/pull/40396#issuecomment-1469534683
> Thank you for update. BTW, `JDBCDialect` is a documented developer API. We should not change the default value. We should keep the original default value to avoid a breaking change to 3rd party Dialect.
>
Thank you for the reminder. But these new API only exists in master branch.
--
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 #40396: [SPARK-42772][SQL] Change the default value of JDBC options about push down to true
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #40396:
URL: https://github.com/apache/spark/pull/40396#issuecomment-1469076028
Yea, we should mention that they can set these options to false if query fails with JDBC errors.
--
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 #40396: [SPARK-42772][SQL] Change the default value of JDBC options about push down to true
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #40396:
URL: https://github.com/apache/spark/pull/40396#issuecomment-1465922700
ping @huaxingao cc @cloud-fan @sadikovi
--
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] dongjoon-hyun commented on a diff in pull request #40396: [SPARK-42772][SQL] Change the default value of JDBC options about push down to true
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #40396:
URL: https://github.com/apache/spark/pull/40396#discussion_r1136636035
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala:
##########
@@ -583,12 +583,16 @@ abstract class JdbcDialect extends Serializable with Logging {
* {@link OracleDialect.OracleSQLQueryBuilder} and
* {@link MsSqlServerDialect.MsSqlServerSQLQueryBuilder}.
*/
- def supportsLimit: Boolean = true
+ def supportsLimit: Boolean = false
Review Comment:
This looks like a breaking change for the 3rd party JDBC Dialect. Please revert this.
--
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] sadikovi commented on a diff in pull request #40396: [SPARK-42772][SQL] Change the default value of JDBC options about push down to true
Posted by "sadikovi (via GitHub)" <gi...@apache.org>.
sadikovi commented on code in PR #40396:
URL: https://github.com/apache/spark/pull/40396#discussion_r1134891054
##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/PostgresIntegrationSuite.scala:
##########
@@ -49,10 +49,6 @@ class PostgresIntegrationSuite extends DockerJDBCIntegrationV2Suite with V2JDBCT
override def sparkConf: SparkConf = super.sparkConf
.set("spark.sql.catalog.postgresql", classOf[JDBCTableCatalog].getName)
.set("spark.sql.catalog.postgresql.url", db.getJdbcUrl(dockerIp, externalPort))
- .set("spark.sql.catalog.postgresql.pushDownTableSample", "true")
Review Comment:
IMHO, I would still keep those configs in the tests, makes it clear that the configs need to be enabled in the test but it is fine either way for me.
--
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] dongjoon-hyun commented on a diff in pull request #40396: [SPARK-42772][SQL] Change the default value of JDBC options about push down to true
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #40396:
URL: https://github.com/apache/spark/pull/40396#discussion_r1136633973
##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/PostgresIntegrationSuite.scala:
##########
@@ -49,10 +49,6 @@ class PostgresIntegrationSuite extends DockerJDBCIntegrationV2Suite with V2JDBCT
override def sparkConf: SparkConf = super.sparkConf
.set("spark.sql.catalog.postgresql", classOf[JDBCTableCatalog].getName)
.set("spark.sql.catalog.postgresql.url", db.getJdbcUrl(dockerIp, externalPort))
- .set("spark.sql.catalog.postgresql.pushDownTableSample", "true")
Review Comment:
Let's keep these prerequisites of the 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] dongjoon-hyun commented on a diff in pull request #40396: [SPARK-42772][SQL] Change the default value of JDBC options about push down to true
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #40396:
URL: https://github.com/apache/spark/pull/40396#discussion_r1137367307
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala:
##########
@@ -583,12 +583,16 @@ abstract class JdbcDialect extends Serializable with Logging {
* {@link OracleDialect.OracleSQLQueryBuilder} and
* {@link MsSqlServerDialect.MsSqlServerSQLQueryBuilder}.
*/
- def supportsLimit: Boolean = true
+ def supportsLimit: Boolean = false
Review Comment:
Oh, got it~
--
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 #40396: [SPARK-42772][SQL] Change the default value of JDBC options about push down to true
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #40396:
URL: https://github.com/apache/spark/pull/40396#discussion_r1135459578
##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/PostgresIntegrationSuite.scala:
##########
@@ -49,10 +49,6 @@ class PostgresIntegrationSuite extends DockerJDBCIntegrationV2Suite with V2JDBCT
override def sparkConf: SparkConf = super.sparkConf
.set("spark.sql.catalog.postgresql", classOf[JDBCTableCatalog].getName)
.set("spark.sql.catalog.postgresql.url", db.getJdbcUrl(dockerIp, externalPort))
- .set("spark.sql.catalog.postgresql.pushDownTableSample", "true")
Review Comment:
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] dongjoon-hyun commented on a diff in pull request #40396: [SPARK-42772][SQL] Change the default value of JDBC options about push down to true
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #40396:
URL: https://github.com/apache/spark/pull/40396#discussion_r1136636446
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala:
##########
@@ -583,12 +583,16 @@ abstract class JdbcDialect extends Serializable with Logging {
* {@link OracleDialect.OracleSQLQueryBuilder} and
* {@link MsSqlServerDialect.MsSqlServerSQLQueryBuilder}.
*/
- def supportsLimit: Boolean = true
+ def supportsLimit: Boolean = false
/**
* Returns ture if dialect supports OFFSET clause.
+ *
+ * Note: Some build-in dialect supports OFFSET clause with some trick, please see:
+ * {@link OracleDialect.OracleSQLQueryBuilder} and
+ * {@link MySQLDialect.MySQLSQLQueryBuilder}.
*/
- def supportsOffset: Boolean = true
+ def supportsOffset: Boolean = false
Review Comment:
ditto. We should not change `JdbcDialects`.
--
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] dongjoon-hyun commented on a diff in pull request #40396: [SPARK-42772][SQL] Change the default value of JDBC options about push down to true
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #40396:
URL: https://github.com/apache/spark/pull/40396#discussion_r1136632994
##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MySQLIntegrationSuite.scala:
##########
@@ -54,9 +54,6 @@ class MySQLIntegrationSuite extends DockerJDBCIntegrationV2Suite with V2JDBCTest
override def sparkConf: SparkConf = super.sparkConf
.set("spark.sql.catalog.mysql", classOf[JDBCTableCatalog].getName)
.set("spark.sql.catalog.mysql.url", db.getJdbcUrl(dockerIp, externalPort))
- .set("spark.sql.catalog.mysql.pushDownAggregate", "true")
- .set("spark.sql.catalog.mysql.pushDownLimit", "true")
- .set("spark.sql.catalog.mysql.pushDownOffset", "true")
Review Comment:
ditto.
##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/OracleIntegrationSuite.scala:
##########
@@ -75,9 +75,6 @@ class OracleIntegrationSuite extends DockerJDBCIntegrationV2Suite with V2JDBCTes
override def sparkConf: SparkConf = super.sparkConf
.set("spark.sql.catalog.oracle", classOf[JDBCTableCatalog].getName)
.set("spark.sql.catalog.oracle.url", db.getJdbcUrl(dockerIp, externalPort))
- .set("spark.sql.catalog.oracle.pushDownAggregate", "true")
- .set("spark.sql.catalog.oracle.pushDownLimit", "true")
- .set("spark.sql.catalog.oracle.pushDownOffset", "true")
Review Comment:
ditto.
--
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 #40396: [SPARK-42772][SQL] Change the default value of JDBC options about push down to true
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #40396:
URL: https://github.com/apache/spark/pull/40396#discussion_r1136664661
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala:
##########
@@ -583,12 +583,16 @@ abstract class JdbcDialect extends Serializable with Logging {
* {@link OracleDialect.OracleSQLQueryBuilder} and
* {@link MsSqlServerDialect.MsSqlServerSQLQueryBuilder}.
*/
- def supportsLimit: Boolean = true
+ def supportsLimit: Boolean = false
/**
* Returns ture if dialect supports OFFSET clause.
+ *
+ * Note: Some build-in dialect supports OFFSET clause with some trick, please see:
+ * {@link OracleDialect.OracleSQLQueryBuilder} and
+ * {@link MySQLDialect.MySQLSQLQueryBuilder}.
*/
- def supportsOffset: Boolean = true
+ def supportsOffset: Boolean = false
Review Comment:
`supportsOffset` just in master branch.
--
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