You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by GitBox <gi...@apache.org> on 2022/01/03 13:59:57 UTC
[GitHub] [drill] vvysotskyi opened a new pull request #2420: DRILL-8090: LIMIT clause is pushed down to an invalid OFFSET-FETCH clause for MS SQL Server
vvysotskyi opened a new pull request #2420:
URL: https://github.com/apache/drill/pull/2420
# [DRILL-8090](https://issues.apache.org/jira/browse/DRILL-8090): LIMIT clause is pushed down to an invalid OFFSET-FETCH clause for MS SQL Server
## Description
- Updated Calcite fork version to include https://github.com/apache/calcite/commit/cc40a48cb8ca16f91bfdc66eaed6151805355d4b, so now regular limit can be pushed down to MS SQL as `TOP N` instead of `FETCH`.
- Updated `JdbcLimitRule` and `JdbcSortRule` to prevent pushing down `FETCH` with `OFFSET` and without `ORDER BY`.
For such a case, some rules at the physical stage will generate a limit on top of the scan that includes `FETCH` only and another limit with `FETCH` and `OFFSET` above, so the limit will be pushed down.
- Allowed matching JDBC rules for physical rel nodes.
- Fixed issue with ClassCastException for Phoenix plugin (issue similar to DRILL-7972).
## Documentation
NA
## Testing
Checked manually.
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on a change in pull request #2420: DRILL-8090: LIMIT clause is pushed down to an invalid OFFSET-FETCH clause for MS SQL Server
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2420:
URL: https://github.com/apache/drill/pull/2420#discussion_r777680441
##########
File path: contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/rules/JdbcLimitRule.java
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.store.jdbc.rules;
+
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.plan.RelTrait;
+import org.apache.calcite.rel.RelCollations;
+import org.apache.calcite.sql.dialect.MssqlSqlDialect;
+import org.apache.drill.exec.planner.common.DrillLimitRelBase;
+import org.apache.drill.exec.store.enumerable.plan.DrillJdbcRuleBase;
+import org.apache.drill.exec.store.jdbc.DrillJdbcConvention;
+
+public class JdbcLimitRule extends DrillJdbcRuleBase.DrillJdbcLimitRule {
+ private final DrillJdbcConvention convention;
+
+ public JdbcLimitRule(RelTrait in, DrillJdbcConvention out) {
+ super(in, out);
+ this.convention = out;
+ }
+
+ @Override
+ public boolean matches(RelOptRuleCall call) {
+ DrillLimitRelBase limit = call.rel(0);
+ if (super.matches(call)) {
+ return limit.getOffset() == null
+ || !limit.getTraitSet().contains(RelCollations.EMPTY)
+ || !(convention.getPlugin().getDialect() instanceof MssqlSqlDialect);
Review comment:
Thanks, added.
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] jnturton commented on a change in pull request #2420: DRILL-8090: LIMIT clause is pushed down to an invalid OFFSET-FETCH clause for MS SQL Server
Posted by GitBox <gi...@apache.org>.
jnturton commented on a change in pull request #2420:
URL: https://github.com/apache/drill/pull/2420#discussion_r777624425
##########
File path: contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/rules/JdbcLimitRule.java
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.store.jdbc.rules;
+
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.plan.RelTrait;
+import org.apache.calcite.rel.RelCollations;
+import org.apache.calcite.sql.dialect.MssqlSqlDialect;
+import org.apache.drill.exec.planner.common.DrillLimitRelBase;
+import org.apache.drill.exec.store.enumerable.plan.DrillJdbcRuleBase;
+import org.apache.drill.exec.store.jdbc.DrillJdbcConvention;
+
+public class JdbcLimitRule extends DrillJdbcRuleBase.DrillJdbcLimitRule {
+ private final DrillJdbcConvention convention;
+
+ public JdbcLimitRule(RelTrait in, DrillJdbcConvention out) {
+ super(in, out);
+ this.convention = out;
+ }
+
+ @Override
+ public boolean matches(RelOptRuleCall call) {
+ DrillLimitRelBase limit = call.rel(0);
+ if (super.matches(call)) {
+ return limit.getOffset() == null
+ || !limit.getTraitSet().contains(RelCollations.EMPTY)
+ || !(convention.getPlugin().getDialect() instanceof MssqlSqlDialect);
Review comment:
Perhaps some kind of comment here would be helpful to someone who visits this file not knowing why MS SQL gets special treatment?
`// LIMIT is translated to TOP for MS SQL, c.f. DRILL-8090`
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on pull request #2420: DRILL-8090: LIMIT clause is pushed down to an invalid OFFSET-FETCH clause for MS SQL Server
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on pull request #2420:
URL: https://github.com/apache/drill/pull/2420#issuecomment-1004379219
@cgivre, I think something like [Parameterized tests](https://github.com/junit-team/junit4/wiki/parameterized-tests) could help to do that.
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre merged pull request #2420: DRILL-8090: LIMIT clause is pushed down to an invalid OFFSET-FETCH clause for MS SQL Server
Posted by GitBox <gi...@apache.org>.
cgivre merged pull request #2420:
URL: https://github.com/apache/drill/pull/2420
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on pull request #2420: DRILL-8090: LIMIT clause is pushed down to an invalid OFFSET-FETCH clause for MS SQL Server
Posted by GitBox <gi...@apache.org>.
cgivre commented on pull request #2420:
URL: https://github.com/apache/drill/pull/2420#issuecomment-1004274089
One other thing, I'd like to add UTs for MS-SQL to the JDBC plugin for both reading and writing. But not in this task ;-)
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on pull request #2420: DRILL-8090: LIMIT clause is pushed down to an invalid OFFSET-FETCH clause for MS SQL Server
Posted by GitBox <gi...@apache.org>.
cgivre commented on pull request #2420:
URL: https://github.com/apache/drill/pull/2420#issuecomment-1004366424
> @cgivre, yes, it is a good idea. I've noticed that we have a lot of common test cases for different databases. It would be good to refactor those tests to avoid copying them.
How did you have in mind? When I wrote the JDBC reader, I added tests for Postgres and one other. I duplicated all the writer tests for a bunch of databases. Is there a more efficient way?
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on pull request #2420: DRILL-8090: LIMIT clause is pushed down to an invalid OFFSET-FETCH clause for MS SQL Server
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on pull request #2420:
URL: https://github.com/apache/drill/pull/2420#issuecomment-1004328836
@cgivre, yes, it is a good idea. I've noticed that we have a lot of common test cases for different databases. It would be good to refactor those tests to avoid copying 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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org